* [PATCH] elf/cache.c: Fix resource leaks identified by static analyzers
@ 2021-05-10 15:58 Siddhesh Poyarekar
2021-05-17 18:29 ` Adhemerval Zanella
0 siblings, 1 reply; 2+ messages in thread
From: Siddhesh Poyarekar @ 2021-05-10 15:58 UTC (permalink / raw)
To: libc-alpha
A coverity run identified a number of resource leaks in cache.c.
There are a couple of simple memory leaks where a local allocation is
not freed before function return. Then there is a mmap leak and a
file descriptor leak where a map is not unmapped in the error case and
a file descriptor remains open respectively.
---
elf/cache.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/elf/cache.c b/elf/cache.c
index c01d302072..8a3404923c 100644
--- a/elf/cache.c
+++ b/elf/cache.c
@@ -547,6 +547,7 @@ write_extensions (int fd, uint32_t str_offset,
|| write (fd, generator, strlen (generator)) != strlen (generator))
error (EXIT_FAILURE, errno, _("Writing of cache extension data failed"));
+ free (hwcaps_array);
free (ext);
}
@@ -778,6 +779,7 @@ save_cache (const char *cache_name)
free (file_entries_new);
free (file_entries);
free (strings_finalized.strings);
+ free (temp_name);
while (entries)
{
@@ -1034,6 +1036,9 @@ load_aux_cache (const char *aux_cache_name)
+ aux_cache->nlibs * sizeof (struct aux_cache_file_entry)
+ aux_cache->len_strings))
{
+ if (aux_cache != MAP_FAILED)
+ munmap (aux_cache, aux_cache_size);
+
close (fd);
init_aux_cache ();
return;
@@ -1143,10 +1148,13 @@ save_aux_cache (const char *aux_cache_name)
if (fd < 0)
goto out_fail;
- if (write (fd, file_entries, file_entries_size + total_strlen)
- != (ssize_t) (file_entries_size + total_strlen)
- || fdatasync (fd) != 0
- || close (fd) != 0)
+ bool fail = ((write (fd, file_entries, file_entries_size + total_strlen)
+ != (ssize_t) (file_entries_size + total_strlen))
+ || fdatasync (fd) != 0);
+
+ fail |= close (fd) != 0;
+
+ if (fail)
{
unlink (temp_name);
goto out_fail;
--
2.31.1
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] elf/cache.c: Fix resource leaks identified by static analyzers
2021-05-10 15:58 [PATCH] elf/cache.c: Fix resource leaks identified by static analyzers Siddhesh Poyarekar
@ 2021-05-17 18:29 ` Adhemerval Zanella
0 siblings, 0 replies; 2+ messages in thread
From: Adhemerval Zanella @ 2021-05-17 18:29 UTC (permalink / raw)
To: Siddhesh Poyarekar, libc-alpha
On 10/05/2021 12:58, Siddhesh Poyarekar via Libc-alpha wrote:
> A coverity run identified a number of resource leaks in cache.c.
> There are a couple of simple memory leaks where a local allocation is
> not freed before function return. Then there is a mmap leak and a
> file descriptor leak where a map is not unmapped in the error case and
> a file descriptor remains open respectively.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> elf/cache.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/elf/cache.c b/elf/cache.c
> index c01d302072..8a3404923c 100644
> --- a/elf/cache.c
> +++ b/elf/cache.c
> @@ -547,6 +547,7 @@ write_extensions (int fd, uint32_t str_offset,
> || write (fd, generator, strlen (generator)) != strlen (generator))
> error (EXIT_FAILURE, errno, _("Writing of cache extension data failed"));
>
> + free (hwcaps_array);
> free (ext);
> }
>
Ok.
> @@ -778,6 +779,7 @@ save_cache (const char *cache_name)
> free (file_entries_new);
> free (file_entries);
> free (strings_finalized.strings);
> + free (temp_name);
>
> while (entries)
> {
Ok.
> @@ -1034,6 +1036,9 @@ load_aux_cache (const char *aux_cache_name)
> + aux_cache->nlibs * sizeof (struct aux_cache_file_entry)
> + aux_cache->len_strings))
> {
> + if (aux_cache != MAP_FAILED)
> + munmap (aux_cache, aux_cache_size);
> +
> close (fd);
> init_aux_cache ();
> return;
Ok.
> @@ -1143,10 +1148,13 @@ save_aux_cache (const char *aux_cache_name)
> if (fd < 0)
> goto out_fail;
>
> - if (write (fd, file_entries, file_entries_size + total_strlen)
> - != (ssize_t) (file_entries_size + total_strlen)
> - || fdatasync (fd) != 0
> - || close (fd) != 0)
> + bool fail = ((write (fd, file_entries, file_entries_size + total_strlen)
> + != (ssize_t) (file_entries_size + total_strlen))
> + || fdatasync (fd) != 0);
> +
> + fail |= close (fd) != 0;
> +
> + if (fail)
> {
> unlink (temp_name);
> goto out_fail;
>
I think there is no need of the extra parenthesis, besides that it looks
ok.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-05-17 18:29 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-10 15:58 [PATCH] elf/cache.c: Fix resource leaks identified by static analyzers Siddhesh Poyarekar
2021-05-17 18:29 ` Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).