Re: Fix some resources leaks (src/bin/pg_basebackup/pg_createsubscriber.c) - Mailing list pgsql-hackers

From Ranier Vilela
Subject Re: Fix some resources leaks (src/bin/pg_basebackup/pg_createsubscriber.c)
Date
Msg-id CAEudQAqQHGrhmY3+rgdqJLM-76sozLm__0_NSJetuQHsa+d41Q@mail.gmail.com
Whole thread Raw
In response to Re: Fix some resources leaks (src/bin/pg_basebackup/pg_createsubscriber.c)  ("Euler Taveira" <euler@eulerto.com>)
List pgsql-hackers
Em qua., 27 de mar. de 2024 às 23:08, Euler Taveira <euler@eulerto.com> escreveu:
On Wed, Mar 27, 2024, at 8:50 PM, Ranier Vilela wrote:
Coverity has some reports in the new code
pg_createsubscriber.c
I think that Coverity is right.

It depends on your "right" definition.
I do not think so.

If your program execution is ephemeral
and the leak is just before exiting, do you think it's worth it?
I think it's worth it.
Even an ephemeral execution can allocate resources, for example, and mainly, in Windows, 
and block these resources for other programs, and on a highly loaded server with very few free resources, 
releasing resources as quickly as possible makes a difference.


1.
CID 1542682: (#1 of 1): Resource leak (RESOURCE_LEAK)
leaked_storage: Variable buf going out of scope leaks the storage it points to.

It will exit in the next instruction.
Yes, by exit() call function.
 

2.
CID 1542704: (#1 of 1): Resource leak (RESOURCE_LEAK)
leaked_storage: Variable conn going out of scope leaks the storage it points to.

The connect_database function whose exit_on_error is false is used in 2 routines:
Calling connect_database with false, per example:
conn = connect_database(dbinfo[i].pubconninfo, false);

If the connection status != CONNECTION_OK, the function returns NULL,
but does not free connection struct, memory and data.

In the loop with thousands of "number of specified databases",
It would quickly end up in problems, especially on Windows.


* cleanup_objects_atexit: that's about to exit;
* drop_primary_replication_slot: that will execute a few routines before exiting.

3.
CID 1542691: (#1 of 1): Resource leak (RESOURCE_LEAK)
leaked_storage: Variable str going out of scope leaks the storage it points to.

It will exit in the next instruction.
Yes, by exit() call function. 

About exit() function:

"exit does not call the destructors of any stack based objects so if those objects have allocated any memory internally then yes that memory will be leaked. "

"Note that objects with automatic storage are not destroyed by calling exit (C++)."

I think that relying on the exit function for cleaning is extremely fragile, especially on Windows.


Having said that, applying this patch is just a matter of style.
IMO, a better and more correct style.

best regards,
Ranier Vilela

pgsql-hackers by date:

Previous
From: Thomas Munro
Date:
Subject: Re: Streaming I/O, vectored I/O (WIP)
Next
From: Amit Kapila
Date:
Subject: Re: Synchronizing slots from primary to standby