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

["Tanska, Kinga" <kinga.tanska@xxxxxxxxxxxxxxx>]

Hi,

I will send correction in next patchset. I didn't include terminating null
byte '\0' to size in snprintf function, which can cause memleaks.
Also specific location of incorrect usage of strcpy will be pointed out 
in commit message.

Regards,
Kinga

On 4/4/2022 3:54 PM, Jes Sorensen wrote:

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