diff mbox series

resolv/res_query: Add note indicating that alloca usage is safe.

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

Commit Message

Joe Simmons-Talbott July 5, 2023, 6:13 p.m. UTC
The buffer size is small (< 1024) and fixed sized so alloca is safe
here.
---
 resolv/res_query.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Joe Simmons-Talbott Aug. 10, 2023, 1:46 p.m. UTC | #1
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
>
Joe Simmons-Talbott Aug. 28, 2023, 1:21 p.m. UTC | #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
> > 
>
Adhemerval Zanella Netto Aug. 28, 2023, 1:50 p.m. UTC | #3
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);
Joe Simmons-Talbott Aug. 28, 2023, 2:58 p.m. UTC | #4
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);
>
Joe Simmons-Talbott Sept. 14, 2023, 3:04 p.m. UTC | #5
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);
> > 
>
Adhemerval Zanella Netto Sept. 14, 2023, 5:36 p.m. UTC | #6
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 mbox series

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;