[patch] Fix [BZ#14122] memory leak in nss_parse_service_list

[Andreas Jaeger]

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