Re: [PATCH 04/10] sscanf: fix overflow
On Tue, May 05 2015, Alexey Dobriyan wrote:
> On Tue, May 5, 2015 at 12:51 PM, Rasmus Villemoes
> wrote:
>> On Sat, May 02 2015, Alexey Dobriyan wrote:
>>
>>> Fun fact:
>>>
>>> uint8_t val;
>>> sscanf("256", "%hhu", &val);
>>>
>>> will return 1 (as it should), and make val=0 (as it should not).
>>>
>>
>> What do you base these "should" and "should not" on? Both C99 and POSIX
>> say that the behaviour is undefined - the kernel can obviously define
>> its own semantics for scanf, but what do you think they should be?
>
> POSIX can say whatever it wants,
That was sort of the point, POSIX doesn't say anything, which is why I
asked what you think the semantics should be.
> it's about common sense.
>
> sscanf(), both kernel and libc, in this situation returns 0 when "0"
> character is nowhere to be found in the string! It should either
> return 25
Really? Do you consider it common sense that sscanf("256 123", "%hhu%d", &x,
&y) can end up returning 2, putting 25 in x and 6 in y?
> or do not return anything
I agree that _that_ would be the sane thing to do, but again, I'm
confused why you then said the first example should return 1.
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 04/10] sscanf: fix overflow
On Tue, May 5, 2015 at 12:51 PM, Rasmus Villemoes
wrote:
> On Sat, May 02 2015, Alexey Dobriyan wrote:
>
>> Fun fact:
>>
>> uint8_t val;
>> sscanf("256", "%hhu", &val);
>>
>> will return 1 (as it should), and make val=0 (as it should not).
>>
>
> What do you base these "should" and "should not" on? Both C99 and POSIX
> say that the behaviour is undefined - the kernel can obviously define
> its own semantics for scanf, but what do you think they should be?
POSIX can say whatever it wants, it's about common sense.
sscanf(), both kernel and libc, in this situation returns 0
when "0" character is nowhere to be found in the string!
It should either return 25 or do not return anything
because situation is ambiguous (read: set ERANGE).
> If we want to correctly handle overflow, the only sane way is to make
> sscanf return 0 in the above case (no conversions done). This also
> seems to be what your patch does, but then I'm confused by your
> first "as it should".
>
>> Apart from correctness, patch allows to remove checks and switch
>> to proper types in several (most?) cases:
>>
>> grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .
>>
>> Such checks can be incorrect too, checking for 3 digits with %3u
>> for parsing uint8_t is not enough.
>
> Yeah, and it may be too much; sscanf("0042", "%hhu", &val") should give
> 42, not 4. I agree that one should be able to rely on scanf doing range
> checking as part of matching.
>
> Actually, I think one should go through all the callers of sscanf which
> use a field width with an integer conversion and see if we can get rid
> of it, and then rip it away from the sscanf implementation. Otherwise
> there's another bug which would need fixing, namely
>
> int x;
> char rest[50];
> sscanf("12345678901234567890", "%3d%s", &x, rest)
>
> should successfully return 2 (storing 123 in x), but it can't when the
> strategy is to convert as much as possible (which may then give an early
> failure due to overflow), then divide by 10 until we haven't consumed
> more than we're allowed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 04/10] sscanf: fix overflow
On Sat, May 02 2015, Alexey Dobriyan wrote:
> Fun fact:
>
> uint8_t val;
> sscanf("256", "%hhu", &val);
>
> will return 1 (as it should), and make val=0 (as it should not).
>
What do you base these "should" and "should not" on? Both C99 and POSIX
say that the behaviour is undefined - the kernel can obviously define
its own semantics for scanf, but what do you think they should be?
If we want to correctly handle overflow, the only sane way is to make
sscanf return 0 in the above case (no conversions done). This also
seems to be what your patch does, but then I'm confused by your
first "as it should".
> Apart from correctness, patch allows to remove checks and switch
> to proper types in several (most?) cases:
>
> grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .
>
> Such checks can be incorrect too, checking for 3 digits with %3u
> for parsing uint8_t is not enough.
Yeah, and it may be too much; sscanf("0042", "%hhu", &val") should give
42, not 4. I agree that one should be able to rely on scanf doing range
checking as part of matching.
Actually, I think one should go through all the callers of sscanf which
use a field width with an integer conversion and see if we can get rid
of it, and then rip it away from the sscanf implementation. Otherwise
there's another bug which would need fixing, namely
int x;
char rest[50];
sscanf("12345678901234567890", "%3d%s", &x, rest)
should successfully return 2 (storing 123 in x), but it can't when the
strategy is to convert as much as possible (which may then give an early
failure due to overflow), then divide by 10 until we haven't consumed
more than we're allowed.
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 04/10] sscanf: fix overflow
Fun fact:
uint8_t val;
sscanf("256", "%hhu", &val);
will return 1 (as it should), and make val=0 (as it should not).
Apart from correctness, patch allows to remove checks and switch
to proper types in several (most?) cases:
grep -e 'scanf.*%[0-9]\+[dioux]' -n -r .
Such checks can be incorrect too, checking for 3 digits with %3u
for parsing uint8_t is not enough.
Signed-off-by: Alexey Dobriyan
---
lib/vsprintf.c | 45 ++---
1 file changed, 34 insertions(+), 11 deletions(-)
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2632,44 +2632,67 @@ int vsscanf(const char *buf, const char *fmt, va_list
args)
switch (qualifier) {
case 'H': /* that's 'hh' in format */
- if (is_sign)
+ if (is_sign) {
+ if (val.s != (signed char)val.s)
+ goto out;
*va_arg(args, signed char *) = val.s;
- else
+ } else {
+ if (val.u != (unsigned char)val.u)
+ goto out;
*va_arg(args, unsigned char *) = val.u;
+ }
break;
case 'h':
- if (is_sign)
+ if (is_sign) {
+ if (val.s != (short)val.s)
+ goto out;
*va_arg(args, short *) = val.s;
- else
+ } else {
+ if (val.u != (unsigned short)val.u)
+ goto out;
*va_arg(args, unsigned short *) = val.u;
+ }
break;
case 'l':
- if (is_sign)
+ if (is_sign) {
+ if (val.s != (long)val.s)
+ goto out;
*va_arg(args, long *) = val.s;
- else
+ } else {
+ if (val.u != (unsigned long)val.u)
+ goto out;
*va_arg(args, unsigned long *) = val.u;
+ }
break;
case 'L':
- if (is_sign)
+ if (is_sign) {
*va_arg(args, long long *) = val.s;
- else
+ } else {
*va_arg(args, unsigned long long *) = val.u;
+ }
break;
case 'Z':
case 'z':
+ if (val.u != (size_t)val.u)
+ goto out;
*va_arg(args, size_t *) = val.u;
break;
default:
- if (is_sign)
+ if (is_sign) {
+ if (val.s != (int)val.s)
+ goto out;
*va_arg(args, int *) = val.s;
- else
+ } else {
+ if (val.u != (unsigned int)val.u)
+ goto out;
*va_arg(args, unsigned int *) = val.u;
+ }
break;
}
num++;
str += len;
}
-
+out:
return num;
}
EXPORT_SYMBOL(vsscanf);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
