elf/cache.c: Fix resource leaks identified by static analyzers

Message ID 20210510155827.131051-1-siddhesh@sourceware.org
State New
Headers show
Series
  • elf/cache.c: Fix resource leaks identified by static analyzers
Related show

Commit Message

Stafford Horne via Libc-alpha May 10, 2021, 3:58 p.m.
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(-)

-- 
2.31.1

Comments

Stafford Horne via Libc-alpha May 17, 2021, 6:29 p.m. | #1
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.

Patch

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;