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

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

[Martin Sebor]

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).

I considered using #pragma GCC diagnostic but using strcpy
here seems to me preferable than sprintf: thanks to
the precondition check it's equally as safe but lighter-weight
and no less readable.

Tested on x86_64-linux running Fedora 29.

Martin

index 75b0e5f2f7..31ab1db60b 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);
+               strcpy (nbuf, name);
+               nbuf[n] = '.';
+               strcpy (nbuf + n + 1, domain);
         }
         return __res_context_query (ctx, longname, class, type, answer,
                                     anslen, answerp, answerp2, nanswerp2,

Re: [PATCH] replace sprintf with strcpy to avoid GCC warning [BZ#28439]

[Florian Weimer]

* 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.

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

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

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

[-- Attachment #2: glibc-28439.diff --]
[-- Type: text/x-patch, Size: 1135 bytes --]

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);

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

[Florian Weimer]

* 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.

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

[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?

Martin

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

[Paul Eggert]

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,

[PATCH v3] replace sprintf with stpcpy to avoid GCC warning [BZ#28439]

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

On 10/9/21 3:57 PM, Paul Eggert wrote:
> 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,

That works for me too.  Attached is v3 with the above patch for
Patchwork to test.  It builds and passes with no extra failures
for me on x86_64-linux.

I'll plan to commit this version sometime next week if it passes
and there's no further feedback.  Florian, please pipe up if you
have any concerns with it.

Martin

[-- Attachment #2: glibc-28439.diff --]
[-- Type: text/x-patch, Size: 509 bytes --]

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,

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

[Florian Weimer]

* 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.

Re: [PATCH v3] replace sprintf with stpcpy to avoid GCC warning [BZ#28439]

[Florian Weimer]

* Martin Sebor:

> 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,

I don't see any problems with this patch, although I dislike the use
of stpcpy in general (mempcpy and stpcopy are both closely associated
with buggy code).

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

[Martin Sebor]

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

Re: [PATCH v3] replace sprintf with stpcpy to avoid GCC warning [BZ#28439]

[Martin Sebor]

On 10/10/21 7:53 AM, Florian Weimer wrote:
> * Martin Sebor:
> 
>> 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,
> 
> I don't see any problems with this patch, although I dislike the use
> of stpcpy in general (mempcpy and stpcopy are both closely associated
> with buggy code).
> 

Okay, thanks.  I've pushed the change in
eb73b87897798de981dbbf019aa957045d768adb.

Martin