Re: null iv parameter passed to combo_init()

On Sat, Jan 08, 2022 at 06:52:14PM -0800, Zhihong Yu wrote:
> On Sat, Jan 8, 2022 at 5:52 PM Noah Misch <noah@leadboat.com> wrote:
> > On Fri, Jan 07, 2022 at 04:32:01PM -0800, Zhihong Yu wrote:

> > I agree it's time to fix cases like this, given
> > https://postgr.es/m/flat/20200904023648.GB3426768@rfd.leadboat.com.  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 ?

On further thought, I would write it 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 helps in two ways.  First, if someone passes iv==NULL and ivlen!=0, my
version will tend to crash, but yours will treat that like ivlen==0.  Since
this would be a programming error, crashing is better.  Second, a compiler can
opt to omit the "ivlen != 0" test from the generated assembly, because the
compiler can know that memcpy(any_value_a, any_value_b, 0) is a no-op.

> 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.

To check whether you've gotten them all, configure with CC='gcc
-fsanitize=undefined -fsanitize-undefined-trap-on-error' and run check-world.