On 3/19/22 11:42, Coly Li wrote:
> 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 ?
Kinga,
Any update on this?
Thanks,
Jes
