Re: null iv parameter passed to combo_init()

On Sat, Jan 8, 2022 at 5:52 PM Noah Misch <noah(at)leadboat(dot)com> wrote:

> On Fri, Jan 07, 2022 at 04:32:01PM -0800, Zhihong Yu wrote:
> > In contrib/pgcrypto/pgcrypto.c :
> >
> > err = px_combo_init(c, (uint8 *) VARDATA_ANY(key), klen, NULL, 0);
> >
> > Note: NULL is passed as iv.
> >
> > When combo_init() is called,
> >
> > if (ivlen > ivs)
> > memcpy(ivbuf, iv, ivs);
> > else
> > memcpy(ivbuf, iv, ivlen);
> >
> > It seems we need to consider the case of null being passed as iv for
> > memcpy() because of this:
> >
> > /usr/include/string.h:44:28: note: nonnull attribute specified here
>
> I agree it's time to fix cases like this, given
> https://postgr.es/m/flat/20200904023648(dot)GB3426768(at)rfd(dot)leadboat(dot)com(dot)
> However,
> it should be one patch fixing all (or at least many) of them.

> > --- a/contrib/pgcrypto/px.c
> > +++ b/contrib/pgcrypto/px.c
> > @@ -198,10 +198,13 @@ combo_init(PX_Combo *cx, const uint8 *key,
> unsigned klen,
> > if (ivs > 0)
> > {
> > ivbuf = palloc0(ivs);
> > - if (ivlen > ivs)
> > - memcpy(ivbuf, iv, ivs);
> > - else
> > - memcpy(ivbuf, iv, ivlen);
> > + if (iv != NULL)
> > + {
> > + if (ivlen > ivs)
> > + memcpy(ivbuf, iv, ivs);
> > + else
> > + memcpy(ivbuf, iv, ivlen);
> > + }
> > }
>
> If someone were to pass NULL iv with nonzero ivlen, that will silently
>
Hi,
If iv is NULL, none of the memcpy() would be called (based on my patch).
Can you elaborate your suggestion in more detail ?

Patch v2 is attached, covering more files.

Since the referenced email was old, line numbers have changed.
It would be nice if an up-to-date list is provided in case more places
should be changed.

Cheers

> malfunction. I'd avoid that risk by writing this way:
>
> --- a/contrib/pgcrypto/px.c
> +++ b/contrib/pgcrypto/px.c
> @@ -202,3 +202,3 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned
> klen,
> memcpy(ivbuf, iv, ivs);
> - else
> + else if (ivlen > 0)
> memcpy(ivbuf, iv, ivlen);
>
> That also gives the compiler an additional optimization strategy.
>