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 01/10] Add parse_integer() (replacement for simple_strto*())
Date: Mon, 4 May 2015 22:54:35 +0300 [thread overview]
Message-ID: <20150504195435.GA21686@p183.telecom.by> (raw)
In-Reply-To: <87y4l4gumd.fsf@rasmusvillemoes.dk>
On Mon, May 04, 2015 at 06:44:42PM +0200, Rasmus Villemoes wrote:
> [I'm merging the subthreads below]
>
> On Mon, May 04 2015, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> > On Mon, May 4, 2015 at 4:24 PM, Rasmus Villemoes
> >> Is there any reason to disallow "-0"?
> >
> > No! -0 is not accepted because code is copied from kstrtoll()
> > which doesn't accept "-0". It is even in the testsuite:
> >
> > static void __init test_kstrtoll_fail(void)
> > {
> > ...
> > /* negative zero isn't an integer in Linux */
> > {"-0", 0},
> > {"-0", 8},
> > {"-0", 10},
> > {"-0", 16},
> >
> > Frankly I don't even remember why it does that, and
> > no one noticed until now. libc functions accept "-0".
>
> I think it's odd to accept "+0" but not "-0", but that's probably just
> because I'm a mathematician. Am I right that you just added these test
> cases because of the existing behaviour of kstrtoll? I suppose that
> behaviour is just a historical accident.
>
> If "-0" is not going to be accepted, I think that deserves a comment
> (with rationale) in the parsing code and not hidden away in the test
> suite.
Again, I honestly do not remember why "-0" was banned.
Let's change it to "+0 -0" for signed case, "+0" for unsigned case.
> >>> unsigned long long memparse(const char *ptr, char **retptr)
> >>> {
> >>> - char *endptr; /* local pointer to end of parsed string */
> >>> + unsigned long long val;
> >>>
> >>> - unsigned long long ret = simple_strtoull(ptr, &endptr, 0);
> >>> -
> >>> - switch (*endptr) {
> >>> + ptr += parse_integer(ptr, 0, &val);
> >>
> >> This seems wrong. simple_strtoull used to "sanitize" the return value
> >> from the (old) _parse_integer, so that endptr still points into the
> >> given string. Unconditionally adding the result from parse_integer may
> >> make ptr point far before the actual string, into who-knows-what.
> >
> > When converting I tried to preserve the amount of error checking done.
> > simple_strtoull() either
> > a) return 0 and not advance pointer, or
> > b) return something and advance pointer.
> >
>
> Are we talking about the same simple_strtoull? I see
>
> cp = _parse_integer_fixup_radix(cp, &base);
> rv = _parse_integer(cp, base, &result);
> /* FIXME */
> cp += (rv & ~KSTRTOX_OVERFLOW);
>
> so cp is definitely advanced even in case of overflow. And in the case
> of "underflow" (no digits found), the old code does initialize *result
> to 0, while parse_integer by design doesn't write anything.
>
> > Current code just ignores error case, so do I.
>
> There's a difference between ignoring an error (which the current code
> does), and ignoring _the possibility_ of an error (which the new code
> does).
>
> There are lots of callers of memparse(), and I don't think any of them
> are prepared to handle *endp ending up pointing before the passed-in
> string (-EINVAL == -22, -ERANGE == -34). I can easily see how that could
> lead to an infinite loop, maybe worse.
Yeah, possible bug could become worse, I'll add error checking,
but, seriously, you're defending this :^)
case Opt_nr_inodes:
===> /* memparse() will accept a K/M/G without a digit */
===> if (!isdigit(*args[0].from))
===> goto bad_val;
pconfig->nr_inodes = memparse(args[0].from, &rest);
break;
memparse() is misdesigned in the same sense strtoul() is misdesigned.
Every "memparse(s, NULL)" user is a bug for example.
next prev parent reply other threads:[~2015-05-04 19:54 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
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 [this message]
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=20150504195435.GA21686@p183.telecom.by \
--to=adobriyan@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).