Re: [PATCH v3] Monitor: use devname as char array instead of pointer

[Kinga Tanska <kinga.tanska@linux.intel.com>]

On Sat, 25 Jun 2022 15:03:32 +0800
Coly Li <colyli@suse.de> wrote:

> > 2022年6月20日 22:06，Kinga Tanska <kinga.tanska@intel.com> 写道：
> > 
> > Device name wasn't filled properly due to incorrect use of strcpy.
> > Strcpy was used twice. Firstly to fill devname with "/dev/md/"
> > and then to add chosen name. First strcpy result was overwritten by
> > second one (as a result <device_name> instead of
> > "/dev/md/<device_name>" was assigned). This commit changes this
> > implementation to use snprintf and devname with fixed size. Also
> > safer string functions are propagated.
> > 
> > Signed-off-by: Kinga Tanska <kinga.tanska@intel.com>
> > ---
> > Monitor.c | 30 +++++++++++-------------------
> > 1 file changed, 11 insertions(+), 19 deletions(-)
> > 
> > diff --git a/Monitor.c b/Monitor.c
> > index 6ca1ebe5..04112791 100644
> > --- a/Monitor.c
> > +++ b/Monitor.c
> > @@ -33,8 +33,8 @@
> > #endif
> > 
> > struct state {
> > -	char *devname;
> > -	char devnm[32];	/* to sync with mdstat info */
> > +	char devname[MD_NAME_MAX + 8];
> > +	char devnm[MD_NAME_MAX];	/* to sync with mdstat
> > info */ unsigned int utime;
> > 	int err;
> > 	char *spare_group;
> > @@ -45,7 +45,7 @@ struct state {
> > 	int devstate[MAX_DISKS];
> > 	dev_t devid[MAX_DISKS];
> > 	int percent;
> > -	char parent_devnm[32]; /* For subarray, devnm of parent.
> > +	char parent_devnm[MD_NAME_MAX]; /* For subarray, devnm of
> > parent.
> > 				* For others, ""
> > 				*/
> > 	struct supertype *metadata;
> > @@ -187,13 +187,7 @@ int Monitor(struct mddev_dev *devlist,
> > 				continue;
> > 
> > 			st = xcalloc(1, sizeof *st);
> > -			if (mdlist->devname[0] == '/')
> > -				st->devname =
> > xstrdup(mdlist->devname);
> > -			else {
> > -				st->devname =
> > xmalloc(8+strlen(mdlist->devname)+1);
> > -				strcpy(strcpy(st->devname,
> > "/dev/md/"),
> > -				       mdlist->devname);
> > -			}
> > +			snprintf(st->devname, MD_NAME_MAX + 8,
> > "/dev/md/%s", basename(mdlist->devname));  
> 
> Hi Kinga,
> 
> The above location is what the whole patch wants to fix. It is not
> worth to change other locations. IMHO the change set is a bit large
> than what it should be, just a suggestion maybe a following style
> change is enough (not test it),
> 
> @@ -190,9 +190,13 @@ int Monitor(struct mddev_dev *devlist,
>                         if (mdlist->devname[0] == '/')
>                                 st->devname =
> xstrdup(mdlist->devname); else {
> -                               st->devname =
> xmalloc(8+strlen(mdlist->devname)+1);
> -                               strcpy(strcpy(st->devname,
> "/dev/md/"),
> -                                      mdlist->devname);
> +                               int _len;
> +
> +                               _len = 8 + strlen(mdlist->devname);
> +                               st->devname = xmalloc(_len + 1);
> +                               memset(st->devname, 0, _len + 1);
> +                               snprintf(st->devname, _len,
> "/dev/md/%s",
> +                                        mdlist->devname);
>                         }
>                         if (!is_mddev(mdlist->devname))
>                                 return 1;
> 
> Thanks.
> 
> Coly Li
> 
> 
> 
> > 			if (!is_mddev(mdlist->devname))
> > 				return 1;
> > 			st->next = statelist;
> > @@ -216,7 +210,7 @@ int Monitor(struct mddev_dev *devlist,
> > 
> > 			st = xcalloc(1, sizeof *st);
> > 			mdlist = conf_get_ident(dv->devname);
> > -			st->devname = xstrdup(dv->devname);
> > +			snprintf(st->devname, MD_NAME_MAX + 8,
> > "%s", dv->devname); st->next = statelist;
> > 			st->devnm[0] = 0;
> > 			st->percent = RESYNC_UNKNOWN;
> > @@ -299,7 +293,6 @@ int Monitor(struct mddev_dev *devlist,
> > 		for (stp = &statelist; (st = *stp) != NULL; ) {
> > 			if (st->from_auto && st->err > 5) {
> > 				*stp = st->next;
> > -				free(st->devname);
> > 				free(st->spare_group);
> > 				free(st);
> > 			} else
> > @@ -552,7 +545,7 @@ static int check_array(struct state *st, struct
> > mdstat_ent *mdstat, goto disappeared;
> > 
> > 	if (st->devnm[0] == 0)
> > -		strcpy(st->devnm, fd2devnm(fd));
> > +		snprintf(st->devnm, MD_NAME_MAX, "%s",
> > fd2devnm(fd));
> > 
> > 	for (mse2 = mdstat; mse2; mse2 = mse2->next)
> > 		if (strcmp(mse2->devnm, st->devnm) == 0) {
> > @@ -682,7 +675,7 @@ static int check_array(struct state *st, struct
> > mdstat_ent *mdstat, strncmp(mse->metadata_version, "external:", 9)
> > == 0 && is_subarray(mse->metadata_version+9)) {
> > 		char *sl;
> > -		strcpy(st->parent_devnm, mse->metadata_version +
> > 10);
> > +		snprintf(st->parent_devnm, MD_NAME_MAX, "%s",
> > mse->metadata_version + 10); sl = strchr(st->parent_devnm, '/');
> > 		if (sl)
> > 			*sl = 0;
> > @@ -770,14 +763,13 @@ static int add_new_arrays(struct mdstat_ent
> > *mdstat, struct state **statelist, continue;
> > 			}
> > 
> > -			st->devname = xstrdup(name);
> > +			snprintf(st->devname, MD_NAME_MAX + 8,
> > "%s", name); if ((fd = open(st->devname, O_RDONLY)) < 0 ||
> > 			    md_get_array_info(fd, &array) < 0) {
> > 				/* no such array */
> > 				if (fd >= 0)
> > 					close(fd);
> > 				put_md_name(st->devname);
> > -				free(st->devname);
> > 				if (st->metadata) {
> > 					st->metadata->ss->free_super(st->metadata);
> > 					free(st->metadata);
> > @@ -789,7 +781,7 @@ static int add_new_arrays(struct mdstat_ent
> > *mdstat, struct state **statelist, st->next = *statelist;
> > 			st->err = 1;
> > 			st->from_auto = 1;
> > -			strcpy(st->devnm, mse->devnm);
> > +			snprintf(st->devnm, MD_NAME_MAX, "%s",
> > mse->devnm); st->percent = RESYNC_UNKNOWN;
> > 			st->expected_spares = -1;
> > 			if (mse->metadata_version &&
> > @@ -797,8 +789,8 @@ static int add_new_arrays(struct mdstat_ent
> > *mdstat, struct state **statelist, "external:", 9) == 0 &&
> > 			    is_subarray(mse->metadata_version+9)) {
> > 				char *sl;
> > -				strcpy(st->parent_devnm,
> > -					mse->metadata_version+10);
> > +				snprintf(st->parent_devnm,
> > MD_NAME_MAX,
> > +					 "%s",
> > mse->metadata_version + 10); sl = strchr(st->parent_devnm, '/');
> > 				*sl = 0;
> > 			} else
> > -- 
> > 2.26.2
> >   
> 

Hi Coly,

Yes, fragment of code, which you selected, is enough to fix it. I have
changed also 32 to define, and fixed unsafe functions in this file to
improve code quality and avoid magic number. What's your opinion, is
this change worth to add?
Would you mind getting two patches, one with this specific fix, and
other one with another improvements in this file?

Regards,
Kinga