[PATCH] Fix %x parsing in vsscanf()

[PATCH] Fix %x parsing in vsscanf()

[Deepak Saxena]

The existing code in kernel/vsprintf.c:vsscanf() does not properly 
handle the case where the format is specfied as %x or %X and the
string contains the number in the format "0xinteger". Instead of
reading "0xinteger", the code currently only sees the '0' and treats
the 'x' as a delimiter. Following patch (against 2.4 and 2.6) fixes
this.  Another option is to put the check in simple_strtoul() and
simple_strtoull() if that is preferred. I like this better b/c
we only have the check once.

Please apply,
~Deepak

===== lib/vsprintf.c 1.2 vs edited =====
--- 1.2/lib/vsprintf.c	Mon Aug 11 04:54:01 2003
+++ edited/lib/vsprintf.c	Tue Sep 23 13:50:50 2003
@@ -615,6 +615,8 @@
 		case 'x':
 		case 'X':
 			base = 16;
+			if(str[0] == '0' && (str[1] == 'x' || str[1] == 'X'))
+				str += 2;
 			break;
 		case 'd':
 		case 'i':


-- 
Deepak Saxena
MontaVista Software - Powering the Embedded Revolution - www.mvista.com

Re: [PATCH] Fix %x parsing in vsscanf()

[viro]

On Tue, Sep 23, 2003 at 02:22:07PM -0700, Deepak Saxena wrote:
> 
> The existing code in kernel/vsprintf.c:vsscanf() does not properly 
> handle the case where the format is specfied as %x or %X and the
> string contains the number in the format "0xinteger". Instead of
> reading "0xinteger", the code currently only sees the '0' and treats
> the 'x' as a delimiter. Following patch (against 2.4 and 2.6) fixes
> this.  Another option is to put the check in simple_strtoul() and
> simple_strtoull() if that is preferred. I like this better b/c
> we only have the check once.
 
	We should put that into strtoul().  Rationale:

<quote>
If the value of base is 16, the characters 0x or 0X may optionally precede
the sequence of letters and digits, following the sign if present.
</quote>

That's from C99 7.20.1.4 (definition of strtoul and friends) and it does
match the normal userland behaviour of strtoul(3) on all platforms I'd
seen...

Re: [PATCH] Fix %x parsing in vsscanf()

[Linus Torvalds]

On Tue, 23 Sep 2003, Deepak Saxena wrote:
> 
> Another option is to put the check in simple_strtoul() and
> simple_strtoull() if that is preferred. I like this better b/c
> we only have the check once.

I'd much rather fix strtoul[l](). In fact, strtoul[l]()  _already_ accepts
the "0x" prefix - but it only does so if the "base" parameter is 0.

Fixing strtoul[l] should fix vsscanf() automatically, no? So I don't see 
the "have the check only once" argument.

		Linus

Re: [PATCH] Fix %x parsing in vsscanf()

[viro]

On Tue, Sep 23, 2003 at 02:28:12PM -0700, Linus Torvalds wrote:
> 
> On Tue, 23 Sep 2003, Deepak Saxena wrote:
> > 
> > Another option is to put the check in simple_strtoul() and
> > simple_strtoull() if that is preferred. I like this better b/c
> > we only have the check once.
> 
> I'd much rather fix strtoul[l](). In fact, strtoul[l]()  _already_ accepts
> the "0x" prefix - but it only does so if the "base" parameter is 0.
> 
> Fixing strtoul[l] should fix vsscanf() automatically, no? So I don't see 
> the "have the check only once" argument.

C99 on behaviour of %x:

"Matches an optionally signed hexadecimal integer, whose format is the same as
expected for the subject sequence of the strtoul function with the value 16
for the base argument."

IOW, strtoul() is definitely the right place to fix that.

Re: [PATCH] Fix %x parsing in vsscanf()

[Deepak Saxena]

On Sep 23 2003, at 22:35, viro@parcelfarce.linux.theplanet.co.uk was caught saying:
> On Tue, Sep 23, 2003 at 02:28:12PM -0700, Linus Torvalds wrote:
> > Fixing strtoul[l] should fix vsscanf() automatically, no? So I don't see 
> > the "have the check only once" argument.
> 
> C99 on behaviour of %x:
> 
> "Matches an optionally signed hexadecimal integer, whose format is the same as
> expected for the subject sequence of the strtoul function with the value 16
> for the base argument."
> 
> IOW, strtoul() is definitely the right place to fix that.

OK, given that and your previous message, here's an updated patch:

