Message ID | mvm61n3puag.fsf@hawking.suse.de |
---|---|
State | New |
Headers | show |
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."
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.
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
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.
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.
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.
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;