[dm-devel] [PATCH 07/21] libmultipath: use strbuf in parse_vpd_pg83()

[Martin Wilck]

On Mon, 2021-11-29 at 19:18 -0600, Benjamin Marzinski wrote:
> On Fri, Nov 19, 2021 at 12:13:24AM +0100, mwilck at suse.com wrote:
> > From: Martin Wilck <mwilck at suse.com>
> > 
> > By using the strbuf API, the code gets more readable, as we need
> > less output size checks.
> > 
> > While at it, avoid a possible crash by passing a NULL pointer
> > to memchr().
> > 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  libmultipath/discovery.c | 113 +++++++++++++++++------------------
> > ----
> >  1 file changed, 49 insertions(+), 64 deletions(-)
> > 
> > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> > index 5ce9031..16b6fae 100644
> > --- a/libmultipath/discovery.c
> > +++ b/libmultipath/discovery.c
> > @@ -1120,6 +1120,7 @@ parse_vpd_pg83(const unsigned char *in,
> > size_t in_len,
> >         size_t len, vpd_len, i;
> >         int vpd_type, prio = -1;
> >         int err = -ENODATA;
> > +       STRBUF_ON_STACK(buf);
> >  
> >          /* Need space at least for one digit */
> >         if (out_len <= 1)
> > @@ -1237,92 +1238,76 @@ parse_vpd_pg83(const unsigned char *in,
> > size_t in_len,
> >         if (vpd_type == 0x2 || vpd_type == 0x3) {
> >                 size_t i;
> >  
> > -               len = sprintf(out, "%d", vpd_type);
> > -               if (2 * vpd_len >= out_len - len) {
> > -                       condlog(1, "%s: WWID overflow, type %d,
> > %zu/%zu bytes required",
> > -                               __func__, vpd_type,
> > -                               2 * vpd_len + len + 1, out_len);
> > -                       vpd_len = (out_len - len - 1) / 2;
> > -               }
> > +               if ((err = print_strbuf(&buf, "%d", vpd_type)) < 0)
> > +                       return err;
> >                 for (i = 0; i < vpd_len; i++)
> > -                       len += sprintf(out + len,
> > -                                      "%02x", vpd[i]);
> > +                       if ((err = print_strbuf(&buf, "%02x",
> > vpd[i])) < 0)
> > +                               return err;
> >         } else if (vpd_type == 0x8) {
> > -               if (!memcmp("eui.", vpd, 4))
> > -                       out[0] =  '2';
> > +               char type;
> > +
> > +                if (!memcmp("eui.", vpd, 4))
> > +                       type =  '2';
> >                 else if (!memcmp("naa.", vpd, 4))
> > -                       out[0] = '3';
> > +                       type = '3';
> >                 else
> > -                       out[0] = '8';
> > +                       type = '8';
> > +               if ((err = fill_strbuf(&buf, type, 1)) < 0)
> > +                       return err;
> >  
> >                 vpd += 4;
> >                 len = vpd_len - 4;
> > -               while (len > 2 && vpd[len - 2] == '\0')
> > -                       --len;
> > -               if (len > out_len - 1) {
> > -                       condlog(1, "%s: WWID overflow, type 8/%c,
> > %zu/%zu bytes required",
> > -                               __func__, out[0], len + 1,
> > out_len);
> > -                       len = out_len - 1;
> > +               if ((err = __append_strbuf_str(&buf, (const char
> > *)vpd, len)) < 0)
> > +                       return err;
> > +
> > +                /* The input is 0-padded, make sure the length is
> > correct */
> > +               truncate_strbuf(&buf,
> > strlen(get_strbuf_str(&buf)));
> > +               len = get_strbuf_len(&buf);
> > +               if (type != '8') {
> > +                       char *buffer = __get_strbuf_buf(&buf);
> > +
> > +                       for (i = 0; i < len; ++i)
> > +                               buffer[i] = tolower(buffer[i]);
> >                 }
> >  
> > -               if (out[0] == '8')
> > -                       for (i = 0; i < len; ++i)
> > -                               out[1 + i] = vpd[i];
> > -               else
> > -                       for (i = 0; i < len; ++i)
> > -                               out[1 + i] = tolower(vpd[i]);
> > -
> > -               /* designator should be 0-terminated, but let's
> > make sure */
> > -               out[len] = '\0';
> > -
> >         } else if (vpd_type == 0x1) {
> >                 const unsigned char *p;
> >                 size_t p_len;
> >  
> > -               out[0] = '1';
> > -               len = 1;
> > -               while ((p = memchr(vpd, ' ', vpd_len))) {
> > +               if ((err = fill_strbuf(&buf, '1', 1)) < 0)
> > +                       return err;
> > +               while (vpd && (p = memchr(vpd, ' ', vpd_len))) {
> 
> vpd should never be zero here unless it wraps around, which seems
> very
> unlikely.  Is this just to make coverity happy, or do you mean
> (*vpd)?
> 

Coverity likes arguments for memchr() to be NULL-checked beforehand, 
but that wasn't the main reason.

See below, there's "vpd = p". And p can (and will sooner or later) be
NULL, because memchr() returns NULL if it doesn't find anything.


> -Ben
> 
> >                         p_len = p - vpd;
> > -                       if (len + p_len > out_len - 1) {
> > -                               condlog(1, "%s: WWID overflow, type
> > 1, %zu/%zu bytes required",
> > -                                       __func__, len + p_len,
> > out_len);
> > -                               p_len = out_len - len - 1;
> > -                       }
> > -                       memcpy(out + len, vpd, p_len);
> > -                       len += p_len;
> > -                       if (len >= out_len - 1) {
> > -                               out[len] = '\0';
> > -                               break;
> > -                       }
> > -                       out[len] = '_';
> > -                       len ++;
> > -                       if (len >= out_len - 1) {
> > -                               out[len] = '\0';
> > -                               break;
> > -                       }
> > +                       if ((err = __append_strbuf_str(&buf, (const
> > char *)vpd,
> > +                                                      p_len)) < 0)
> > +                               return err;
> >                         vpd = p;

Here.

Regards,
Martin