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
Attachments:
buildindexarrayalloc.patchapplication/octet-stream; name=buildindexarrayalloc.patch; x-unix-mode=0644Download
From be8fb5d0892466114c995a28686db487b2026ebf Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Fri, 11 Sep 2020 13:34:56 +0200
Subject: [PATCH] Exit early when no objects are to be indexed
Check for objects passed to the function before allocating space for
the index, as the returned allocation will otherwise not contain an
index and thus be potentially misleading.
---
src/bin/pg_dump/common.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/bin/pg_dump/common.c b/src/bin/pg_dump/common.c
index 08239dde4f..fe477ccc99 100644
--- a/src/bin/pg_dump/common.c
+++ b/src/bin/pg_dump/common.c
@@ -719,6 +719,9 @@ buildIndexArray(void *objArray, int numObjs, Size objSize)
DumpableObject **ptrs;
int i;
+ if (numObjs == 0)
+ return NULL;
+
ptrs = (DumpableObject **) pg_malloc(numObjs * sizeof(DumpableObject *));
for (i = 0; i < numObjs; i++)
ptrs[i] = (DumpableObject *) ((char *) objArray + i * objSize);
--
2.21.1 (Apple Git-122.3)
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.
+ if (numObjs <= 0)
+ return NULL;
+regards,
Ranier Vilela
Import Notes
Resolved by subject fallback
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
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.
Show quoted text
On Sat, Sep 12, 2020 at 12:40:49PM +0200, Julien Rouhaud wrote:
agreed.
Ok, done as ac673a1 then.
--
Michael