[v2] replace sprintf with strcpy to avoid GCC warning [BZ#28439]
Checks
Context |
Check |
Description |
dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
dj/TryBot-32bit |
success
|
Build for i686
|
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
* 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
@@ -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);