* [PATCH 1/5] ldconfig: avoid leak on empty paths in config file
2021-07-27 17:41 [PATCH 0/5] Static analysis fixes Siddhesh Poyarekar
@ 2021-07-27 17:41 ` Siddhesh Poyarekar
2021-08-03 15:08 ` Arjun Shankar
2021-07-27 17:41 ` [PATCH 2/5] gconv_parseconfdir: Fix memory leak Siddhesh Poyarekar
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Siddhesh Poyarekar @ 2021-07-27 17:41 UTC (permalink / raw)
To: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/5] ldconfig: avoid leak on empty paths in config file
2021-07-27 17:41 ` [PATCH 1/5] ldconfig: avoid leak on empty paths in config file Siddhesh Poyarekar
@ 2021-08-03 15:08 ` Arjun Shankar
0 siblings, 0 replies; 11+ messages in thread
From: Arjun Shankar @ 2021-08-03 15:08 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: 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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/5] gconv_parseconfdir: Fix memory leak
2021-07-27 17:41 [PATCH 0/5] Static analysis fixes Siddhesh Poyarekar
2021-07-27 17:41 ` [PATCH 1/5] ldconfig: avoid leak on empty paths in config file Siddhesh Poyarekar
@ 2021-07-27 17:41 ` Siddhesh Poyarekar
2021-08-03 15:09 ` Arjun Shankar
2021-07-27 17:41 ` [PATCH 3/5] iconv_charmap: Close output file when done Siddhesh Poyarekar
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Siddhesh Poyarekar @ 2021-07-27 17:41 UTC (permalink / raw)
To: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/5] gconv_parseconfdir: Fix memory leak
2021-07-27 17:41 ` [PATCH 2/5] gconv_parseconfdir: Fix memory leak Siddhesh Poyarekar
@ 2021-08-03 15:09 ` Arjun Shankar
0 siblings, 0 replies; 11+ messages in thread
From: Arjun Shankar @ 2021-08-03 15:09 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: 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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/5] iconv_charmap: Close output file when done
2021-07-27 17:41 [PATCH 0/5] Static analysis fixes Siddhesh Poyarekar
2021-07-27 17:41 ` [PATCH 1/5] ldconfig: avoid leak on empty paths in config file Siddhesh Poyarekar
2021-07-27 17:41 ` [PATCH 2/5] gconv_parseconfdir: Fix memory leak Siddhesh Poyarekar
@ 2021-07-27 17:41 ` Siddhesh Poyarekar
2021-08-03 15:15 ` Arjun Shankar
2021-07-27 17:41 ` [PATCH 4/5] copy_and_spawn_sgid: Avoid double calls to close() Siddhesh Poyarekar
2021-07-27 17:41 ` [PATCH 5/5] gaiconf_init: Avoid double-free in label and precedence lists Siddhesh Poyarekar
4 siblings, 1 reply; 11+ messages in thread
From: Siddhesh Poyarekar @ 2021-07-27 17:41 UTC (permalink / raw)
To: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/5] iconv_charmap: Close output file when done
2021-07-27 17:41 ` [PATCH 3/5] iconv_charmap: Close output file when done Siddhesh Poyarekar
@ 2021-08-03 15:15 ` Arjun Shankar
0 siblings, 0 replies; 11+ messages in thread
From: Arjun Shankar @ 2021-08-03 15:15 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: 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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/5] copy_and_spawn_sgid: Avoid double calls to close()
2021-07-27 17:41 [PATCH 0/5] Static analysis fixes Siddhesh Poyarekar
` (2 preceding siblings ...)
2021-07-27 17:41 ` [PATCH 3/5] iconv_charmap: Close output file when done Siddhesh Poyarekar
@ 2021-07-27 17:41 ` Siddhesh Poyarekar
2021-08-03 15:25 ` Arjun Shankar
2021-07-27 17:41 ` [PATCH 5/5] gaiconf_init: Avoid double-free in label and precedence lists Siddhesh Poyarekar
4 siblings, 1 reply; 11+ messages in thread
From: Siddhesh Poyarekar @ 2021-07-27 17:41 UTC (permalink / raw)
To: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/5] copy_and_spawn_sgid: Avoid double calls to close()
2021-07-27 17:41 ` [PATCH 4/5] copy_and_spawn_sgid: Avoid double calls to close() Siddhesh Poyarekar
@ 2021-08-03 15:25 ` Arjun Shankar
0 siblings, 0 replies; 11+ messages in thread
From: Arjun Shankar @ 2021-08-03 15:25 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: 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!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 5/5] gaiconf_init: Avoid double-free in label and precedence lists
2021-07-27 17:41 [PATCH 0/5] Static analysis fixes Siddhesh Poyarekar
` (3 preceding siblings ...)
2021-07-27 17:41 ` [PATCH 4/5] copy_and_spawn_sgid: Avoid double calls to close() Siddhesh Poyarekar
@ 2021-07-27 17:41 ` Siddhesh Poyarekar
2021-08-03 15:34 ` Arjun Shankar
4 siblings, 1 reply; 11+ messages in thread
From: Siddhesh Poyarekar @ 2021-07-27 17:41 UTC (permalink / raw)
To: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 5/5] gaiconf_init: Avoid double-free in label and precedence lists
2021-07-27 17:41 ` [PATCH 5/5] gaiconf_init: Avoid double-free in label and precedence lists Siddhesh Poyarekar
@ 2021-08-03 15:34 ` Arjun Shankar
0 siblings, 0 replies; 11+ messages in thread
From: Arjun Shankar @ 2021-08-03 15:34 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: 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!
^ permalink raw reply [flat|nested] 11+ messages in thread