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

Message ID d4e55042-0138-c2ff-a1ba-4b782030e884@gmail.com
State New
Headers show
Series
  • [v3] replace sprintf with stpcpy to avoid GCC warning [BZ#28439]
Related show

Commit Message

Florian Weimer via Libc-alpha Oct. 9, 2021, 11:43 p.m.
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

Comments

Florian Weimer Oct. 10, 2021, 1:53 p.m. | #1
* 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).
Florian Weimer via Libc-alpha Oct. 11, 2021, 3:43 p.m. | #2
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

Patch

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,