Account for alloca use when collecting interface addresses
diff mbox

Message ID mvm61n3puag.fsf@hawking.suse.de
State New
Headers show

Commit Message

Andreas Schwab March 24, 2014, 2:04 p.m. UTC
# ip li add name dummy0 type dummy
# site_id=$(head -c6 /dev/urandom | od -tx2 -An | tr ' ' ':')
# for ((i = 0; i < 65536; i++)) do
> ip ad ad $(printf fd80$site_id::%04x $i)/128 dev dummy0
> done
# (ulimit -s 900; getent ahosts localhost)
# ip li de dummy0

	[BZ #16002]
	* sysdeps/unix/sysv/linux/check_pf.c (make_request): Use
	alloca_account and account alloca use for struct in6ailist.
---
 sysdeps/unix/sysv/linux/check_pf.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

Comments

Siddhesh Poyarekar March 24, 2014, 2:30 p.m. UTC | #1
On Mon, Mar 24, 2014 at 03:04:23PM +0100, Andreas Schwab wrote:
> # ip li add name dummy0 type dummy
> # site_id=$(head -c6 /dev/urandom | od -tx2 -An | tr ' ' ':')
> # for ((i = 0; i < 65536; i++)) do
> > ip ad ad $(printf fd80$site_id::%04x $i)/128 dev dummy0
> > done
> # (ulimit -s 900; getent ahosts localhost)
> # ip li de dummy0

This needs a more verbose description for the commit log.  The change
itself looks OK to me.

Thanks,
Siddhesh

> 	[BZ #16002]
> 	* sysdeps/unix/sysv/linux/check_pf.c (make_request): Use
> 	alloca_account and account alloca use for struct in6ailist.
> ---
>  sysdeps/unix/sysv/linux/check_pf.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
> index 5c8e75a..6d8468d 100644
> --- a/sysdeps/unix/sysv/linux/check_pf.c
> +++ b/sysdeps/unix/sysv/linux/check_pf.c
> @@ -139,9 +139,10 @@ make_request (int fd, pid_t pid)
>  #endif
>    bool use_malloc = false;
>    char *buf;
> +  size_t alloca_used = 0;
>  
>    if (__libc_use_alloca (buf_size))
> -    buf = alloca (buf_size);
> +    buf = alloca_account (buf_size, alloca_used);
>    else
>      {
>        buf = malloc (buf_size);
> @@ -163,6 +164,7 @@ make_request (int fd, pid_t pid)
>    {
>      struct in6addrinfo info;
>      struct in6ailist *next;
> +    bool use_malloc;
>    } *in6ailist = NULL;
>    size_t in6ailistlen = 0;
>    bool seen_ipv4 = false;
> @@ -239,7 +241,19 @@ make_request (int fd, pid_t pid)
>  		    }
>  		}
>  
> -	      struct in6ailist *newp = alloca (sizeof (*newp));
> +	      struct in6ailist *newp;
> +	      if (__libc_use_alloca (alloca_used + sizeof (*newp)))
> +		{
> +		  newp = alloca_account (sizeof (*newp), alloca_used);
> +		  newp->use_malloc = false;
> +		}
> +	      else
> +		{
> +		  newp = malloc (sizeof (*newp));
> +		  if (newp == NULL)
> +		    goto out_fail;
> +		  newp->use_malloc = true;
> +		}
>  	      newp->info.flags = (((ifam->ifa_flags
>  				    & (IFA_F_DEPRECATED
>  				       | IFA_F_OPTIMISTIC))
> @@ -286,7 +300,10 @@ make_request (int fd, pid_t pid)
>        do
>  	{
>  	  result->in6ai[--in6ailistlen] = in6ailist->info;
> -	  in6ailist = in6ailist->next;
> +	  struct in6ailist *next = in6ailist->next;
> +	  if (in6ailist->use_malloc)
> +	    free (in6ailist);
> +	  in6ailist = next;
>  	}
>        while (in6ailist != NULL);
>      }
> @@ -302,7 +319,14 @@ make_request (int fd, pid_t pid)
>      free (buf);
>    return result;
>  
> -out_fail:
> + out_fail:
> +  while (in6ailist != NULL)
> +    {
> +      struct in6ailist *next = in6ailist->next;
> +      if (in6ailist->use_malloc)
> +	free (in6ailist);
> +      in6ailist = next;
> +    }
>    if (use_malloc)
>      free (buf);
>    return NULL;
> -- 
> 1.9.1
> 
> 
> -- 
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."
Andreas Schwab March 24, 2014, 2:41 p.m. UTC | #2
Siddhesh Poyarekar <siddhesh@redhat.com> writes:

> On Mon, Mar 24, 2014 at 03:04:23PM +0100, Andreas Schwab wrote:
>> # ip li add name dummy0 type dummy
>> # site_id=$(head -c6 /dev/urandom | od -tx2 -An | tr ' ' ':')
>> # for ((i = 0; i < 65536; i++)) do
>> > ip ad ad $(printf fd80$site_id::%04x $i)/128 dev dummy0
>> > done
>> # (ulimit -s 900; getent ahosts localhost)
>> # ip li de dummy0
>
> This needs a more verbose description for the commit log.

It's just the reproducer, you can try yourself without breaking anything
(unless you already have a dummy0 device).

Andreas.
Siddhesh Poyarekar March 24, 2014, 3:38 p.m. UTC | #3
On Mon, Mar 24, 2014 at 03:41:54PM +0100, Andreas Schwab wrote:
> Siddhesh Poyarekar <siddhesh@redhat.com> writes:
> 
> > On Mon, Mar 24, 2014 at 03:04:23PM +0100, Andreas Schwab wrote:
> >> # ip li add name dummy0 type dummy
> >> # site_id=$(head -c6 /dev/urandom | od -tx2 -An | tr ' ' ':')
> >> # for ((i = 0; i < 65536; i++)) do
> >> > ip ad ad $(printf fd80$site_id::%04x $i)/128 dev dummy0
> >> > done
> >> # (ulimit -s 900; getent ahosts localhost)
> >> # ip li de dummy0
> >
> > This needs a more verbose description for the commit log.
> 
> It's just the reproducer, you can try yourself without breaking anything
> (unless you already have a dummy0 device).

Oh I got that; I just thought that it would be a good idea to put it
in the commit log since we can't add it in the testsuite and hence
suggested putting some English text around it to describe it, which is
kind of what you did in your commit :)

Siddhesh
Ondřej Bílka March 24, 2014, 4:57 p.m. UTC | #4
On Mon, Mar 24, 2014 at 03:04:23PM +0100, Andreas Schwab wrote:
> # ip li add name dummy0 type dummy
> # site_id=$(head -c6 /dev/urandom | od -tx2 -An | tr ' ' ':')
> # for ((i = 0; i < 65536; i++)) do
> > ip ad ad $(printf fd80$site_id::%04x $i)/128 dev dummy0
> > done
> # (ulimit -s 900; getent ahosts localhost)
> # ip li de dummy0
> 
> 	[BZ #16002]
> 	* sysdeps/unix/sysv/linux/check_pf.c (make_request): Use
> 	alloca_account and account alloca use for struct in6ailist.

As far as I recall there was a similar patch that did not get in.

After I reviewed that I noticed that you could drop alloca altogether.

Its only used to construct a single linked list and then copy it to
malloced array. Faster way would be to malloc array directly, realloc as
necessary.
Andreas Schwab March 24, 2014, 6:14 p.m. UTC | #5
Ondřej Bílka <neleai@seznam.cz> writes:

> As far as I recall there was a similar patch that did not get in.
>
> After I reviewed that I noticed that you could drop alloca altogether.
>
> Its only used to construct a single linked list and then copy it to
> malloced array. Faster way would be to malloc array directly, realloc as
> necessary.

Sorry, I missed it when I searched for __check_pf.  Could you rebase
your patch?  This issue is orthogonal to the alloca issue.

Andreas.
Joseph Myers March 24, 2014, 9:19 p.m. UTC | #6
I'm seeing compilation warnings (we clearly need to move to a -Werror 
default, I see a recent patch of mine introduced some as well...):

../sysdeps/unix/sysv/linux/check_pf.c:326:20: warning: 'in6ailist' may be used uninitialized in this function [-Wmaybe-uninitialized]

The warning looks right to me: the change introduced uses of in6ailist 
after the out_fail label, while there are jumps to out_fail from before 
in6ailist is declared and initialized to NULL.

Patch
diff mbox

diff --git a/sysdeps/unix/sysv/linux/check_pf.c b/sysdeps/unix/sysv/linux/check_pf.c
index 5c8e75a..6d8468d 100644
--- a/sysdeps/unix/sysv/linux/check_pf.c
+++ b/sysdeps/unix/sysv/linux/check_pf.c
@@ -139,9 +139,10 @@  make_request (int fd, pid_t pid)
 #endif
   bool use_malloc = false;
   char *buf;
+  size_t alloca_used = 0;
 
   if (__libc_use_alloca (buf_size))
-    buf = alloca (buf_size);
+    buf = alloca_account (buf_size, alloca_used);
   else
     {
       buf = malloc (buf_size);
@@ -163,6 +164,7 @@  make_request (int fd, pid_t pid)
   {
     struct in6addrinfo info;
     struct in6ailist *next;
+    bool use_malloc;
   } *in6ailist = NULL;
   size_t in6ailistlen = 0;
   bool seen_ipv4 = false;
@@ -239,7 +241,19 @@  make_request (int fd, pid_t pid)
 		    }
 		}
 
