[PATCH 0/5] Static analysis fixes

[PATCH 0/5] Static analysis fixes

[Siddhesh Poyarekar via Libc-alpha]

These are some issues identified through static analysis.  The
gaiconf_init implementation is quite ugly and needs a refactor, but the
patch 5/5 is a minimal fix to ensure that a double-free is avoided.

Siddhesh Poyarekar (5):
  ldconfig: avoid leak on empty paths in config file
  gconv_parseconfdir: Fix memory leak
  iconv_charmap: Close output file when done
  copy_and_spawn_sgid: Avoid double calls to close()
  gaiconf_init: Avoid double-free in label and precedence lists

 elf/ldconfig.c                       | 6 +++++-
 iconv/gconv_parseconfdir.h           | 9 ++++-----
 iconv/iconv_charmap.c                | 2 ++
 support/support_capture_subprocess.c | 1 +
 sysdeps/posix/getaddrinfo.c          | 2 ++
 5 files changed, 14 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH 1/5] ldconfig: avoid leak on empty paths in config file

[Siddhesh Poyarekar via Libc-alpha]

---
 elf/ldconfig.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/elf/ldconfig.c b/elf/ldconfig.c
index 1037e8d0cf..b8893637f8 100644
--- a/elf/ldconfig.c
+++ b/elf/ldconfig.c
@@ -503,7 +503,11 @@ add_dir_1 (const char *line, const char *from_file, int from_line)
     entry->path[--i] = '\0';
 
   if (i == 0)
-    return;
+    {
+      free (entry->path);
+      free (entry);
+      return;
+    }
 
   char *path = entry->path;
   if (opt_chroot != NULL)
-- 
2.31.1

Re: [PATCH 1/5] ldconfig: avoid leak on empty paths in config file

[Arjun Shankar via Libc-alpha]

Hi Siddhesh,

> ---
>  elf/ldconfig.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/elf/ldconfig.c b/elf/ldconfig.c
> index 1037e8d0cf..b8893637f8 100644
> --- a/elf/ldconfig.c
> +++ b/elf/ldconfig.c
> @@ -503,7 +503,11 @@ add_dir_1 (const char *line, const char *from_file, int from_line)
>      entry->path[--i] = '\0';
>  
>    if (i == 0)
> -    return;
> +    {
> +      free (entry->path);
> +      free (entry);
> +      return;
> +    }
>  
>    char *path = entry->path;
>    if (opt_chroot != NULL)

This looks good to me.

entry and entry->path are allocated earlier via xmalloc and xstrdup
respectively and this frees them prior to an early conditional return.

Reviewed-by: Arjun Shankar <arjun@redhat.com>

Cheers!

[PATCH 2/5] gconv_parseconfdir: Fix memory leak

[Siddhesh Poyarekar via Libc-alpha]

The allocated `conf` would leak if we have to skip over the file due
to the underlying filesystem not supporting dt_type.
---
 iconv/gconv_parseconfdir.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h
index a4153e54c6..2f062689ec 100644
--- a/iconv/gconv_parseconfdir.h
+++ b/iconv/gconv_parseconfdir.h
@@ -153,12 +153,11 @@ gconv_parseconfdir (const char *dir, size_t dir_len)
 	      struct stat64 st;
 	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
 		continue;
-	      if (ent->d_type == DT_UNKNOWN
-		  && (lstat64 (conf, &st) == -1
-		      || !S_ISREG (st.st_mode)))
-		continue;
 
-	      found |= read_conf_file (conf, dir, dir_len);
+	      if (ent->d_type != DT_UNKNOWN
+		  || (lstat64 (conf, &st) != -1 && S_ISREG (st.st_mode)))
+		found |= read_conf_file (conf, dir, dir_len);
+
 	      free (conf);
 	    }
 	}
-- 
2.31.1

Re: [PATCH 2/5] gconv_parseconfdir: Fix memory leak

[Arjun Shankar via Libc-alpha]

Hi Siddhesh,

> The allocated `conf` would leak if we have to skip over the file due
> to the underlying filesystem not supporting dt_type.
> ---
>  iconv/gconv_parseconfdir.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/iconv/gconv_parseconfdir.h b/iconv/gconv_parseconfdir.h
> index a4153e54c6..2f062689ec 100644
> --- a/iconv/gconv_parseconfdir.h
> +++ b/iconv/gconv_parseconfdir.h
> @@ -153,12 +153,11 @@ gconv_parseconfdir (const char *dir, size_t dir_len)
>  	      struct stat64 st;
>  	      if (asprintf (&conf, "%s/%s", buf, ent->d_name) < 0)
>  		continue;
> -	      if (ent->d_type == DT_UNKNOWN
> -		  && (lstat64 (conf, &st) == -1
> -		      || !S_ISREG (st.st_mode)))
> -		continue;
>  
> -	      found |= read_conf_file (conf, dir, dir_len);
> +	      if (ent->d_type != DT_UNKNOWN
> +		  || (lstat64 (conf, &st) != -1 && S_ISREG (st.st_mode)))
> +		found |= read_conf_file (conf, dir, dir_len);
> +
>  	      free (conf);
>  	    }
>  	}

Reversed the condition to conditionally modify `found' first, then
unconditionally free `conf' afterward to avoid a leak. The change looks
correct to me.

