Memory leak fix in rmtree.c
Hello hackers,
Just like some of my colleagues I've been using Svace*
and I think I've found a bug in src/common/rmtree.c .
In 64th line function returns false in case it couldn't open a directory,
but the memory, that have been allocated for char** dirnames is
not freed.
The patch that has a fix of this is attached and is based on the latest
master code.
* - https://svace.pages.ispras.ru/svace-website/en/
Kind regards,
Ian Ilyasov.
Attachments:
v001-Fix_memory_leak_in_rmtree.patchtext/x-patch; name=v001-Fix_memory_leak_in_rmtree.patchDownload
Subject: [PATCH] Fixed memory leak in case we couldn't open a directory in rmtree.c.
---
Index: src/common/rmtree.c
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/common/rmtree.c b/src/common/rmtree.c
--- a/src/common/rmtree.c (revision 48a6bf5c4ea8e04cc9bb33a8120a21743da515ed)
+++ b/src/common/rmtree.c (date 1707211004528)
@@ -61,6 +61,7 @@
if (dir == NULL)
{
pg_log_warning("could not open directory \"%s\": %m", path);
+ pfree(dirnames);
return false;
}
On 6 Feb 2024, at 10:34, Ильясов Ян <ianilyasov@outlook.com> wrote:
Just like some of my colleagues I've been using Svace*
and I think I've found a bug in src/common/rmtree.c .In 64th line function returns false in case it couldn't open a directory,
but the memory, that have been allocated for char** dirnames is
not freed.
dirnames isn't allocated at this point, it's palloc'd after this return
statement on line 67.
--
Daniel Gustafsson
dirnames isn't allocated at this point, it's palloc'd after this return
statement on line 67.--
Daniel Gustafsson
I am sorry, I pointed on the wrong branch. I see that in master
it is really in line 67th , and the allocation goes well. But in
REL_16_STABLE the allocation is in line 58th and my patch is for this branch only.
Kind regards,
Ian Ilyasov.
On 6 Feb 2024, at 11:21, Ильясов Ян <ianilyasov@outlook.com> wrote:
dirnames isn't allocated at this point, it's palloc'd after this return
statement on line 67.I am sorry, I pointed on the wrong branch. I see that in master
it is really in line 67th , and the allocation goes well. But in
REL_16_STABLE the allocation is in line 58th and my patch is for this branch only.
Ok, that makes more sense. That was changed in commit f1e9f6bbfa53, and in the
discussion for that patch it was deemed that no callers actually suffered from
the leak as they were exiting (or erroring out) on error, so it was never back-
patched.
/messages/by-id/CAEudQAoN3-2ZKBALThnEk_q2hu8En5A0WG9O+5siJTQKVZzoWQ@mail.gmail.com
That still holds true today, so I don't see a big incentive to spend energy on
backpatching that since it mainly serves to silence analyzers. Grepping for
the pattern of allocating in the declaration doesn't reveal any other codepaths
where the allocation leaks like this.
--
Daniel Gustafsson
I agree with your argument.
Thank you for your time.
Kind regards,
Ian Ilyasov
Juniour Software Developer at Postgres Professional