Обсуждение: remove redundant memset() call

On Thu, Oct 13, 2022 at 10:55:08AM -0700, Zhihong Yu wrote:
> Hi,
> I was looking at combo_init in contrib/pgcrypto/px.c .
> 
> There is a memset() call following palloc0() - the call is redundant.
> 
> Please see the patch for the proposed change.
> 
> Thanks

> diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
> index 3b098c6151..d35ccca777 100644
> --- a/contrib/pgcrypto/px.c
> +++ b/contrib/pgcrypto/px.c
> @@ -203,7 +203,6 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned klen,
>      if (klen > ks)
>          klen = ks;
>      keybuf = palloc0(ks);
> -    memset(keybuf, 0, ks);
>      memcpy(keybuf, key, klen);
>  
>      err = px_cipher_init(c, keybuf, klen, ivbuf);

Uh, the memset() is ks length but the memcpy() is klen, and the above
test allows ks to be larger than klen.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

On Thu, Oct 13, 2022 at 12:12:35PM -0700, Zhihong Yu wrote:
> On Thu, Oct 13, 2022 at 12:10 PM Bruce Momjian <bruce@momjian.us> wrote:
> 
>     On Thu, Oct 13, 2022 at 10:55:08AM -0700, Zhihong Yu wrote:
>     > Hi,
>     > I was looking at combo_init in contrib/pgcrypto/px.c .
>     >
>     > There is a memset() call following palloc0() - the call is redundant.
>     >
>     > Please see the patch for the proposed change.
>     >
>     > Thanks
> 
>     > diff --git a/contrib/pgcrypto/px.c b/contrib/pgcrypto/px.c
>     > index 3b098c6151..d35ccca777 100644
>     > --- a/contrib/pgcrypto/px.c
>     > +++ b/contrib/pgcrypto/px.c
>     > @@ -203,7 +203,6 @@ combo_init(PX_Combo *cx, const uint8 *key, unsigned
>     klen,
>     >       if (klen > ks)
>     >               klen = ks;
>     >       keybuf = palloc0(ks);
>     > -     memset(keybuf, 0, ks);
>     >       memcpy(keybuf, key, klen);
>     > 
>     >       err = px_cipher_init(c, keybuf, klen, ivbuf);
> 
>     Uh, the memset() is ks length but the memcpy() is klen, and the above
>     test allows ks to be larger than klen.
> 
>     --
>       Bruce Momjian  <bruce@momjian.us>        https://momjian.us
>       EDB                                      https://enterprisedb.com
> 
>       Indecision is a decision.  Inaction is an action.  Mark Batterson
> 
> 
> Hi,
> the memory has been zero'ed out by palloc0().
> 
> memcpy is not relevant w.r.t. resetting memory.

Ah, good point.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

On Thu, Oct 13, 2022 at 03:15:13PM -0400, Bruce Momjian wrote:
> On Thu, Oct 13, 2022 at 12:12:35PM -0700, Zhihong Yu wrote:
>> the memory has been zero'ed out by palloc0().
>> 
>> memcpy is not relevant w.r.t. resetting memory.
> 
> Ah, good point.

Yeah, it looks like this was missed in ca7f8e2.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

> On 13 Oct 2022, at 21:18, Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Thu, Oct 13, 2022 at 03:15:13PM -0400, Bruce Momjian wrote:
>> On Thu, Oct 13, 2022 at 12:12:35PM -0700, Zhihong Yu wrote:
>>> the memory has been zero'ed out by palloc0().
>>>
>>> memcpy is not relevant w.r.t. resetting memory.
>>
>> Ah, good point.
>
> Yeah, it looks like this was missed in ca7f8e2.

Agreed, it looks like I missed that one, I can't see any reason to keep it.  Do
you want me to take care of it Bruce, and clean up after myself, or are you
already on it?

--
Daniel Gustafsson        https://vmware.com/

On Thu, Oct 13, 2022 at 09:40:42PM +0200, Daniel Gustafsson wrote:
> > On 13 Oct 2022, at 21:18, Nathan Bossart <nathandbossart@gmail.com> wrote:
> > 
> > On Thu, Oct 13, 2022 at 03:15:13PM -0400, Bruce Momjian wrote:
> >> On Thu, Oct 13, 2022 at 12:12:35PM -0700, Zhihong Yu wrote:
> >>> the memory has been zero'ed out by palloc0().
> >>> 
> >>> memcpy is not relevant w.r.t. resetting memory.
> >> 
> >> Ah, good point.
> > 
> > Yeah, it looks like this was missed in ca7f8e2.
> 
> Agreed, it looks like I missed that one, I can't see any reason to keep it.  Do
> you want me to take care of it Bruce, and clean up after myself, or are you
> already on it?

You can do it, thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Indecision is a decision.  Inaction is an action.  Mark Batterson

> On 13 Oct 2022, at 21:59, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Thu, Oct 13, 2022 at 09:40:42PM +0200, Daniel Gustafsson wrote:
>>> On 13 Oct 2022, at 21:18, Nathan Bossart <nathandbossart@gmail.com> wrote:
>>>
>>> On Thu, Oct 13, 2022 at 03:15:13PM -0400, Bruce Momjian wrote:
>>>> On Thu, Oct 13, 2022 at 12:12:35PM -0700, Zhihong Yu wrote:
>>>>> the memory has been zero'ed out by palloc0().
>>>>>
>>>>> memcpy is not relevant w.r.t. resetting memory.
>>>>
>>>> Ah, good point.
>>>
>>> Yeah, it looks like this was missed in ca7f8e2.
>>
>> Agreed, it looks like I missed that one, I can't see any reason to keep it. Do
>> you want me to take care of it Bruce, and clean up after myself, or are you
>> already on it?
>
> You can do it, thanks.

Done now.

--
Daniel Gustafsson        https://vmware.com/