-	      struct in6ailist *newp = alloca (sizeof (*newp));
+	      struct in6ailist *newp;
+	      if (__libc_use_alloca (alloca_used + sizeof (*newp)))
+		{
+		  newp = alloca_account (sizeof (*newp), alloca_used);
+		  newp->use_malloc = false;
+		}
+	      else
+		{
+		  newp = malloc (sizeof (*newp));
+		  if (newp == NULL)
+		    goto out_fail;
+		  newp->use_malloc = true;
+		}
 	      newp->info.flags = (((ifam->ifa_flags
 				    & (IFA_F_DEPRECATED
 				       | IFA_F_OPTIMISTIC))
@@ -286,7 +300,10 @@  make_request (int fd, pid_t pid)
       do
 	{
 	  result->in6ai[--in6ailistlen] = in6ailist->info;
-	  in6ailist = in6ailist->next;
+	  struct in6ailist *next = in6ailist->next;
+	  if (in6ailist->use_malloc)
+	    free (in6ailist);
+	  in6ailist = next;
 	}
       while (in6ailist != NULL);
     }
@@ -302,7 +319,14 @@  make_request (int fd, pid_t pid)
     free (buf);
   return result;
 
-out_fail:
+ out_fail:
+  while (in6ailist != NULL)
+    {
+      struct in6ailist *next = in6ailist->next;
+      if (in6ailist->use_malloc)
+	free (in6ailist);
+      in6ailist = next;
+    }
   if (use_malloc)
     free (buf);
   return NULL;