Fix resource leak (src/backend/libpq/be-secure-common.c)
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
diff --git a/src/backend/libpq/be-secure-common.c b/src/backend/libpq/be-secure-common.c
index 0582606192..a67a5e8619 100644
--- a/src/backend/libpq/be-secure-common.c
+++ b/src/backend/libpq/be-secure-common.c
@@ -85,12 +85,16 @@ run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf,
}
else if (pclose_rc != 0)
{
+ const char * reason;
+
explicit_bzero(buf, size);
+ reason = wait_result_to_str(pclose_rc);
ereport(loglevel,
(errcode_for_file_access(),
errmsg("command \"%s\" failed",
command),
- errdetail_internal("%s", wait_result_to_str(pclose_rc))));
+ errdetail_internal("%s", reason)));
+ pfree(reason);
goto error;
}
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
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
diff --git a/src/backend/archive/shell_archive.c b/src/backend/archive/shell_archive.c
index 0925348bfe..506c5a30ad 100644
--- a/src/backend/archive/shell_archive.c
+++ b/src/backend/archive/shell_archive.c
@@ -125,9 +125,11 @@ shell_archive_file(ArchiveModuleState *state, const char *file,
errdetail("The failed archive command was: %s",
xlogarchcmd)));
}
+ pfree(xlogarchcmd);
return false;
}
+ pfree(xlogarchcmd);
elog(DEBUG1, "archived write-ahead log file \"%s\"", file);
return true;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
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
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
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
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
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