[PATCH] dynahash: add memory allocation failure check
Hi all,
I found a case of potential NULL pointer dereference.
In src/backend/utils/hash/dynahash.c in function HTAB *hash_create() the
result of the DynaHashAlloc() is used unsafely.
The function DynaHashAlloc() calls MemoryContextAllocExtended() with
MCXT_ALLOC_NO_OOM and can return a NULL pointer.
Added the pointer check for avoiding a potential problem.
---
Best regards, Korotkov Maksim
PostgresPro
m.korotkov@postgrespro.ru
Attachments:
0001-dynahash-add-memory-allocation-failure-check.patchtext/x-diff; name=0001-dynahash-add-memory-allocation-failure-check.patchDownload
From 75916a6855bb9e96c7b34b76c0380edc157c150c Mon Sep 17 00:00:00 2001
From: Maksim Korotkov <m.korotkov@postgrespro.ru>
Date: Tue, 22 Apr 2025 12:20:58 +0300
Subject: [PATCH] dynahash: add memory allocation failure check
The function DynaHashAlloc() calls MemoryContextAllocExtended() with MCXT_ALLOC_NO_OOM
and can return a NULL pointer.
Fixes: e3860ffa4dd ("Initial pgindent run with pg_bsd_indent version 2.0.")
Signed-off-by: Maksim Korotkov <m.korotkov@postgrespro.ru>
---
src/backend/utils/hash/dynahash.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 3f25929f2d8..0cfee50dc21 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -391,6 +391,12 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
/* Initialize the hash header, plus a copy of the table name */
hashp = (HTAB *) DynaHashAlloc(sizeof(HTAB) + strlen(tabname) + 1);
+ if (unlikely(hashp == NULL))
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory")));
+ }
MemSet(hashp, 0, sizeof(HTAB));
hashp->tabname = (char *) (hashp + 1);
--
2.34.1
On 23 Apr 2025, at 13:32, m.korotkov@postgrespro.ru wrote:
I found a case of potential NULL pointer dereference.
In src/backend/utils/hash/dynahash.c in function HTAB *hash_create() the result of the DynaHashAlloc() is used unsafely.
The function DynaHashAlloc() calls MemoryContextAllocExtended() with MCXT_ALLOC_NO_OOM and can return a NULL pointer.
Added the pointer check for avoiding a potential problem.
Yeah, good catch.
And all HTAB->alloc() (which relies on DynaHashAlloc) callers seem to check for NULL result.
It seems there are a lot of cases of MCXT_ALLOC_NO_OOM, perhaps should we check them all?
Best regards, Andrey Borodin.
Andrey Borodin wrote 2025-04-23 12:46:
It seems there are a lot of cases of MCXT_ALLOC_NO_OOM, perhaps should
we check them all?
Yep, I think we should.
I found this issue with the Svace static analyzer, and it might have
missed other issues.
Perhaps a more comprehensive investigation is needed here.
---
Best regards, Maksim Korotkov
Hi Maksim,
I found a case of potential NULL pointer dereference.
In src/backend/utils/hash/dynahash.c in function HTAB *hash_create() the
result of the DynaHashAlloc() is used unsafely.
The function DynaHashAlloc() calls MemoryContextAllocExtended() with
MCXT_ALLOC_NO_OOM and can return a NULL pointer.
Added the pointer check for avoiding a potential problem.
Thanks for the patch. It looks correct to me.
I didn't check if it needs to be back-ported and if it does - to how
many branches.
--
Best regards,
Aleksander Alekseev
m.korotkov@postgrespro.ru writes:
I found a case of potential NULL pointer dereference.
In src/backend/utils/hash/dynahash.c in function HTAB *hash_create() the
result of the DynaHashAlloc() is used unsafely.
The function DynaHashAlloc() calls MemoryContextAllocExtended() with
MCXT_ALLOC_NO_OOM and can return a NULL pointer.
Ugh, that's a stupid bug. Evidently my fault, too (9c911ec06).
Added the pointer check for avoiding a potential problem.
This doesn't seem like a nice way to fix it. The code right there is
assuming palloc semantics, which was okay before 9c911ec06, but is so
no longer. I think the right thing is to put it back to palloc
semantics, which means not using DynaHashAlloc there, as attached.
regards, tom lane
Attachments:
v2-fix-missing-palloc-check-in-dynahash.patchtext/x-diff; charset=us-ascii; name=v2-fix-missing-palloc-check-in-dynahash.patchDownload
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 3f25929f2d8..1ad155d446e 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -390,7 +390,8 @@ hash_create(const char *tabname, long nelem, const HASHCTL *info, int flags)
}
/* Initialize the hash header, plus a copy of the table name */
- hashp = (HTAB *) DynaHashAlloc(sizeof(HTAB) + strlen(tabname) + 1);
+ hashp = (HTAB *) MemoryContextAlloc(CurrentDynaHashCxt,
+ sizeof(HTAB) + strlen(tabname) + 1);
MemSet(hashp, 0, sizeof(HTAB));
hashp->tabname = (char *) (hashp + 1);
On 24 Apr 2025, at 00:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
- hashp = (HTAB *) DynaHashAlloc(sizeof(HTAB) + strlen(tabname) + 1); + hashp = (HTAB *) MemoryContextAlloc(CurrentDynaHashCxt, + sizeof(HTAB) + strlen(tabname) + 1);
This seems correct to me.
While fixing this maybe use MemoryContextAllocZero() instead of subsequent MemSet()?
But this might unroll loop of unnecessary beautifications like DynaHashAlloc() calling Assert(MemoryContextIsValid(CurrentDynaHashCxt)) just before MemoryContextAllocExtended() will repeat same exercise.
Best regards, Andrey Borodin.
Andrey Borodin <x4mmm@yandex-team.ru> writes:
While fixing this maybe use MemoryContextAllocZero() instead of subsequent MemSet()?
I thought about that but intentionally left it as-is, because that
would force zeroing of the space reserved for the hashtable name too.
That's unnecessary, and since it'd often be odd-sized it might result
in a less efficient fill loop.
regards, tom lane
On 24 Apr 2025, at 19:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I thought about that but intentionally left it as-is, because that
would force zeroing of the space reserved for the hashtable name too.
That's unnecessary, and since it'd often be odd-sized it might result
in a less efficient fill loop.
Well, that's just few hundred bytes at most. But I agree that makes sense.
Best regards, Andrey Borodin.