Thread: Avoid incorrect allocation in buildIndexArray
Looking at a pg_dump patch I realized that when we call buildIndexArray without having found objects to index, we still call pg_malloc with zero which in turn mallocs 1 byte. The byte in question is of course negligable, but it does seem cleaner to return early with NULL instead of returning an empty allocation which doesn't actually contain an index. Any reason not to bail early as per the attached? cheers ./daniel
Attachment
On Fri, Sep 11, 2020 at 1:39 PM Daniel Gustafsson <daniel@yesql.se> wrote: > > Looking at a pg_dump patch I realized that when we call buildIndexArray without > having found objects to index, we still call pg_malloc with zero which in turn > mallocs 1 byte. The byte in question is of course negligable, but it does seem > cleaner to return early with NULL instead of returning an empty allocation > which doesn't actually contain an index. > > Any reason not to bail early as per the attached? +1
On Fri, Sep 11, 2020 at 1:39 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>
> Looking at a pg_dump patch I realized that when we call buildIndexArray without
> having found objects to index, we still call pg_malloc with zero which in turn
> mallocs 1 byte. The byte in question is of course negligable, but it does seem
> cleaner to return early with NULL instead of returning an empty allocation
> which doesn't actually contain an index.
>
> Any reason not to bail early as per the attached?
+1
Since, it is protecting from invalid entries.
numObjs is int, the better it would be then.
numObjs is int, the better it would be then.
+ if (numObjs <= 0)
+ return NULL;
+
+ return NULL;
+
regards,
Ranier Vilela
On Fri, Sep 11, 2020 at 01:49:26PM +0200, Julien Rouhaud wrote: > On Fri, Sep 11, 2020 at 1:39 PM Daniel Gustafsson <daniel@yesql.se> wrote: >> Any reason not to bail early as per the attached? > > +1 Makes sense to me. This has also the advantage to cause a crash if there is an attempt to refer to those empty arrays in case of future refactoring, which is rather defensive. By looking at findObjectByOid(), I can also see that we check for a negative number, so I concur with Ranier's comment to check after that on top of 0. If there are no objections, I'll apply that on HEAD. -- Michael
Attachment
Le sam. 12 sept. 2020 à 11:14, Michael Paquier <michael@paquier.xyz> a écrit :
On Fri, Sep 11, 2020 at 01:49:26PM +0200, Julien Rouhaud wrote:
> On Fri, Sep 11, 2020 at 1:39 PM Daniel Gustafsson <daniel@yesql.se> wrote:
>> Any reason not to bail early as per the attached?
>
> +1
Makes sense to me. This has also the advantage to cause a crash if
there is an attempt to refer to those empty arrays in case of future
refactoring, which is rather defensive. By looking at
findObjectByOid(), I can also see that we check for a negative number,
yes, I also checked that current code is already checking for that.
so I concur with Ranier's comment to check after that on top of 0.
If there are no objections, I'll apply that on HEAD.
agreed.
On Sat, Sep 12, 2020 at 12:40:49PM +0200, Julien Rouhaud wrote: > agreed. Ok, done as ac673a1 then. -- Michael
Attachment
Em dom., 13 de set. de 2020 às 22:46, Michael Paquier <michael@paquier.xyz> escreveu:
On Sat, Sep 12, 2020 at 12:40:49PM +0200, Julien Rouhaud wrote:
> agreed.
Ok, done as ac673a1 then.
Thanks Michael.
regards,
Ranier Vilela