[v2] replace sprintf with strcpy to avoid GCC warning [BZ#28439]

Message ID 33640df2-6ba0-5c0c-2829-747f813118b5@gmail.com
State New
Headers show
Series
  • [v2] replace sprintf with strcpy to avoid GCC warning [BZ#28439]
Related show

Commit Message

On 10/9/21 2:16 PM, Florian Weimer wrote:
> * Martin Sebor via Libc-alpha:

> 

>> The patch below replaces a call to sprintf with an equivalent

>> pair of strcpy calls to avoid a GCC false positive due to

>> a recent optimizer improvement (still under review).

> 

> What's the warning?  Can we use __snprintf instead?

> 

> The context looks like this:

> 

> 	char nbuf[MAXDNAME];

> 	size_t n, d;

> 

> 		n = strlen(name);

> 		d = strlen(domain);

> 		if (n + d + 1 >= MAXDNAME) {

> 			RES_SET_H_ERRNO(statp, NO_RECOVERY);

> 			return (-1);

> 		}

> 		sprintf(nbuf, "%s.%s", name, domain);

> 

> So it should be possible to use something like this (untested):

> 

>    char nbuf[MAXDNAME + 1];

> 

>    /* nbuf[MAXDNAME] is used to detect overlong inputs.  */

>    nbuf[MAXDNAME] = '\0';

>    __snprintf (nbuf, sizeof (nbuf), "%s.%s", name, domain);

>    if (nbuf[MAXDNAME] != '\0')

>      {

>        RES_SET_H_ERRNO(statp, NO_RECOVERY);

>        return -1;

>      }

> 

> But I don't know what the warning is about, and if it would still

> trigger.


The warning is in BZ#28439:

res_query.c: In function ‘__res_context_querydomain’:
res_query.c:613:35: warning: ‘%s’ directive writing up to 1023 bytes 
into a region of size between 1 and 1024 [-Wformat-overflow=]
   613 |                 sprintf(nbuf, "%s.%s", name, domain);
       |                                   ^~
res_query.c:613:17: note: ‘sprintf’ output between 2 and 2048 bytes into 
a destination of size 1025
   613 |                 sprintf(nbuf, "%s.%s", name, domain);
       |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Your change above suppresses is as well because without the assert
GCC knows nothing about the lengths of the source strings.  I would
have thought the strcpy approach preferable but if you want to use
snprintf the recommended way to detect truncation (and avoid
-Wformat-truncation) is testing the return value as in the attached
patch.  Also tested on x86_64-linux.

Martin

Comments

Florian Weimer Oct. 9, 2021, 9:15 p.m. | #1
* Martin Sebor:

> diff --git a/resolv/res_query.c b/resolv/res_query.c

> index 75b0e5f2f7..adc8a13f75 100644

> --- a/resolv/res_query.c

> +++ b/resolv/res_query.c

> @@ -589,10 +589,9 @@ __res_context_querydomain (struct resolv_context *ctx,

>  	struct __res_state *statp = ctx->resp;

>  	char nbuf[MAXDNAME];

>  	const char *longname = nbuf;

> -	size_t n, d;

>  

>  	if (domain == NULL) {

> -		n = strlen(name);

> +		size_t n = strlen(name);

>  

>  		/* Decrement N prior to checking it against MAXDNAME

>  		   so that we detect a wrap to SIZE_MAX and return

> @@ -603,15 +602,13 @@ __res_context_querydomain (struct resolv_context *ctx,

>  			return (-1);

>  		}

>  		longname = name;

> -	} else {

> -		n = strlen(name);

> -		d = strlen(domain);

> -		if (n + d + 1 >= MAXDNAME) {

> -			RES_SET_H_ERRNO(statp, NO_RECOVERY);

> -			return (-1);

> -		}

> -		sprintf(nbuf, "%s.%s", name, domain);

>  	}

> +	else if (__snprintf (nbuf, sizeof nbuf, "%s.%s", name, domain)

> +		 >= sizeof nbuf)

> +	  {

> +	    RES_SET_H_ERRNO(statp, NO_RECOVERY);

> +	    return -1;

> +	  }

>  	return __res_context_query (ctx, longname, class, type, answer,

>  				    anslen, answerp, answerp2, nanswerp2,

>  				    resplen2, answerp2_malloced);


Maybe add a comment about EOVERFLOW?  I think it still works because
the -1 from snprintf turns into SIZE_MAX.
On 10/9/21 3:15 PM, Florian Weimer wrote:
> * Martin Sebor:

> 

>> diff --git a/resolv/res_query.c b/resolv/res_query.c

>> index 75b0e5f2f7..adc8a13f75 100644

>> --- a/resolv/res_query.c

>> +++ b/resolv/res_query.c

>> @@ -589,10 +589,9 @@ __res_context_querydomain (struct resolv_context *ctx,

>>   	struct __res_state *statp = ctx->resp;

>>   	char nbuf[MAXDNAME];

>>   	const char *longname = nbuf;

>> -	size_t n, d;

>>   

>>   	if (domain == NULL) {

>> -		n = strlen(name);

>> +		size_t n = strlen(name);

>>   

>>   		/* Decrement N prior to checking it against MAXDNAME

>>   		   so that we detect a wrap to SIZE_MAX and return

>> @@ -603,15 +602,13 @@ __res_context_querydomain (struct resolv_context *ctx,

>>   			return (-1);

>>   		}

>>   		longname = name;

>> -	} else {

>> -		n = strlen(name);

>> -		d = strlen(domain);

>> -		if (n + d + 1 >= MAXDNAME) {

>> -			RES_SET_H_ERRNO(statp, NO_RECOVERY);

>> -			return (-1);

>> -		}

>> -		sprintf(nbuf, "%s.%s", name, domain);

>>   	}

>> +	else if (__snprintf (nbuf, sizeof nbuf, "%s.%s", name, domain)

>> +		 >= sizeof nbuf)

>> +	  {

>> +	    RES_SET_H_ERRNO(statp, NO_RECOVERY);

>> +	    return -1;

>> +	  }

>>   	return __res_context_query (ctx, longname, class, type, answer,

>>   				    anslen, answerp, answerp2, nanswerp2,

>>   				    resplen2, answerp2_malloced);

> 

> Maybe add a comment about EOVERFLOW?  I think it still works because

> the -1 from snprintf turns into SIZE_MAX.


snprintf returns "the number of bytes that would have been written
if sizeof buf had been sufficiently large" no?  Or is __snprintf
different?

Martin
Paul Eggert Oct. 9, 2021, 9:57 p.m. | #3
On 10/9/21 1:56 PM, Martin Sebor via Libc-alpha wrote:
> I would

> have thought the strcpy approach preferable


Me too. sprintf/snprintf/__snprintf is overkill here. Instead, I suggest 
something like the following (untested) patch, as it is easier for 
static analysis that would otherwise have to worry about the INT_MAX 
gorp that plagues the sprintf family.

diff --git a/resolv/res_query.c b/resolv/res_query.c
index 75b0e5f2f7..5d0a68dc81 100644
--- a/resolv/res_query.c
+++ b/resolv/res_query.c
@@ -610,7 +610,9 @@ __res_context_querydomain (struct resolv_context *ctx,
  			RES_SET_H_ERRNO(statp, NO_RECOVERY);
  			return (-1);
  		}
-		sprintf(nbuf, "%s.%s", name, domain);
+		char *p = __stpcpy (nbuf, name);
+		*p++ = '.';
+		strcpy (p, domain);
  	}
  	return __res_context_query (ctx, longname, class, type, answer,
  				    anslen, answerp, answerp2, nanswerp2,
Florian Weimer Oct. 10, 2021, 8:28 a.m. | #4
* Martin Sebor:

> On 10/9/21 3:15 PM, Florian Weimer wrote:

>> * Martin Sebor:

>> 

>>> diff --git a/resolv/res_query.c b/resolv/res_query.c

>>> index 75b0e5f2f7..adc8a13f75 100644

>>> --- a/resolv/res_query.c

>>> +++ b/resolv/res_query.c

>>> @@ -589,10 +589,9 @@ __res_context_querydomain (struct resolv_context *ctx,

>>>   	struct __res_state *statp = ctx->resp;

>>>   	char nbuf[MAXDNAME];

>>>   	const char *longname = nbuf;

>>> -	size_t n, d;

>>>   

>>>   	if (domain == NULL) {

>>> -		n = strlen(name);

>>> +		size_t n = strlen(name);

>>>   

>>>   		/* Decrement N prior to checking it against MAXDNAME

>>>   		   so that we detect a wrap to SIZE_MAX and return

>>> @@ -603,15 +602,13 @@ __res_context_querydomain (struct resolv_context *ctx,

>>>   			return (-1);

>>>   		}

>>>   		longname = name;

>>> -	} else {

>>> -		n = strlen(name);

>>> -		d = strlen(domain);

>>> -		if (n + d + 1 >= MAXDNAME) {

>>> -			RES_SET_H_ERRNO(statp, NO_RECOVERY);

>>> -			return (-1);

>>> -		}

>>> -		sprintf(nbuf, "%s.%s", name, domain);

>>>   	}

>>> +	else if (__snprintf (nbuf, sizeof nbuf, "%s.%s", name, domain)

>>> +		 >= sizeof nbuf)

>>> +	  {

>>> +	    RES_SET_H_ERRNO(statp, NO_RECOVERY);

>>> +	    return -1;

>>> +	  }

>>>   	return __res_context_query (ctx, longname, class, type, answer,

>>>   				    anslen, answerp, answerp2, nanswerp2,

>>>   				    resplen2, answerp2_malloced);

>> 

>> Maybe add a comment about EOVERFLOW?  I think it still works because

>> the -1 from snprintf turns into SIZE_MAX.

>

> snprintf returns "the number of bytes that would have been written

> if sizeof buf had been sufficiently large" no?  Or is __snprintf

> different?


The return type is int, not size_t, and there are two input arguments.
So there is potential for overflow.
On 10/10/21 2:28 AM, Florian Weimer wrote:
> * Martin Sebor:

> 

>> On 10/9/21 3:15 PM, Florian Weimer wrote:

>>> * Martin Sebor:

>>>

>>>> diff --git a/resolv/res_query.c b/resolv/res_query.c

>>>> index 75b0e5f2f7..adc8a13f75 100644

>>>> --- a/resolv/res_query.c

>>>> +++ b/resolv/res_query.c

>>>> @@ -589,10 +589,9 @@ __res_context_querydomain (struct resolv_context *ctx,

>>>>    	struct __res_state *statp = ctx->resp;

>>>>    	char nbuf[MAXDNAME];

>>>>    	const char *longname = nbuf;

>>>> -	size_t n, d;

>>>>    

>>>>    	if (domain == NULL) {

>>>> -		n = strlen(name);

>>>> +		size_t n = strlen(name);

>>>>    

>>>>    		/* Decrement N prior to checking it against MAXDNAME

>>>>    		   so that we detect a wrap to SIZE_MAX and return

>>>> @@ -603,15 +602,13 @@ __res_context_querydomain (struct resolv_context *ctx,

>>>>    			return (-1);

>>>>    		}

>>>>    		longname = name;

>>>> -	} else {

>>>> -		n = strlen(name);

>>>> -		d = strlen(domain);

>>>> -		if (n + d + 1 >= MAXDNAME) {

>>>> -			RES_SET_H_ERRNO(statp, NO_RECOVERY);

>>>> -			return (-1);

>>>> -		}

>>>> -		sprintf(nbuf, "%s.%s", name, domain);

>>>>    	}

>>>> +	else if (__snprintf (nbuf, sizeof nbuf, "%s.%s", name, domain)

>>>> +		 >= sizeof nbuf)

>>>> +	  {

>>>> +	    RES_SET_H_ERRNO(statp, NO_RECOVERY);

>>>> +	    return -1;

>>>> +	  }

>>>>    	return __res_context_query (ctx, longname, class, type, answer,

>>>>    				    anslen, answerp, answerp2, nanswerp2,

>>>>    				    resplen2, answerp2_malloced);

>>>

>>> Maybe add a comment about EOVERFLOW?  I think it still works because

>>> the -1 from snprintf turns into SIZE_MAX.

>>

>> snprintf returns "the number of bytes that would have been written

>> if sizeof buf had been sufficiently large" no?  Or is __snprintf

>> different?

> 

> The return type is int, not size_t, and there are two input arguments.

> So there is potential for overflow.


Ah, I see what you meant by EOVERFLOW now.   Yes, the conversion
to size_t would have handled the case of any error but I agree
that calling out the overflow might have been helpful.

Martin

Patch

diff --git a/resolv/res_query.c b/resolv/res_query.c
index 75b0e5f2f7..adc8a13f75 100644
--- a/resolv/res_query.c
+++ b/resolv/res_query.c
@@ -589,10 +589,9 @@  __res_context_querydomain (struct resolv_context *ctx,
 	struct __res_state *statp = ctx->resp;
 	char nbuf[MAXDNAME];
 	const char *longname = nbuf;
-	size_t n, d;
 
 	if (domain == NULL) {
-		n = strlen(name);
+		size_t n = strlen(name);
 
 		/* Decrement N prior to checking it against MAXDNAME
 		   so that we detect a wrap to SIZE_MAX and return
@@ -603,15 +602,13 @@  __res_context_querydomain (struct resolv_context *ctx,
 			return (-1);
 		}
 		longname = name;
-	} else {
-		n = strlen(name);
-		d = strlen(domain);
-		if (n + d + 1 >= MAXDNAME) {
-			RES_SET_H_ERRNO(statp, NO_RECOVERY);
-			return (-1);
-		}
-		sprintf(nbuf, "%s.%s", name, domain);
 	}
+	else if (__snprintf (nbuf, sizeof nbuf, "%s.%s", name, domain)
+		 >= sizeof nbuf)
+	  {
+	    RES_SET_H_ERRNO(statp, NO_RECOVERY);
+	    return -1;
+	  }
 	return __res_context_query (ctx, longname, class, type, answer,
 				    anslen, answerp, answerp2, nanswerp2,
 				    resplen2, answerp2_malloced);