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

  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
  

* 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
  

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,
  

* 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