This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [patch] Fix [BZ#14122] memory leak in nss_parse_service_list
On 05/21/2012 07:13 AM, Paul Pluzhnikov wrote:
On Sun, May 20, 2012 at 4:22 PM, Paul Pluzhnikov<ppluzhnikov@google.com> wrote:
Here is a rather simple fix for [BZ#14122]: memory leak in
nss_parse_service_list.
That patch fixed leaks in the error paths, but did not fix the original problem.
Attached is an updated patch that fixes both.
Re-tested on Linux/x86_64 an i686.
Thanks,
--
Paul Pluzhnikov
2012-05-20 Paul Pluzhnikov<ppluzhnikov@google.com>
[BZ #14122]
* nss/nsswitch.c (defconfig_entries): New variable.
(__nss_database_lookup): Don't leak defconfig entries.
(nss_parse_service_list): Don't leak on error paths.
(free_database_entries): New function.
(free_defconfig): New function.
(free_mem): Move common code to free_database_entries.
diff --git a/nss/nsswitch.c b/nss/nsswitch.c
index 53ff5f8..3614856 100644
--- a/nss/nsswitch.c
+++ b/nss/nsswitch.c
@@ -86,6 +86,9 @@ static const char *const __nss_shlib_revision = LIBNSS_FILES_SO + 15;
/* The root of the whole data base. */
static name_database *service_table;
+/* defconfig entries. */
Please give a more descriptive comment. What about
List of default service lists that are generated by glibc since
/etc/nsswitch does not provide a value. The list is only allocated for
freeing it at the end.
+static name_database_entry *defconfig_entries;
+
/* Nonzero if this is the nscd process. */
static bool is_nscd;
@@ -141,8 +144,25 @@ __nss_database_lookup (const char *database, const char *alternate_name,
DEFCONFIG specifies the default service list for this database,
or null to use the most common default. */
if (*ni == NULL)
- *ni = nss_parse_service_list (defconfig
- ?: "nis [NOTFOUND=return] files");
+ {
+ *ni = nss_parse_service_list (defconfig
+ ?: "nis [NOTFOUND=return] files");
+ if (*ni != NULL)
+ {
+ /* Don't leak the memory we've just allocated. */
+ name_database_entry *entry;
+
Add a comment for the +1, e.g.:
Allocate ENTRY plus size of name (1 here)
+ entry = (name_database_entry *) malloc (sizeof (*entry) + 1);
+
+ if (entry != NULL)
+ {
+ entry->next = defconfig_entries;
+ entry->service = *ni;
+ entry->name[0] = '\0';
+ defconfig_entries = entry;
+ }
+ }
+ }
__libc_lock_unlock (lock);
@@ -644,7 +664,7 @@ nss_parse_service_list (const char *line)
else if (__strncasecmp (name, "UNAVAIL", 7) == 0)
status = NSS_STATUS_UNAVAIL;
else
- return result;
+ goto finish;
}
else if (line - name == 8)
{
@@ -653,15 +673,15 @@ nss_parse_service_list (const char *line)
else if (__strncasecmp (name, "TRYAGAIN", 8) == 0)
status = NSS_STATUS_TRYAGAIN;
else
- return result;
+ goto finish;
}
else
- return result;
+ goto finish;
while (isspace (line[0]))
++line;
if (line[0] != '=')
- return result;
+ goto finish;
do
++line;
while (isspace (line[0]));
@@ -677,7 +697,7 @@ nss_parse_service_list (const char *line)
&& __strncasecmp (name, "CONTINUE", 8) == 0)
action = NSS_ACTION_CONTINUE;
else
- return result;
+ goto finish;
if (not)
{
@@ -705,6 +725,11 @@ nss_parse_service_list (const char *line)
*nextp = new_service;
nextp = &new_service->next;
+ continue;
+
+ finish:
+ free (new_service);
+ return result;
}
}
@@ -816,22 +841,9 @@ __nss_disable_nscd (void (*cb) (size_t, struct traced_file *))
}
#endif
-
-/* Free all resources if necessary. */
-libc_freeres_fn (free_mem)
+static void
+free_database_entries (name_database_entry *entry)
{
- name_database *top = service_table;
- name_database_entry *entry;
- service_library *library;
-
- if (top == NULL)
- /* Maybe we have not read the nsswitch.conf file. */
- return;
-
- /* Don't disturb ongoing other threads (if there are any). */
- service_table = NULL;
-
- entry = top->entry;
while (entry != NULL)
{
name_database_entry *olde = entry;
@@ -851,6 +863,36 @@ libc_freeres_fn (free_mem)
entry = entry->next;
free (olde);
}
+}
+
+/* Free all resources if necessary. */
+libc_freeres_fn (free_defconfig)
+{
+ name_database_entry *entry = defconfig_entries;
+
+ if (entry == NULL)
+ /* defconfig was not used. */
+ return;
+
+ /* Don't disturb ongoing other threads (if there are any). */
+ defconfig_entries = NULL;
+
+ free_database_entries (entry);
+}
+
+libc_freeres_fn (free_mem)
+{
+ name_database *top = service_table;
+ service_library *library;
+
+ if (top == NULL)
+ /* Maybe we have not read the nsswitch.conf file. */
+ return;
+
+ /* Don't disturb ongoing other threads (if there are any). */
+ service_table = NULL;
+
+ free_database_entries (top->entry);
library = top->library;
while (library != NULL)
The patch is fine, please add some comments as proposed and commit,
While you're add it, change the first two lines as well:
-/* Copyright (C) 1996-2012
- Free Software Foundation, Inc.
+/* Copyright (C) 1996-2012 Free Software Foundation, Inc.
thanks,
Andreas
--
Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
GPG fingerprint = 93A3 365E CE47 B889 DF7F FED1 389A 563C C272 A126