public inbox for [email protected]  

help / color / mirror / Atom feed

From: Noah Misch <[email protected]>
To: Zhihong Yu <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: null iv parameter passed to combo_init()
Date: Sat, 8 Jan 2022 19:11:00 -0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALNJ-vTh9BXARHg3iaCAzr97Fws2W5aR6nRPJfT4u3VdKDzRTQ@mail.gmail.com>
References: <CALNJ-vSBb2Ees=KB0frYBh7foK-QNZMbK7Vz66bUJa09D+CHAw@mail.gmail.com>
	<[email protected]>
	<CALNJ-vTh9BXARHg3iaCAzr97Fws2W5aR6nRPJfT4u3VdKDzRTQ@mail.gmail.com>

On Sat, Jan 08, 2022 at 06:52:14PM -0800, Zhihong Yu wrote:
> On Sat, Jan 8, 2022 at 5:52 PM Noah Misch <[email protected]> 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/[email protected].  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.

view thread (3+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: null iv parameter passed to combo_init()
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox