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(at)leadboat(dot)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(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 ?

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.