Re: null iv parameter passed to combo_init() - Mailing list pgsql-hackers

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

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.