Message ID | 20230620214031.1241057-1-josimmon@redhat.com |
---|---|
State | New |
Headers | show |
Series | getaddrinfo: Get rid of alloca | expand |
On 20/06/23 18:40, Joe Simmons-Talbott via Libc-alpha wrote: > Use a scratch_buffer rather than alloca to avoid potential stack > overflow. > --- > sysdeps/posix/getaddrinfo.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > > diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c > index 0356b622be..442475d621 100644 > --- a/sysdeps/posix/getaddrinfo.c > +++ b/sysdeps/posix/getaddrinfo.c > @@ -2404,22 +2404,17 @@ getaddrinfo (const char *name, const char *service, > struct addrinfo *q; > struct addrinfo *last = NULL; > char *canonname = NULL; > - bool malloc_results; > size_t alloc_size = nresults * (sizeof (*results) + sizeof (size_t)); > + struct scratch_buffer buf; > + scratch_buffer_init (&buf); > > - malloc_results > - = !__libc_use_alloca (alloc_size); > - if (malloc_results) > + if (!scratch_buffer_set_array_size (&buf, 1, alloc_size)) I think it would be better to use: if (!scratch_buffer_set_array_size (&buf, nresults, sizeof (*results) + sizeof (size_t))) [...] To use the overflow checks done by scratch_buffer_set_array_size ( sizeof (*results) + sizeof (size_t) is always safe so I think there is no need to add extra checks). > { > - results = malloc (alloc_size); > - if (results == NULL) > - { > - __free_in6ai (in6ai); > - return EAI_MEMORY; > - } > + __free_in6ai (in6ai); > + return EAI_MEMORY; > } > - else > - results = alloca (alloc_size); > + results = buf.data; > + > order = (size_t *) (results + nresults); > > /* Now we definitely need the interface information. */ > @@ -2590,8 +2585,7 @@ getaddrinfo (const char *name, const char *service, > /* Fill in the canonical name into the new first entry. */ > p->ai_canonname = canonname; > > - if (malloc_results) > - free (results); > + scratch_buffer_free (&buf); > } > > __free_in6ai (in6ai);
On Fri, Jun 30, 2023 at 10:15:48AM -0300, Adhemerval Zanella Netto wrote: > > > On 20/06/23 18:40, Joe Simmons-Talbott via Libc-alpha wrote: > > Use a scratch_buffer rather than alloca to avoid potential stack > > overflow. > > --- > > sysdeps/posix/getaddrinfo.c | 22 ++++++++-------------- > > 1 file changed, 8 insertions(+), 14 deletions(-) > > > > diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c > > index 0356b622be..442475d621 100644 > > --- a/sysdeps/posix/getaddrinfo.c > > +++ b/sysdeps/posix/getaddrinfo.c > > @@ -2404,22 +2404,17 @@ getaddrinfo (const char *name, const char *service, > > struct addrinfo *q; > > struct addrinfo *last = NULL; > > char *canonname = NULL; > > - bool malloc_results; > > size_t alloc_size = nresults * (sizeof (*results) + sizeof (size_t)); > > + struct scratch_buffer buf; > > + scratch_buffer_init (&buf); > > > > - malloc_results > > - = !__libc_use_alloca (alloc_size); > > - if (malloc_results) > > + if (!scratch_buffer_set_array_size (&buf, 1, alloc_size)) > > I think it would be better to use: > > if (!scratch_buffer_set_array_size (&buf, nresults, > sizeof (*results) + sizeof (size_t))) > [...] > > To use the overflow checks done by scratch_buffer_set_array_size ( > sizeof (*results) + sizeof (size_t) is always safe so I think there is no > need to add extra checks). You are correct. Thanks for catching that and for the review. I've pushed your suggestion as v2. Thanks, Joe
diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c index 0356b622be..442475d621 100644 --- a/sysdeps/posix/getaddrinfo.c +++ b/sysdeps/posix/getaddrinfo.c @@ -2404,22 +2404,17 @@ getaddrinfo (const char *name, const char *service, struct addrinfo *q; struct addrinfo *last = NULL; char *canonname = NULL; - bool malloc_results; size_t alloc_size = nresults * (sizeof (*results) + sizeof (size_t)); + struct scratch_buffer buf; + scratch_buffer_init (&buf); - malloc_results - = !__libc_use_alloca (alloc_size); - if (malloc_results) + if (!scratch_buffer_set_array_size (&buf, 1, alloc_size)) { - results = malloc (alloc_size); - if (results == NULL) - { - __free_in6ai (in6ai); - return EAI_MEMORY; - } + __free_in6ai (in6ai); + return EAI_MEMORY; } - else - results = alloca (alloc_size); + results = buf.data; + order = (size_t *) (results + nresults); /* Now we definitely need the interface information. */ @@ -2590,8 +2585,7 @@ getaddrinfo (const char *name, const char *service, /* Fill in the canonical name into the new first entry. */ p->ai_canonname = canonname; - if (malloc_results) - free (results); + scratch_buffer_free (&buf); } __free_in6ai (in6ai);