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

[Coly Li <colyli@xxxxxxx>]

On 2/9/22 4:56 PM, Kinga Tanska wrote:
> Device name wasn't filled properly due to incorrect use of strcpy.

Could you point out the specific location for improper strcpy?


> Instead pointer, devname with fixed size is used and safer string
> functions are propagated.
>
> Signed-off-by: Kinga Tanska <kinga.tanska@xxxxxxxxx>
> ---
>   Monitor.c | 30 +++++++++++-------------------
>   1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/Monitor.c b/Monitor.c
> index 30c031a2..d02344ec 100644
> --- a/Monitor.c
> +++ b/Monitor.c
> @@ -34,8 +34,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;
> @@ -46,7 +46,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;
> @@ -184,13 +184,7 @@ int Monitor(struct mddev_dev *devlist,
>   			if (strcasecmp(mdlist->devname, "<ignore>") == 0)
>   				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));


I feel the above change is incorrect, the tailing '\0' of the string 
might be cut by your change.


>   			st->next = statelist;
>   			st->devnm[0] = 0;
>   			st->percent = RESYNC_UNKNOWN;
> @@ -206,7 +200,7 @@ int Monitor(struct mddev_dev *devlist,
>   		for (dv = devlist; dv; dv = dv->next) {
>   			struct state *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;
> @@ -289,7 +283,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
> @@ -540,7 +533,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) {
> @@ -670,7 +663,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;
> @@ -758,14 +751,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);
> @@ -777,7 +769,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 &&
> @@ -785,8 +777,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


With your change, the tailing '\0' for dev name might be cut. Could you 
please check whether it may introduce potential memleak ?


Thanks.


Coly Li