[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(dot)korotkov(at)postgrespro(dot)ru

> On 23 Apr 2025, at 13:32, m(dot)korotkov(at)postgrespro(dot)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(dot)korotkov(at)postgrespro(dot)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

> On 24 Apr 2025, at 00:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)yandex-team(dot)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(at)sss(dot)pgh(dot)pa(dot)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.