Re: [PATCH 1/2] Fix __check_pf()/make_request() stack overflow segfault (convert to alloca_account)

[OndÅej BÃlka <neleai at seznam dot cz>]

On Fri, Oct 04, 2013 at 04:29:02PM -0400, Debabrata Banerjee wrote:
> Tested calls to getaddrinfo() with 64k+ local IPv4 and IPv6 addresses.
> 
> Changelog:
> 
> 2013-10-04  Debabrata Banerjee  <dbanerje@akamai.com>
> 
>         * sysdeps/unix/sysv/linux/check_pf.c (make_request): use alloca_account
>         for allocations and fallback to malloc, preventing potential segfaults
> 
> Signed-off-by: Debabrata Banerjee <dbanerje@akamai.com>
> ---
>  sysdeps/unix/sysv/linux/check_pf.c | 95 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
> index 34c2146..76bf516 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);
>    else
>      {
>        buf = malloc (buf_size);
> @@ -239,7 +242,10 @@ make_request (int fd, pid_t pid)
>  		    }
>  		}
>  
this one is ok


> -	      struct in6ailist *newp = alloca (sizeof (*newp));
> +	      if (__libc_use_alloca (alloca_used + sizeof (struct in6ailist)))
> +	        {
> +	          struct in6ailist *newp =
> +	              alloca_account (sizeof (*newp), alloca_used);
>  	      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;
>  	    }
> +	      else
> +	        {
> +	          size_t allocate_size;
> +
> +	          if (result_size < sizeof (*result)
snip
> +	          else
> +	            memcpy (result->in6ai[in6ailistlen].addr, address,
> +	                sizeof (result->in6ai[in6ailistlen].addr));
> +
> +	          ++in6ailistlen;
> +	        }
> +	    }

This one duplicates too much code. A better way would be keep allocation
logic in one place. A simple way would be keep linked list of objects
that need freeing like

if (__libc_use_alloca (alloca_used + sizeof (struct in6ailist)))
  *newp = alloca_account (sizeof (*newp), alloca_used);
else
  {
    void **tmp = malloc (sizeof (*newp) + sizeof (void **));
    *tmp = to_free;
    to_free = tmp;
    newp = (struct in6ailist*) (newp + 1);
  }

and then free them by

while (to_free)
  {
    void *next = *to_free;
    free (to_free);
    to_free = (void **) next;
  }


>  	  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
> +      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;

These do not look relevant to fixing stack overflow, move them to
separate patch.

> @@ -305,6 +378,8 @@ make_request (int fd, pid_t pid)
>  out_fail:
>    if (use_malloc)
>      free (buf);
> +  if (result)
> +    free (result);

You do not have to add if as free(NULL) is legal call.