Re: [PATCH] Fix __check_pf()/make_request() stack overflow segfault (convert to alloca_account) and memory leak.

["Carlos O'Donell" <carlos at redhat dot com>]

On 10/03/2013 05:51 PM, Debabrata Banerjee wrote:
> Copyright assignment now on file with FSF.

Confirmed you have a copyright on file.

Is there a detailed description of the exact problems
you are fixing with this patch?

Could you please file one or more bugs in bugzilla for 
these user visible defects?

Could you please split this into at least 2 patches
for review.

The first patch should be as minimal as possible and 
convert to alloca_account.

The next patch should fix the memory leak.

> Tested with valgrind --leak-check=full --show-reachable=yes while calling
> getaddrinfo() and subsequent freeaddrinfo() with 64k+ local IPv4 and IPv6 addresses.

Full testsuite run on x86-64? Regressions?

Please make sure you've read through:
http://sourceware.org/glibc/wiki/Contribution%20checklist

I'll review this right now so you can incorporate the review into your next
version.

> Changelog:
> 
> 2013-10-03  Debabrata Banerjee  <dbanerje@akamai.com>
> 

	[BZ #XXXX]

You'd add this line matching the bug # you fixed. This way we can track
the fix and users can reopen if they have problems with the fix.

> 	* sysdeps/unix/sysv/linux/check_pf.c (make_request): use alloca_account

s/use/Use/g.

> 	for allocations and fallback to malloc, preventing potential segfaults

s/, preventing potential segfaults//g.

Should be terse and describe what changed not why.

> 	(__check_pf): fix memory leak and locking when not building in NSCD.
> 	(__free_in6ai): Likewise.
> 

Note: When the fix gets checked in the NEWS file 


> Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
> ---
>  sysdeps/unix/sysv/linux/check_pf.c | 118 +++++++++++++++++++++++++++++++------
>  1 file changed, 101 insertions(+), 17 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
> index 34c2146..8a86571 100644
> --- a/sysdeps/unix/sysv/linux/check_pf.c
> +++ b/sysdeps/unix/sysv/linux/check_pf.c
> @@ -139,9 +139,12 @@ make_request (int fd, pid_t pid)
>  #endif
>    bool use_malloc = false;
>    char *buf;
> +  size_t alloca_used = 0;
> +  size_t result_size = 0;
> +  struct cached_data *result = NULL;
>  
>    if (__libc_use_alloca (buf_size))
> -    buf = alloca (buf_size);
> +    buf = alloca_account (buf_size, alloca_used);

ok.

>    else
>      {
>        buf = malloc (buf_size);
> @@ -239,7 +242,10 @@ make_request (int fd, pid_t pid)
>  		    }
>  		}
>  
> -	      struct in6ailist *newp = alloca (sizeof (*newp));
> +	      if (__libc_use_alloca (alloca_used + sizeof (struct in6ailist)))
> +	        {
> +	          struct in6ailist *newp =
> +	              alloca_account (sizeof (*newp), alloca_used);

All the code after this in the body of the if has the wrong indentation.

>  	      newp->info.flags = (((ifam->ifa_flags
>  				    & (IFA_F_DEPRECATED
>  				       | IFA_F_OPTIMISTIC))
> @@ -262,6 +268,58 @@ make_request (int fd, pid_t pid)
>  	      in6ailist = newp;
>  	      ++in6ailistlen;
>  	    }

This code also has the wrong indentation.

> +	      else
> +	        {
> +	          size_t allocate_size;
> +
> +	          if (result_size < sizeof (*result)
> +	              + ((in6ailistlen + 1) * sizeof (struct in6addrinfo)))
> +	            {
> +	              allocate_size = sizeof (*result)
> +	                      + (2 * in6ailistlen * sizeof (struct in6addrinfo));
> +	              void *newbuf = realloc (result, allocate_size);
> +	              if (newbuf != NULL)
> +	                result = newbuf;
> +	              else
> +	                goto out_fail;
> +
> +                      if (result_size == 0)
> +                        {
> +                          int i = 0;
> +                          do
> +                            {
> +                              result->in6ai[i++] = in6ailist->info;
> +                              in6ailist = in6ailist->next;
> +                            }
> +                          while (in6ailist != NULL);
> +                        }
> +
> +                      result_size = allocate_size;
> +	            }
> +
> +	          result->in6ai[in6ailistlen].flags = (((ifam->ifa_flags
> +	              & (IFA_F_DEPRECATED
> +	                  | IFA_F_OPTIMISTIC))
> +	              ? in6ai_deprecated : 0)
> +	              | ((ifam->ifa_flags
> +	                  & IFA_F_HOMEADDRESS)
> +	                  ? in6ai_homeaddress : 0));

All of this has to indent correctly against the starting brace.

> +	          result->in6ai[in6ailistlen].prefixlen = ifam->ifa_prefixlen;
> +	          result->in6ai[in6ailistlen].index = ifam->ifa_index;
> +	          if (ifam->ifa_family == AF_INET)
> +	            {
> +	              result->in6ai[in6ailistlen].addr[0] = 0;
> +	              result->in6ai[in6ailistlen].addr[1] = 0;
> +	              result->in6ai[in6ailistlen].addr[2] = htonl (0xffff);
> +	              result->in6ai[in6ailistlen].addr[3] = *(const in_addr_t *) address;
> +	            }
> +	          else
> +	            memcpy (result->in6ai[in6ailistlen].addr, address,
> +	                sizeof (result->in6ai[in6ailistlen].addr));
> +
> +	          ++in6ailistlen;
> +	        }
> +	    }

Is there no way we can refactor this into a function?

I would really like to avoid the duplication.

The refactoring would be an extra patch, but would really
clean this up.

>  	  else if (nlmh->nlmsg_type == NLMSG_DONE)
>  	    /* We found the end, leave the loop.  */
>  	    done = true;
> @@ -269,20 +327,15 @@ make_request (int fd, pid_t pid)
>      }
>    while (! done);
>  
> -  struct cached_data *result;
> -  if (seen_ipv6 && in6ailist != NULL)
> +  if (seen_ipv6)
> +    {
> +      if (result == NULL && in6ailist != NULL)
>      {
>        result = malloc (sizeof (*result)
>  		       + in6ailistlen * sizeof (struct in6addrinfo));
>        if (result == NULL)
>  	goto out_fail;
>  
> -      result->timestamp = get_nl_timestamp ();
> -      result->usecnt = 2;
> -      result->seen_ipv4 = seen_ipv4;
> -      result->seen_ipv6 = true;
> -      result->in6ailen = in6ailistlen;
> -
>        do
>  	{
>  	  result->in6ai[--in6ailistlen] = in6ailist->info;
> @@ -290,9 +343,29 @@ make_request (int fd, pid_t pid)
>  	}
>        while (in6ailist != NULL);
>      }
> +
> +#ifdef IS_IN_nscd
> +      result->timestamp = nl_timestamp;
> +      result->usecnt = 2;
> +#elif defined(USE_NSCD)
> +      result->timestamp = __nscd_get_nl_timestamp ();
> +      result->usecnt = 2;
> +#else
> +      result->usecnt = 1;
> +#endif

This looks like a fixup for an incomplete transition
for supporting IS_IN_nscd/USE_NSDCD which should be 
a distinct patch.

> +      result->seen_ipv4 = seen_ipv4;
> +      result->seen_ipv6 = true;
> +      result->in6ailen = in6ailistlen;
> +    }
>    else
>      {
> +      if (result)
> +        free (result);
> +#if defined(IS_IN_nscd) || defined(USE_NSCD)
>        atomic_add (&noai6ai_cached.usecnt, 2);
> +#else
> +      atomic_add (&noai6ai_cached.usecnt, 1);
> +#endif
>        noai6ai_cached.seen_ipv4 = seen_ipv4;
>        noai6ai_cached.seen_ipv6 = seen_ipv6;
>        result = &noai6ai_cached;
> @@ -305,6 +378,8 @@ make_request (int fd, pid_t pid)
>  out_fail:
>    if (use_malloc)
>      free (buf);
> +  if (result)
> +    free (result);
>    return NULL;
>  }
>  
> @@ -320,6 +395,7 @@ __check_pf (bool *seen_ipv4, bool *seen_ipv6,
>    struct cached_data *olddata = NULL;
>    struct cached_data *data = NULL;
>  
> +#if defined(IS_IN_nscd) || defined(USE_NSCD)
>    __libc_lock_lock (lock);
>  
>    if (cache_valid_p ())
> @@ -328,6 +404,7 @@ __check_pf (bool *seen_ipv4, bool *seen_ipv6,
>        atomic_increment (&cache->usecnt);
>      }
>    else
> +#endif
>      {
>        int fd = __socket (PF_NETLINK, SOCK_RAW, NETLINK_ROUTE);
>  
> @@ -347,14 +424,21 @@ __check_pf (bool *seen_ipv4, bool *seen_ipv6,
>  	  close_not_cancel_no_status (fd);
>  	}
>  
> -      if (data != NULL)
> +#if defined(IS_IN_nscd) || defined(USE_NSCD)
> +      if (data != cache)
>  	{
>  	  olddata = cache;
>  	  cache = data;
> +
> +          if (olddata != NULL && atomic_add_zero (&olddata->usecnt, -1))
> +            free (olddata);
>  	}
>      }
>  
>    __libc_lock_unlock (lock);
> +#else
> +    }
> +#endif
>  
>    if (data != NULL)
>      {
> @@ -386,16 +470,16 @@ __free_in6ai (struct in6addrinfo *ai)
>        struct cached_data *data =
>  	(struct cached_data *) ((char *) ai
>  				- offsetof (struct cached_data, in6ai));
> -
> -      if (atomic_add_zero (&data->usecnt, -1))
> -	{
> +#if defined(IS_IN_nscd) || defined(USE_NSCD)
>  	  __libc_lock_lock (lock);
>  
> -	  if (data->usecnt == 0)
> -	    /* Still unused.  */
> +       if (atomic_add_zero (&data->usecnt, -1))
>  	    free (data);
>  
>  	  __libc_lock_unlock (lock);
> -	}
> +#else
> +       if (atomic_add_zero (&data->usecnt, -1))
> +         free(data);
> +#endif
>      }
>  }
> 

Cheers,
Carlos.