On Mon, 2021-11-29 at 19:18 -0600, Benjamin Marzinski wrote:
> On Fri, Nov 19, 2021 at 12:13:24AM +0100, mwilck@xxxxxxxx wrote:
> > From: Martin Wilck <mwilck@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> > ---
> > 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
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
