| Message ID | 20230705181341.1470594-1-josimmon@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | resolv/res_query: Add note indicating that alloca usage is safe. | expand |
On Wed, Jul 05, 2023 at 02:13:41PM -0400, Joe Simmons-Talbott wrote: > The buffer size is small (< 1024) and fixed sized so alloca is safe > here. Ping. Thanks, Joe > --- > resolv/res_query.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index 049de91b95..0e0e7be624 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > int n, use_malloc = 0; > > size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > + /* alloca is safe here since bufsize < 1024 and fixed sized. */ > u_char *buf = alloca (bufsize); > u_char *query1 = buf; > int nquery1 = -1; > -- > 2.39.2 >
Ping. On Thu, Aug 10, 2023 at 09:46:55AM -0400, Joe Simmons-Talbott via Libc-alpha wrote: > On Wed, Jul 05, 2023 at 02:13:41PM -0400, Joe Simmons-Talbott wrote: > > The buffer size is small (< 1024) and fixed sized so alloca is safe > > here. > Ping. > > Thanks, > Joe > > --- > > resolv/res_query.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/resolv/res_query.c b/resolv/res_query.c > > index 049de91b95..0e0e7be624 100644 > > --- a/resolv/res_query.c > > +++ b/resolv/res_query.c > > @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > int n, use_malloc = 0; > > > > size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > > + /* alloca is safe here since bufsize < 1024 and fixed sized. */ > > u_char *buf = alloca (bufsize); > > u_char *query1 = buf; > > int nquery1 = -1; > > -- > > 2.39.2 > > >
On 05/07/23 15:13, Joe Simmons-Talbott via Libc-alpha wrote: > The buffer size is small (< 1024) and fixed sized so alloca is safe > here. > --- > resolv/res_query.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index 049de91b95..0e0e7be624 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > int n, use_malloc = 0; > > size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > + /* alloca is safe here since bufsize < 1024 and fixed sized. */ > u_char *buf = alloca (bufsize); The bufsize on current Linux build is: size_t bufsize = (type == 439963904 ? 2 : 1) * (12 + 4 + 255 + 1); So with upper bound as 544 (2 * (12 + 4 + 255 + 1)). However, it might increase to 2 * PACKETSIZE later with malloc. This is exactly the scenarion scratch_buffer was created, so maybe we should use it. Below a complete untested patch: diff --git a/resolv/res_query.c b/resolv/res_query.c index 049de91b95..b234db83c1 100644 --- a/resolv/res_query.c +++ b/resolv/res_query.c @@ -80,6 +80,7 @@ #include <stdlib.h> #include <string.h> #include <shlib-compat.h> +#include <scratch_buffer.h> #if PACKETSZ > 65536 #define MAXPACKET PACKETSZ @@ -114,11 +115,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, struct __res_state *statp = ctx->resp; UHEADER *hp = (UHEADER *) answer; UHEADER *hp2; - int n, use_malloc = 0; - - size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; - u_char *buf = alloca (bufsize); - u_char *query1 = buf; + int n; + + /* It requires 2 times QUERYSIZE for type == T_QUERY_A_AND_AAAA. */ + struct scratch_buffer buf; + scratch_buffer_init (&buf); + _Static_assert (2 * QUERYSIZE <= sizeof (buf.__space.__c), + "scratch_buffer too small"); + u_char *query1 = buf.data; int nquery1 = -1; u_char *query2 = NULL; int nquery2 = 0; @@ -129,14 +133,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, if (type == T_QUERY_A_AND_AAAA) { n = __res_context_mkquery (ctx, QUERY, name, class, T_A, NULL, - query1, bufsize); + query1, buf.length); if (n > 0) { if ((statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) { /* Use RESOLV_EDNS_BUFFER_SIZE because the receive buffer can be reallocated. */ - n = __res_nopt (ctx, n, query1, bufsize, + n = __res_nopt (ctx, n, query1, buf.length, RESOLV_EDNS_BUFFER_SIZE); if (n < 0) goto unspec_nomem; @@ -146,20 +150,20 @@ __res_context_query (struct resolv_context *ctx, const char *name, /* Align the buffer. */ int npad = ((nquery1 + __alignof__ (HEADER) - 1) & ~(__alignof__ (HEADER) - 1)) - nquery1; - if (n > bufsize - npad) + if (n > buf.length - npad) { n = -1; goto unspec_nomem; } int nused = n + npad; - query2 = buf + nused; + query2 = buf.data + nused; n = __res_context_mkquery (ctx, QUERY, name, class, T_AAAA, - NULL, query2, bufsize - nused); + NULL, query2, buf.length - nused); if (n > 0 && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) /* Use RESOLV_EDNS_BUFFER_SIZE because the receive buffer can be reallocated. */ - n = __res_nopt (ctx, n, query2, bufsize, + n = __res_nopt (ctx, n, query2, buf.length, RESOLV_EDNS_BUFFER_SIZE); nquery2 = n; } @@ -169,7 +173,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, else { n = __res_context_mkquery (ctx, QUERY, name, class, type, NULL, - query1, bufsize); + query1, buf.length); if (n > 0 && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) @@ -181,27 +185,25 @@ __res_context_query (struct resolv_context *ctx, const char *name, advertise = anslen; else advertise = RESOLV_EDNS_BUFFER_SIZE; - n = __res_nopt (ctx, n, query1, bufsize, advertise); + n = __res_nopt (ctx, n, query1, buf.length, advertise); } nquery1 = n; } - if (__glibc_unlikely (n <= 0) && !use_malloc) { + if (__glibc_unlikely (n <= 0)) { /* Retry just in case res_nmkquery failed because of too short buffer. Shouldn't happen. */ - bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * MAXPACKET; - buf = malloc (bufsize); - if (buf != NULL) { - query1 = buf; - use_malloc = 1; + if (scratch_buffer_set_array_size (&buf, + T_QUERY_A_AND_AAAA ? 2 : 1, + MAXPACKET)) { + query1 = buf.data; goto again; } } if (__glibc_unlikely (n <= 0)) { RES_SET_H_ERRNO(statp, NO_RECOVERY); - if (use_malloc) - free (buf); + scratch_buffer_free (&buf); return (n); } @@ -224,8 +226,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, answerp2_malloced); } - if (use_malloc) - free (buf); + scratch_buffer_free (&buf); if (n < 0) { RES_SET_H_ERRNO(statp, TRY_AGAIN); return (n);
On Mon, Aug 28, 2023 at 10:50:21AM -0300, Adhemerval Zanella Netto wrote: > > > On 05/07/23 15:13, Joe Simmons-Talbott via Libc-alpha wrote: > > The buffer size is small (< 1024) and fixed sized so alloca is safe > > here. > > --- > > resolv/res_query.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/resolv/res_query.c b/resolv/res_query.c > > index 049de91b95..0e0e7be624 100644 > > --- a/resolv/res_query.c > > +++ b/resolv/res_query.c > > @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > int n, use_malloc = 0; > > > > size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > > + /* alloca is safe here since bufsize < 1024 and fixed sized. */ > > u_char *buf = alloca (bufsize); > > The bufsize on current Linux build is: > > size_t bufsize = (type == 439963904 ? 2 : 1) * (12 + 4 + 255 + 1); > > So with upper bound as 544 (2 * (12 + 4 + 255 + 1)). However, it might > increase to 2 * PACKETSIZE later with malloc. This is exactly the scenarion > scratch_buffer was created, so maybe we should use it. Below a complete > untested patch: Thanks for the patch. I've tested it with 'make check' on x86_64-linux-gnu. I'm not sure how to handle this and don't feel comfortable submitting it on your behalf or as my own work but am happy to give a Reviewed-by once there's a commit message. Thanks, Joe > > diff --git a/resolv/res_query.c b/resolv/res_query.c > index 049de91b95..b234db83c1 100644 > --- a/resolv/res_query.c > +++ b/resolv/res_query.c > @@ -80,6 +80,7 @@ > #include <stdlib.h> > #include <string.h> > #include <shlib-compat.h> > +#include <scratch_buffer.h> > > #if PACKETSZ > 65536 > #define MAXPACKET PACKETSZ > @@ -114,11 +115,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, > struct __res_state *statp = ctx->resp; > UHEADER *hp = (UHEADER *) answer; > UHEADER *hp2; > - int n, use_malloc = 0; > - > - size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > - u_char *buf = alloca (bufsize); > - u_char *query1 = buf; > + int n; > + > + /* It requires 2 times QUERYSIZE for type == T_QUERY_A_AND_AAAA. */ > + struct scratch_buffer buf; > + scratch_buffer_init (&buf); > + _Static_assert (2 * QUERYSIZE <= sizeof (buf.__space.__c), > + "scratch_buffer too small"); > + u_char *query1 = buf.data; > int nquery1 = -1; > u_char *query2 = NULL; > int nquery2 = 0; > @@ -129,14 +133,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, > if (type == T_QUERY_A_AND_AAAA) > { > n = __res_context_mkquery (ctx, QUERY, name, class, T_A, NULL, > - query1, bufsize); > + query1, buf.length); > if (n > 0) > { > if ((statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > { > /* Use RESOLV_EDNS_BUFFER_SIZE because the receive > buffer can be reallocated. */ > - n = __res_nopt (ctx, n, query1, bufsize, > + n = __res_nopt (ctx, n, query1, buf.length, > RESOLV_EDNS_BUFFER_SIZE); > if (n < 0) > goto unspec_nomem; > @@ -146,20 +150,20 @@ __res_context_query (struct resolv_context *ctx, const char *name, > /* Align the buffer. */ > int npad = ((nquery1 + __alignof__ (HEADER) - 1) > & ~(__alignof__ (HEADER) - 1)) - nquery1; > - if (n > bufsize - npad) > + if (n > buf.length - npad) > { > n = -1; > goto unspec_nomem; > } > int nused = n + npad; > - query2 = buf + nused; > + query2 = buf.data + nused; > n = __res_context_mkquery (ctx, QUERY, name, class, T_AAAA, > - NULL, query2, bufsize - nused); > + NULL, query2, buf.length - nused); > if (n > 0 > && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > /* Use RESOLV_EDNS_BUFFER_SIZE because the receive > buffer can be reallocated. */ > - n = __res_nopt (ctx, n, query2, bufsize, > + n = __res_nopt (ctx, n, query2, buf.length, > RESOLV_EDNS_BUFFER_SIZE); > nquery2 = n; > } > @@ -169,7 +173,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > else > { > n = __res_context_mkquery (ctx, QUERY, name, class, type, NULL, > - query1, bufsize); > + query1, buf.length); > > if (n > 0 > && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > @@ -181,27 +185,25 @@ __res_context_query (struct resolv_context *ctx, const char *name, > advertise = anslen; > else > advertise = RESOLV_EDNS_BUFFER_SIZE; > - n = __res_nopt (ctx, n, query1, bufsize, advertise); > + n = __res_nopt (ctx, n, query1, buf.length, advertise); > } > > nquery1 = n; > } > > - if (__glibc_unlikely (n <= 0) && !use_malloc) { > + if (__glibc_unlikely (n <= 0)) { > /* Retry just in case res_nmkquery failed because of too > short buffer. Shouldn't happen. */ > - bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * MAXPACKET; > - buf = malloc (bufsize); > - if (buf != NULL) { > - query1 = buf; > - use_malloc = 1; > + if (scratch_buffer_set_array_size (&buf, > + T_QUERY_A_AND_AAAA ? 2 : 1, > + MAXPACKET)) { > + query1 = buf.data; > goto again; > } > } > if (__glibc_unlikely (n <= 0)) { > RES_SET_H_ERRNO(statp, NO_RECOVERY); > - if (use_malloc) > - free (buf); > + scratch_buffer_free (&buf); > return (n); > } > > @@ -224,8 +226,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > answerp2_malloced); > } > > - if (use_malloc) > - free (buf); > + scratch_buffer_free (&buf); > if (n < 0) { > RES_SET_H_ERRNO(statp, TRY_AGAIN); > return (n); >
Ping. On Mon, Aug 28, 2023 at 10:58:25AM -0400, Joe Simmons-Talbott via Libc-alpha wrote: > On Mon, Aug 28, 2023 at 10:50:21AM -0300, Adhemerval Zanella Netto wrote: > > > > > > On 05/07/23 15:13, Joe Simmons-Talbott via Libc-alpha wrote: > > > The buffer size is small (< 1024) and fixed sized so alloca is safe > > > here. > > > --- > > > resolv/res_query.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/resolv/res_query.c b/resolv/res_query.c > > > index 049de91b95..0e0e7be624 100644 > > > --- a/resolv/res_query.c > > > +++ b/resolv/res_query.c > > > @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > > int n, use_malloc = 0; > > > > > > size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > > > + /* alloca is safe here since bufsize < 1024 and fixed sized. */ > > > u_char *buf = alloca (bufsize); > > > > The bufsize on current Linux build is: > > > > size_t bufsize = (type == 439963904 ? 2 : 1) * (12 + 4 + 255 + 1); > > > > So with upper bound as 544 (2 * (12 + 4 + 255 + 1)). However, it might > > increase to 2 * PACKETSIZE later with malloc. This is exactly the scenarion > > scratch_buffer was created, so maybe we should use it. Below a complete > > untested patch: > > Thanks for the patch. I've tested it with 'make check' on > x86_64-linux-gnu. I'm not sure how to handle this and don't feel > comfortable submitting it on your behalf or as my own work but am happy > to give a Reviewed-by once there's a commit message. > > Thanks, > Joe > > > > diff --git a/resolv/res_query.c b/resolv/res_query.c > > index 049de91b95..b234db83c1 100644 > > --- a/resolv/res_query.c > > +++ b/resolv/res_query.c > > @@ -80,6 +80,7 @@ > > #include <stdlib.h> > > #include <string.h> > > #include <shlib-compat.h> > > +#include <scratch_buffer.h> > > > > #if PACKETSZ > 65536 > > #define MAXPACKET PACKETSZ > > @@ -114,11 +115,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > struct __res_state *statp = ctx->resp; > > UHEADER *hp = (UHEADER *) answer; > > UHEADER *hp2; > > - int n, use_malloc = 0; > > - > > - size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; > > - u_char *buf = alloca (bufsize); > > - u_char *query1 = buf; > > + int n; > > + > > + /* It requires 2 times QUERYSIZE for type == T_QUERY_A_AND_AAAA. */ > > + struct scratch_buffer buf; > > + scratch_buffer_init (&buf); > > + _Static_assert (2 * QUERYSIZE <= sizeof (buf.__space.__c), > > + "scratch_buffer too small"); > > + u_char *query1 = buf.data; > > int nquery1 = -1; > > u_char *query2 = NULL; > > int nquery2 = 0; > > @@ -129,14 +133,14 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > if (type == T_QUERY_A_AND_AAAA) > > { > > n = __res_context_mkquery (ctx, QUERY, name, class, T_A, NULL, > > - query1, bufsize); > > + query1, buf.length); > > if (n > 0) > > { > > if ((statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > > { > > /* Use RESOLV_EDNS_BUFFER_SIZE because the receive > > buffer can be reallocated. */ > > - n = __res_nopt (ctx, n, query1, bufsize, > > + n = __res_nopt (ctx, n, query1, buf.length, > > RESOLV_EDNS_BUFFER_SIZE); > > if (n < 0) > > goto unspec_nomem; > > @@ -146,20 +150,20 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > /* Align the buffer. */ > > int npad = ((nquery1 + __alignof__ (HEADER) - 1) > > & ~(__alignof__ (HEADER) - 1)) - nquery1; > > - if (n > bufsize - npad) > > + if (n > buf.length - npad) > > { > > n = -1; > > goto unspec_nomem; > > } > > int nused = n + npad; > > - query2 = buf + nused; > > + query2 = buf.data + nused; > > n = __res_context_mkquery (ctx, QUERY, name, class, T_AAAA, > > - NULL, query2, bufsize - nused); > > + NULL, query2, buf.length - nused); > > if (n > 0 > > && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > > /* Use RESOLV_EDNS_BUFFER_SIZE because the receive > > buffer can be reallocated. */ > > - n = __res_nopt (ctx, n, query2, bufsize, > > + n = __res_nopt (ctx, n, query2, buf.length, > > RESOLV_EDNS_BUFFER_SIZE); > > nquery2 = n; > > } > > @@ -169,7 +173,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > else > > { > > n = __res_context_mkquery (ctx, QUERY, name, class, type, NULL, > > - query1, bufsize); > > + query1, buf.length); > > > > if (n > 0 > > && (statp->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0) > > @@ -181,27 +185,25 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > advertise = anslen; > > else > > advertise = RESOLV_EDNS_BUFFER_SIZE; > > - n = __res_nopt (ctx, n, query1, bufsize, advertise); > > + n = __res_nopt (ctx, n, query1, buf.length, advertise); > > } > > > > nquery1 = n; > > } > > > > - if (__glibc_unlikely (n <= 0) && !use_malloc) { > > + if (__glibc_unlikely (n <= 0)) { > > /* Retry just in case res_nmkquery failed because of too > > short buffer. Shouldn't happen. */ > > - bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * MAXPACKET; > > - buf = malloc (bufsize); > > - if (buf != NULL) { > > - query1 = buf; > > - use_malloc = 1; > > + if (scratch_buffer_set_array_size (&buf, > > + T_QUERY_A_AND_AAAA ? 2 : 1, > > + MAXPACKET)) { > > + query1 = buf.data; > > goto again; > > } > > } > > if (__glibc_unlikely (n <= 0)) { > > RES_SET_H_ERRNO(statp, NO_RECOVERY); > > - if (use_malloc) > > - free (buf); > > + scratch_buffer_free (&buf); > > return (n); > > } > > > > @@ -224,8 +226,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, > > answerp2_malloced); > > } > > > > - if (use_malloc) > > - free (buf); > > + scratch_buffer_free (&buf); > > if (n < 0) { > > RES_SET_H_ERRNO(statp, TRY_AGAIN); > > return (n); > > >
On 14/09/23 12:04, Joe Simmons-Talbott wrote: > Ping. > > On Mon, Aug 28, 2023 at 10:58:25AM -0400, Joe Simmons-Talbott via Libc-alpha wrote: >> On Mon, Aug 28, 2023 at 10:50:21AM -0300, Adhemerval Zanella Netto wrote: >>> >>> >>> On 05/07/23 15:13, Joe Simmons-Talbott via Libc-alpha wrote: >>>> The buffer size is small (< 1024) and fixed sized so alloca is safe >>>> here. >>>> --- >>>> resolv/res_query.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/resolv/res_query.c b/resolv/res_query.c >>>> index 049de91b95..0e0e7be624 100644 >>>> --- a/resolv/res_query.c >>>> +++ b/resolv/res_query.c >>>> @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, >>>> int n, use_malloc = 0; >>>> >>>> size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; >>>> + /* alloca is safe here since bufsize < 1024 and fixed sized. */ >>>> u_char *buf = alloca (bufsize); >>> >>> The bufsize on current Linux build is: >>> >>> size_t bufsize = (type == 439963904 ? 2 : 1) * (12 + 4 + 255 + 1); >>> >>> So with upper bound as 544 (2 * (12 + 4 + 255 + 1)). However, it might >>> increase to 2 * PACKETSIZE later with malloc. This is exactly the scenarion >>> scratch_buffer was created, so maybe we should use it. Below a complete >>> untested patch: >> >> Thanks for the patch. I've tested it with 'make check' on >> x86_64-linux-gnu. I'm not sure how to handle this and don't feel >> comfortable submitting it on your behalf or as my own work but am happy >> to give a Reviewed-by once there's a commit message. I had forgotten about this, I will send a proper patch.
diff --git a/resolv/res_query.c b/resolv/res_query.c index 049de91b95..0e0e7be624 100644 --- a/resolv/res_query.c +++ b/resolv/res_query.c @@ -117,6 +117,7 @@ __res_context_query (struct resolv_context *ctx, const char *name, int n, use_malloc = 0; size_t bufsize = (type == T_QUERY_A_AND_AAAA ? 2 : 1) * QUERYSIZE; + /* alloca is safe here since bufsize < 1024 and fixed sized. */ u_char *buf = alloca (bufsize); u_char *query1 = buf; int nquery1 = -1;