Fix resource leak (src/backend/libpq/be-secure-common.c)

Started by Ranier Vilelaabout 2 years ago9 messageshackers
Jump to latest
#1Ranier Vilela
ranier.vf@gmail.com

Hi,

Per Coverity.

Coverity reported a resource leak at the function
run_ssl_passphrase_command.
7. alloc_fn: Storage is returned from allocation function
wait_result_to_str.["show details"]

8. noescape: Assuming resource wait_result_to_str(pclose_rc) is not freed
or pointed-to as ellipsis argument to errdetail_internal.

CID 1533043: (#1 of 1): Resource leak (RESOURCE_LEAK)
9. leaked_storage: Failing to save or free storage allocated by
wait_result_to_str(pclose_rc) leaks it.

I think that Coverity is right.

Fix by freeing the pointer, like pclose_check (src/common/exec.c) similar
case.

Patch attached.

best regards,

Attachments:

fix-resource-leak-be-secure-common.patchapplication/octet-stream; name=fix-resource-leak-be-secure-common.patchDownload+5-1
#2Daniel Gustafsson
daniel@yesql.se
In reply to: Ranier Vilela (#1)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

On 2 Apr 2024, at 20:13, Ranier Vilela <ranier.vf@gmail.com> wrote:

Fix by freeing the pointer, like pclose_check (src/common/exec.c) similar case.

Off the cuff, seems reasonable when loglevel is LOG.

--
Daniel Gustafsson

#3Ranier Vilela
ranier.vf@gmail.com
In reply to: Daniel Gustafsson (#2)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

Em ter., 2 de abr. de 2024 às 15:31, Daniel Gustafsson <daniel@yesql.se>
escreveu:

On 2 Apr 2024, at 20:13, Ranier Vilela <ranier.vf@gmail.com> wrote:

Fix by freeing the pointer, like pclose_check (src/common/exec.c)

similar case.

Off the cuff, seems reasonable when loglevel is LOG.

Per Coverity.

Another case of resource leak, when loglevel is LOG.
In the function shell_archive_file (src/backend/archive/shell_archive.c)
The pointer *xlogarchcmd* is not freed.

best regards,
Ranier Vilela

Attachments:

fix-resource-leak-shell_archive.patchapplication/octet-stream; name=fix-resource-leak-shell_archive.patchDownload+2-0
#4Daniel Gustafsson
daniel@yesql.se
In reply to: Ranier Vilela (#3)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

On 10 Apr 2024, at 20:31, Ranier Vilela <ranier.vf@gmail.com> wrote:

Em ter., 2 de abr. de 2024 às 15:31, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> escreveu:

On 2 Apr 2024, at 20:13, Ranier Vilela <ranier.vf@gmail.com <mailto:ranier.vf@gmail.com>> wrote:

Fix by freeing the pointer, like pclose_check (src/common/exec.c) similar case.

Off the cuff, seems reasonable when loglevel is LOG.

Per Coverity.

Another case of resource leak, when loglevel is LOG.
In the function shell_archive_file (src/backend/archive/shell_archive.c)
The pointer *xlogarchcmd* is not freed.

Thanks, I'll have a look. I've left this for post-freeze on purpose to not
cause unnecessary rebasing. Will take a look over the next few days unless
beaten to it.

--
Daniel Gustafsson

#5Ranier Vilela
ranier.vf@gmail.com
In reply to: Daniel Gustafsson (#4)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

Em qua., 10 de abr. de 2024 às 15:33, Daniel Gustafsson <daniel@yesql.se>
escreveu:

On 10 Apr 2024, at 20:31, Ranier Vilela <ranier.vf@gmail.com> wrote:

Em ter., 2 de abr. de 2024 às 15:31, Daniel Gustafsson <daniel@yesql.se>
escreveu:

On 2 Apr 2024, at 20:13, Ranier Vilela <ranier.vf@gmail.com> wrote:

Fix by freeing the pointer, like pclose_check (src/common/exec.c)

similar case.

Off the cuff, seems reasonable when loglevel is LOG.

Per Coverity.

Another case of resource leak, when loglevel is LOG.
In the function shell_archive_file (src/backend/archive/shell_archive.c)
The pointer *xlogarchcmd* is not freed.

Thanks, I'll have a look. I've left this for post-freeze on purpose to not
cause unnecessary rebasing. Will take a look over the next few days unless
beaten to it.

Any chance we'll have these fixes in v17?

best regards,
Ranier Vilela

#6Daniel Gustafsson
daniel@yesql.se
In reply to: Ranier Vilela (#5)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

On 13 May 2024, at 20:05, Ranier Vilela <ranier.vf@gmail.com> wrote:
Em qua., 10 de abr. de 2024 às 15:33, Daniel Gustafsson <daniel@yesql.se> escreveu:

Thanks, I'll have a look. I've left this for post-freeze on purpose to not
cause unnecessary rebasing. Will take a look over the next few days unless
beaten to it.

Any chance we'll have these fixes in v17?

Nice timing, I was actually rebasing them today to get them committed.

--
Daniel Gustafsson

#7Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#6)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

On Mon, May 13, 2024 at 08:06:57PM +0200, Daniel Gustafsson wrote:

Any chance we'll have these fixes in v17?

Nice timing, I was actually rebasing them today to get them committed.

Looks sensible seen from here, as these paths could use a LOG or rely
on a memory context permanent to the backend causing junk to be
accumulated. It's not that much, still that would accumulate.
--
Michael

#8Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#7)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

On 14 May 2024, at 08:03, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, May 13, 2024 at 08:06:57PM +0200, Daniel Gustafsson wrote:

Any chance we'll have these fixes in v17?

Nice timing, I was actually rebasing them today to get them committed.

Looks sensible seen from here, as these paths could use a LOG or rely
on a memory context permanent to the backend causing junk to be
accumulated. It's not that much, still that would accumulate.

Pushed.

--
Daniel Gustafsson

#9Ranier Vilela
ranier.vf@gmail.com
In reply to: Daniel Gustafsson (#8)
Re: Fix resource leak (src/backend/libpq/be-secure-common.c)

Em ter., 14 de mai. de 2024 às 07:21, Daniel Gustafsson <daniel@yesql.se>
escreveu:

On 14 May 2024, at 08:03, Michael Paquier <michael@paquier.xyz> wrote:

On Mon, May 13, 2024 at 08:06:57PM +0200, Daniel Gustafsson wrote:

Any chance we'll have these fixes in v17?

Nice timing, I was actually rebasing them today to get them committed.

Looks sensible seen from here, as these paths could use a LOG or rely
on a memory context permanent to the backend causing junk to be
accumulated. It's not that much, still that would accumulate.

Pushed.

Thanks Daniel.

best regards,
Ranier Vilela