On Sat, Jan 8, 2022 at 7:11 PM Noah Misch <n...@leadboat.com> wrote: > On Sat, Jan 08, 2022 at 06:52:14PM -0800, Zhihong Yu wrote: > > On Sat, Jan 8, 2022 at 5:52 PM Noah Misch <n...@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.gb3426...@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. >
Hi, Updated patch is attached. > > > 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. >
0003-memcpy-null.patch
Description: Binary data
