Re: [PATCH] Lib:The patch include fix for "-" during string to long conversion

Re: [PATCH] Lib:The patch include fix for "-" during string to long conversion

[George Spelvin]

Namit Gupta wrote:
> @@ -91,10 +98,18 @@ EXPORT_SYMBOL(simple_strtol);
>   */
>  long long simple_strtoll(const char *cp, char **endp, unsigned int base)
>  {
> 
>      long l;
>  
>      if (cp[0] != '-')
>          return simple_strtoull(cp, endp, base);
>      l = -simple_strtoull(cp+1, endp, base);

In addition to the somewhat ugly the leading blank line,
I'd call this not very well tested.

Re: [PATCH] Lib:The patch include fix for "-" during string to long conversion

[Jan Beulich]

>>> On 04.11.12 at 14:26, Namit Gupta <namit2010@gmail.com> wrote:
> API simple_strtol() and simple_strtoul() are giving incorrect result for
> string "-".
> These API's consider "-" as a '-' (negative integer) and return incorrect
> string pointer.
> The API returns pointer next to '-' character. However it should return
> starting pointer of the string.

Where is that behavior specified? If we take the C standard as
reference, it is not being made explicit whether, for "base" other
than zero, the sequence of letters and digits has to be non-empty.

If we take this as implied, then your patch should not only handle
"-" alone, but also cases where "-" is followed by other than a
letter or digit, or an out of range one.

Jan

> Below I have included possible solution for that issue.
> Please review.
> 
> 
> From eea8b5fd7e5bb7b206a41f5eec934152a77a9431 Mon Sep 17 00:00:00 2001
> From: Namit Gupta <namit2010@gmail.com>
> Date: Sun, 4 Nov 2012 23:36:23 +0530
> Subject: [PATCH 1/1] Lib:The patch include fix for "-" during string to
> long conversion
> 
> API simple_strtol and simple_strtoll give wrong result for string "-",
> The API consider "-" as a -ve number which and give incorrect result
> for "-" string.
> 
> Signed-off-by: Namit Gupta <namit2010@gmail.com>
> ---
>  lib/vsprintf.c |   20 ++++++++++++++++----
>  1 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 39c99fe..cde449d 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -76,8 +76,14 @@ EXPORT_SYMBOL(simple_strtoul);
>   */
>  long simple_strtol(const char *cp, char **endp, unsigned int base)
>  {
> -    if (*cp == '-')
> -        return -simple_strtoul(cp + 1, endp, base);
> +    if (*cp == '-') {
> +        if (*(cp+1))
> +            return -simple_strtoul(cp + 1, endp, base);
> +        else if (*endp) { /* The string contains only "-"*/
> +            *endp = (char *)cp;
> +            return 0;
> +        }
> +    }
> 
>      return simple_strtoul(cp, endp, base);
>  }
> @@ -91,8 +97,14 @@ EXPORT_SYMBOL(simple_strtol);
>   */
>  long long simple_strtoll(const char *cp, char **endp, unsigned int base)
>  {
> -    if (*cp == '-')
> -        return -simple_strtoull(cp + 1, endp, base);
> +    if (*cp == '-') {
> +        if (*(cp+1))
> +            return -simple_strtoull(cp + 1, endp, base);
> +        else if (*endp) { /* The string contains only "-"*/
> +            *endp = (char *)cp;
> +            return 0;
> +        }
> +    }
> 
>      return simple_strtoull(cp, endp, base);
>  }
> -- 
> 1.7.1
> 
> 
> Regards,
> Namit

Re: [PATCH] Lib:The patch include fix for "-" during string to long conversion

[George Spelvin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3317 bytes --]

Namit Gupta <namit2010@gmail.com> wrote:
> API simple_strtol() and simple_strtoul() are giving incorrect result for
> string "-".
> These API's consider "-" as a '-' (negative integer) and return incorrect
> string pointer.
> The API returns pointer next to '-' character. However it should return
> starting pointer of the string.
> Below I have included possible solution for that issue.
> Please review.

tl;dr: Interesting issue.  Arguably, but not toally clear that, this is
a bug.  However, the proposed fix is broken; better fix below.


Just to clarify, the issue is that when fed the string "-", simple_strtol
is setting endp to cp+1 (and returning 0).  I don't think there's actually
anything wrong with simple_strtoul; did you mean simple_strtoull()?

glibc's strtol, for example, returns 0 digits converted when fed strings
that consist of a valid prefix (whitespace plus optional sign) if not
followed by valid digits.  (I just checked.)

If you want to follow C89 exactly (even though, by not permitting leading
whitespace or + signs, simple_strtol() obviously *doesn't*), returning
*endp = cp is required.  The question is what simple_strtol *should* do.

C89 talks about breaking the input into leading whitespace, a "subject
sequence" of characters beginning with the first non-whitespace, and a
final string of unrecognized characters.

Then C89 defines the "expected form" of the subject sequence as "that
of an integer constant as described in §3.1.3.2, optionally preceded by
a plus or minus sign, but not including an integer suffix."  The grammar
in the given section does not permit 0-length strings.

(The above is if base=0; if base != 0, the validity of 0-length
strings is not clear.)

Finally, we get to:
#    If the subject sequence is empty or does not have the expected
# form, no conversion is performed; the value of nptr is stored in the
# object pointed to by endptr, provided that endptr is not a null
# pointer.

Thus, since "-" is not of the expected form, *endp = cp is required.


The question is whether a "simple_" variant should be expected to do
lookahead like this, or if it should just do what the strtol man page
says: "strtol() stores the address of the first invalid character in
*endptr."

An important point is that lookahead already exists in the handling of
"0x" prefixes.  "0x0" through "0xf" are valid hex numbers, but "0xg"
and the like are 1-digit numbers followed by an unparsed string.
(See _parse_integer_fixup_radix() in lib/kstrtox.c.)


Independent of the above discussion, the proposed fix is definitely
not okay.  If you're going to consider this a bug, fix it properly
and also return 0 for "-z" as well; don't special-case the NUL byte.

It's awkward to do by pre-validating cp[1], because the range of valid
digits depends on the base.  A simpler fix would let simple_strotul make
that determination:

long simple_strtol(const char *cp, char **endp, unsigned int base)
{
	long l;

	if (cp[0] != '-')
		return simple_strtoul(cp, endp, base);
	l = -simple_strtoul(cp+1, endp, base);
	/* Return 0 digits converted if - sign is followed by 0 valid digits */
	if (endp && *endp == cp+1)
		*endp = cp;
	return l;
}

(Signed-off-by: George Spelvin <linux@horizon.com> if anyone wants to
copy this logic to simple_strtoll and turn it into a proper patch.)