I'm wondering if the new condition is harder to read than simply !ing the
old one. But I guess it's not that important compared to the fix itself.

Reviewed-by: Arjun Shankar <arjun@redhat.com>

Cheers!

[PATCH 3/5] iconv_charmap: Close output file when done

[Siddhesh Poyarekar via Libc-alpha]

---
 iconv/iconv_charmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/iconv/iconv_charmap.c b/iconv/iconv_charmap.c
index e2d53fee3c..a8b6b56124 100644
--- a/iconv/iconv_charmap.c
+++ b/iconv/iconv_charmap.c
@@ -234,6 +234,8 @@ charmap_conversion (const char *from_code, struct charmap_t *from_charmap,
     while (++remaining < argc);
 
   /* All done.  */
+  if (output != stdout)
+    fclose (output);
   free_table (cvtbl);
   return status;
 }
-- 
2.31.1

Re: [PATCH 3/5] iconv_charmap: Close output file when done

[Arjun Shankar via Libc-alpha]

Hi Siddhesh,

> ---
>  iconv/iconv_charmap.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/iconv/iconv_charmap.c b/iconv/iconv_charmap.c
> index e2d53fee3c..a8b6b56124 100644
> --- a/iconv/iconv_charmap.c
> +++ b/iconv/iconv_charmap.c
> @@ -234,6 +234,8 @@ charmap_conversion (const char *from_code, struct charmap_t *from_charmap,
>      while (++remaining < argc);
>  
>    /* All done.  */
> +  if (output != stdout)
> +    fclose (output);
>    free_table (cvtbl);
>    return status;
>  }

Earlier on, output is either the result of an fopen, or assigned stdout.
So, this change looks right.

Reviewed-by: Arjun Shankar <arjun@redhat.com>

Cheers!

[PATCH 4/5] copy_and_spawn_sgid: Avoid double calls to close()

[Siddhesh Poyarekar via Libc-alpha]

If close() on infd and outfd succeeded, reset the fd numbers so that
we don't attempt to close them again.
---
 support/support_capture_subprocess.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index 27bfd19c93..0bacf6dbc2 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -170,6 +170,7 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
      support_subprogram because we only want the program exit status, not the
      contents.  */
   ret = 0;
+  infd = outfd = -1;
 
   char * const args[] = {execname, child_id, NULL};
 
-- 
2.31.1

Re: [PATCH 4/5] copy_and_spawn_sgid: Avoid double calls to close()

[Arjun Shankar via Libc-alpha]

Hi Siddhesh,

> If close() on infd and outfd succeeded, reset the fd numbers so that
> we don't attempt to close them again.
> ---
>  support/support_capture_subprocess.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index 27bfd19c93..0bacf6dbc2 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -170,6 +170,7 @@ copy_and_spawn_sgid (char *child_id, gid_t gid)
>       support_subprogram because we only want the program exit status, not the
>       contents.  */
>    ret = 0;
> +  infd = outfd = -1;
>  
>    char * const args[] = {execname, child_id, NULL};

Looks good to me.

infd and outfd are indeed close'd above, and close'd again below if they are
non-negative. Setting to -1 ensures that we won't attempt to close them again.

Reviewed-by: Arjun Shankar <arjun@redhat.com>

Cheers!

[PATCH 5/5] gaiconf_init: Avoid double-free in label and precedence lists

[Siddhesh Poyarekar via Libc-alpha]

labellist and precedencelist could get freed a second time if there
are allocation failures, so set them to NULL to avoid a double-free.
---
 sysdeps/posix/getaddrinfo.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 838a68f022..43dfc6739e 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -2008,6 +2008,7 @@ gaiconf_init (void)
 	      l = l->next;
 	    }
 	  free_prefixlist (labellist);
+	  labellist = NULL;
 
 	  /* Sort the entries so that the most specific ones are at
 	     the beginning.  */
@@ -2046,6 +2047,7 @@ gaiconf_init (void)
 	      l = l->next;
 	    }
 	  free_prefixlist (precedencelist);
+	  precedencelist = NULL;
 
 	  /* Sort the entries so that the most specific ones are at
 	     the beginning.  */
-- 
2.31.1

Re: [PATCH 5/5] gaiconf_init: Avoid double-free in label and precedence lists

[Arjun Shankar via Libc-alpha]

Hi Siddhesh,

> labellist and precedencelist could get freed a second time if there
> are allocation failures, so set them to NULL to avoid a double-free.
> ---
>  sysdeps/posix/getaddrinfo.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index 838a68f022..43dfc6739e 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -2008,6 +2008,7 @@ gaiconf_init (void)
>  	      l = l->next;
>  	    }
>  	  free_prefixlist (labellist);
> +	  labellist = NULL;
>  
>  	  /* Sort the entries so that the most specific ones are at
>  	     the beginning.  */
> @@ -2046,6 +2047,7 @@ gaiconf_init (void)
>  	      l = l->next;
>  	    }
>  	  free_prefixlist (precedencelist);
> +	  precedencelist = NULL;
>  
>  	  /* Sort the entries so that the most specific ones are at
>  	     the beginning.  */

Looks good to me.

Yes, a later allocation failure can trigger a `goto no_file' which
tries to free these (again, in this case). Setting to NULL avoids that.

Reviewed-by: Arjun Shankar <arjun@redhat.com>

Cheers!