Обсуждение: pg_dump: fix memory leak

Hi all,
I have found a potential memory leak in src/bin/pg_dump/dumputils.c in 
the function generate_restrict_key().
Memory is allocated to the ret pointer if pg_strong_random returns 
false, and this leads to a memory leak.
I have replaced the allocation to avoid this leak.
---
Best regards, Korotkov Maksim
PostgresPro
m.korotkov@postgrespro.ru

> On 28 Aug 2025, at 16:14, m.korotkov@postgrespro.ru wrote:
>
> Hi all,
> I have found a potential memory leak in src/bin/pg_dump/dumputils.c in the function generate_restrict_key().
> Memory is allocated to the ret pointer if pg_strong_random returns false, and this leads to a memory leak.
> I have replaced the allocation to avoid this leak.

This is not actually a leak since the application will terminate immediately if
a restrict key cannot be generated.  If you inspect the callsites you will see
this pattern:

    if (!restrict_key)
        restrict_key = generate_restrict_key();
    if (!restrict_key)
        pg_fatal("could not generate restrict key");

--
Daniel Gustafsson

Daniel Gustafsson wrote 2025-08-29 10:13:
> This is not actually a leak since the application will terminate 
> immediately if
> a restrict key cannot be generated.
I agree that the current usage of the function does not present a 
problem, but there is no certainty that this situation will remain 
unchanged. In my view, it would be prudent to explicitly release the 
memory.
---
Best regards, Korotkov Maksim
PostgresPro
m.korotkov@postgrespro.ru

> On 29 Aug 2025, at 09:36, m.korotkov@postgrespro.ru wrote:
>
> Daniel Gustafsson wrote 2025-08-29 10:13:
>> This is not actually a leak since the application will terminate immediately if
>> a restrict key cannot be generated.
> I agree that the current usage of the function does not present a problem, but there is no certainty that this
situationwill remain unchanged. 

I certainly hope it won't change, ignoring a failure from pg_strong_random() is
a seriously bad idea.  If the function is rewritten to change its errorhandling
then allocation might be changed, right now there is no leak and no bug.

--
Daniel Gustafsson

On 2025-Aug-29, Daniel Gustafsson wrote:

> > On 28 Aug 2025, at 16:14, m.korotkov@postgrespro.ru wrote:
> > 
> > I have found a potential memory leak in src/bin/pg_dump/dumputils.c
> > in the function generate_restrict_key().  Memory is allocated to the
> > ret pointer if pg_strong_random returns false, and this leads to a
> > memory leak.  I have replaced the allocation to avoid this leak.
> 
> This is not actually a leak since the application will terminate
> immediately if a restrict key cannot be generated.  If you inspect the
> callsites you will see this pattern:
> 
>     if (!restrict_key)
>         restrict_key = generate_restrict_key();
>     if (!restrict_key)
>         pg_fatal("could not generate restrict key");

Hmm, this begs the question -- why do we make generate_restrict_key()
return NULL in that case?  Maybe we should redefine its contract to be
that it either returns a valid key, or it calls pg_fatal().  Then
callers don't have to worry about it.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

> On 29 Aug 2025, at 11:52, Álvaro Herrera <alvherre@kurilemu.de> wrote:

> Hmm, this begs the question -- why do we make generate_restrict_key()
> return NULL in that case?  Maybe we should redefine its contract to be
> that it either returns a valid key, or it calls pg_fatal().  Then
> callers don't have to worry about it.

That is certainly an option.

--
Daniel Gustafsson