Thread: [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
Attachment
> 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
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.