linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexey Dobriyan <adobriyan@gmail.com>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 04/10] sscanf: fix overflow
Date: Tue, 5 May 2015 14:10:36 +0300	[thread overview]
Message-ID: <CACVxJT9yTO8-wRJ4Y_w=DguCiniyu_dP1BXg+KE1ME+PrabgrA@mail.gmail.com> (raw)
In-Reply-To: <87egmvfj2c.fsf@rasmusvillemoes.dk>

On Tue, May 5, 2015 at 12:51 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Sat, May 02 2015, Alexey Dobriyan <adobriyan@gmail.com> 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.

  reply	other threads:[~2015-05-05 11:10 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-05-02  0:47 [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) Alexey Dobriyan
2015-05-02  0:48 ` [PATCH 02/10] parse_integer: rewrite kstrto*() Alexey Dobriyan
2015-05-02  0:50 ` [PATCH 03/10] parse_integer: convert sscanf() Alexey Dobriyan
2015-05-02  1:10   ` [PATCH CORRECT " Alexey Dobriyan
2015-05-02  0:51 ` [PATCH 04/10] sscanf: fix overflow Alexey Dobriyan
2015-05-05  9:51   ` Rasmus Villemoes
2015-05-05 11:10     ` Alexey Dobriyan [this message]
2015-05-06  7:49       ` Rasmus Villemoes
2015-05-02  0:53 ` [PATCH 05/10] parse_integer: convert lib/ Alexey Dobriyan
2015-05-04 14:10   ` Rasmus Villemoes
2015-05-04 14:57     ` Alexey Dobriyan
2015-05-02  0:55 ` [PATCH 06/10] parse_integer: convert mm/ Alexey Dobriyan
2015-05-04 14:33   ` Rasmus Villemoes
2015-05-04 15:09     ` Alexey Dobriyan
2015-05-02  0:56 ` [PATCH 07/10] parse_integer: convert misc fs/ code Alexey Dobriyan
2015-05-02  0:59 ` [PATCH 08/10] fs/cachefiles/: convert to parse_integer() Alexey Dobriyan
2015-05-02  1:01 ` [PATCH 09/10] ocfs2: convert to parse_integer()/kstrto*() Alexey Dobriyan
2015-05-02  1:03 ` [PATCH 10/10] ext2, ext3, ext4: " Alexey Dobriyan
2015-05-04 13:24 ` [PATCH 01/10] Add parse_integer() (replacement for simple_strto*()) Rasmus Villemoes
2015-05-04 14:32   ` Alexey Dobriyan
2015-05-04 16:44     ` Rasmus Villemoes
2015-05-04 19:54       ` Alexey Dobriyan
2015-05-04 21:48         ` Rasmus Villemoes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CACVxJT9yTO8-wRJ4Y_w=DguCiniyu_dP1BXg+KE1ME+PrabgrA@mail.gmail.com' \
    --to=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --subject='Re: [PATCH 04/10] sscanf: fix overflow' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).