===== lib/vsprintf.c 1.1 vs edited =====
--- 1.1/lib/vsprintf.c	Tue Jan 22 19:31:16 2002
+++ edited/lib/vsprintf.c	Tue Sep 23 15:07:20 2003
@@ -32,6 +32,8 @@
 {
 	unsigned long result = 0,value;
 
+	if ((base == 16) && (cp[0] == '0' && (cp[1] == 'x' || cp[1] == 'X')))
+		cp += 2;
 	if (!base) {
 		base = 10;
 		if (*cp == '0') {
@@ -76,6 +78,8 @@
 {
 	unsigned long long result = 0,value;
 
+	if ((base == 16) && (cp[0] == '0' && (cp[1] == 'x' || cp[1] == 'X')))
+		cp += 2;
 	if (!base) {
 		base = 10;
 		if (*cp == '0') {

-- 
Deepak Saxena
MontaVista Software - Powering the Embedded Revolution - www.mvista.com

Re: [PATCH] Fix %x parsing in vsscanf()

[viro]

On Tue, Sep 23, 2003 at 03:16:11PM -0700, Deepak Saxena wrote:
>  {
>  	unsigned long result = 0,value;
>  
> +	if ((base == 16) && (cp[0] == '0' && (cp[1] == 'x' || cp[1] == 'X')))
> +		cp += 2;
>  	if (!base) {
>  		base = 10;
>  		if (*cp == '0') {

Not quite right.
	a) on "0xZ" correct reaction is to eat '0' and stop.
	b) while we are at it, might as well fix the case of 0X<...> with
base 0.

The following, AFAICS, would be correct:

        if (*cp == '0') {
                cp++;
                if (unlikely((*cp == 'x' || *cp == 'X') && isxdigit(cp[1]))) {
                        if (!base || base == 16) {
                                cp++;
                                base = 16;
                        }
                } else if (!base)
                        base = 8;
        } else if (!base)
                base = 10;

Re: [PATCH] Fix %x parsing in vsscanf()

[Deepak Saxena]

On Sep 23 2003, at 23:26, viro@parcelfarce.linux.theplanet.co.uk was caught saying:
> The following, AFAICS, would be correct:
> 
>         if (*cp == '0') {
>                 cp++;
>                 if (unlikely((*cp == 'x' || *cp == 'X') && isxdigit(cp[1]))) {
>                         if (!base || base == 16) {
>                                 cp++;
>                                 base = 16;
>                         }
>                 } else if (!base)
>                         base = 8;
>         } else if (!base)
>                 base = 10;

We can remove everything but "base = 10;" from the second "else if"
clause b/c by this point we're guaranteed that it's not a hex or
octal value.

===== lib/vsprintf.c 1.16 vs edited =====
--- 1.16/lib/vsprintf.c	Sat Jul 12 12:49:57 2003
+++ edited/lib/vsprintf.c	Tue Sep 23 17:56:50 2003
@@ -32,16 +32,17 @@
 {
 	unsigned long result = 0,value;
 
-	if (!base) {
+	if (*cp == '0') {
+	       	cp++;
+	       	if (unlikely((*cp == 'x' || *cp == 'X') && isxdigit(cp[1]))) {
+		       	if (!base || base == 16) {
+			       	cp++;
+			       	base = 16;
+		       	}
+	       	} else if (!base)
+		       	base = 8;
+	} else if (!base) {
 		base = 10;
-		if (*cp == '0') {
-			base = 8;
-			cp++;
-			if ((*cp == 'x') && isxdigit(cp[1])) {
-				cp++;
-				base = 16;
-			}
-		}
 	}
 	while (isxdigit(*cp) &&
 	       (value = isdigit(*cp) ? *cp-'0' : toupper(*cp)-'A'+10) < base) {
@@ -76,16 +77,17 @@
 {
 	unsigned long long result = 0,value;
 
-	if (!base) {
+	if (*cp == '0') {
+	       	cp++;
+	       	if (unlikely((*cp == 'x' || *cp == 'X') && isxdigit(cp[1]))) {
+		       	if (!base || base == 16) {
+			       	cp++;
+			       	base = 16;
+		       	}
+	       	} else if (!base)
+		       	base = 8;
+	} else if (!base) {
 		base = 10;
-		if (*cp == '0') {
-			base = 8;
-			cp++;
-			if ((*cp == 'x') && isxdigit(cp[1])) {
-				cp++;
-				base = 16;
-			}
-		}
 	}
 	while (isxdigit(*cp) && (value = isdigit(*cp) ? *cp-'0' : (islower(*cp)
 	    ? toupper(*cp) : *cp)-'A'+10) < base) {



-- 
Deepak Saxena
MontaVista Software - Powering the Embedded Revolution - www.mvista.com

Re: [PATCH] Fix %x parsing in vsscanf()

[viro]

On Tue, Sep 23, 2003 at 06:02:58PM -0700, Deepak Saxena wrote:
> On Sep 23 2003, at 23:26, viro@parcelfarce.linux.theplanet.co.uk was caught saying:
> > The following, AFAICS, would be correct:
> > 
> >         if (*cp == '0') {
> >                 cp++;
> >                 if (unlikely((*cp == 'x' || *cp == 'X') && isxdigit(cp[1]))) {
> >                         if (!base || base == 16) {
> >                                 cp++;
> >                                 base = 16;
> >                         }
> >                 } else if (!base)
> >                         base = 8;
> >         } else if (!base)
> >                 base = 10;
> 
> We can remove everything but "base = 10;" from the second "else if"
> clause b/c by this point we're guaranteed that it's not a hex or
> octal value.

Bzzert.  strtoul() with e.g. base 8 on "1234" should parse it as octal.