Thread: [MASSMAIL] Fix some resources leaks (src/bin/pg_basebackup/pg_createsubscriber.c)
[MASSMAIL] Fix some resources leaks (src/bin/pg_basebackup/pg_createsubscriber.c)
From
Ranier Vilela
Date:
Hi,
Per Coverity.
Coverity has some reports in the new code
pg_createsubscriber.c
I think that Coverity is right.
CID 1542682: (#1 of 1): Resource leak (RESOURCE_LEAK)
leaked_storage: Variable buf going out of scope leaks the storage it points to.
2.
CID 1542704: (#1 of 1): Resource leak (RESOURCE_LEAK)
leaked_storage: Variable conn going out of scope leaks the storage it points to.3.
Patch attached.
CID 1542691: (#1 of 1): Resource leak (RESOURCE_LEAK)
leaked_storage: Variable str going out of scope leaks the storage it points to.best regards,
Ranier Vilela
Attachment
Re: Fix some resources leaks (src/bin/pg_basebackup/pg_createsubscriber.c)
From
"Euler Taveira"
Date:
On Wed, Mar 27, 2024, at 8:50 PM, Ranier Vilela wrote:
Coverity has some reports in the new codepg_createsubscriber.cI think that Coverity is right.
It depends on your "right" definition. If your program execution is ephemeral
and the leak is just before exiting, do you think it's worth it?
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.
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:
* 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.
Having said that, applying this patch is just a matter of style.
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 codepg_createsubscriber.cI think that Coverity is right.It depends on your "right" definition.
I do not think so.
If your program execution is ephemeraland 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
"Euler Taveira" <euler@eulerto.com> writes:
> 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. If your program execution is ephemeral
> and the leak is just before exiting, do you think it's worth it?
I agree with Ranier, actually. The functions in question don't
exit() themselves, but return control to a caller that might or
might not choose to exit. I don't think they should be assuming
that an exit will happen promptly, even if that's true today.
The community Coverity run found a few additional leaks of the same
kind in that file. I pushed a merged fix for all of them.
regards, tom lane
Em seg., 1 de abr. de 2024 às 14:52, Tom Lane <tgl@sss.pgh.pa.us> escreveu:
"Euler Taveira" <euler@eulerto.com> writes:
> 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. If your program execution is ephemeral
> and the leak is just before exiting, do you think it's worth it?
I agree with Ranier, actually. The functions in question don't
exit() themselves, but return control to a caller that might or
might not choose to exit. I don't think they should be assuming
that an exit will happen promptly, even if that's true today.
The community Coverity run found a few additional leaks of the same
kind in that file. I pushed a merged fix for all of them.
Thanks Tom, for the commit.
best regards,
Ranier Vilela