273467 - -"-0x1" results in 1 instead of NaN

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Allen]

This evaluates as the negation of ToNumber("-0x01").  The grammar for ToNumber
does not permit hex integer literals to be signed.  Any string that can't be
interpreted by the grammar is supposed to result in a NaN.  IE follows the spec
and gives NaN.

This is difference between ToNumber and parseInt, which permits negative hex
literals.

Comment 1

[Nicholas Allen]

Attachment: the obvious test case

Comment 2

[Bob Clary [:bc] (inactive)]

-> default qa

Comment 3

[Saint Wesonga]

Attachment: Patch v1

Handle the above specified edge case

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 393466 [details] [diff] [review]
Patch v1

>diff --git a/js/src/jsnum.cpp b/js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp
>+++ b/js/src/jsnum.cpp
>@@ -883,16 +883,27 @@ js_ValueToNumber(JSContext *cx, jsval *v
>             /*
>              * Note that ECMA doesn't treat a string beginning with a '0' as
>              * an octal number here. This works because all such numbers will
>              * be interpreted as decimal by js_strtod and will never get
>              * passed to js_strtointeger (which would interpret them as
>              * octal).
>              */
>             JSSTRING_CHARS_AND_END(str, bp, end);

You are patching old source. Please pull from hg.mozilla.org/mozilla-central.

>+
>+            // ECMA doesn't allow negative HEX numbers (bug 273467)

Nits: hex not HEX (it's not an acronym), and full-stop punctuation (.) at end of comment.

>+            const jschar *s;

This variable isn't necessary, see further below.

>+            JSBool negative;
>+
>+            s = js_SkipWhiteSpace(bp, end);
>+            if (s != end && (negative = (*s == '-')) != 0 &&
>+                s + 1 != end && s[1] == '0' &&
>+                s + 2 != end && (s[2] == 'X' || s[2] == 'x'))
>+                   break;

There's no need for the single-use negative variable, nor for != 0 with a boolean.

Don't test redundantly: in order for a negative hex literal at least three characters must lie after the -, 0xN where N is a hex digit. So just test s + 3 < end and then match characters.

The break; is overindented by 3 columns.

The multiline condition should force bracing of the single-line break; consquent. See the style guide (https://wiki.mozilla.org/JavaScript:SpiderMonkey:C_Coding_Style).

>+
>             if ((!js_strtod(cx, bp, end, &ep, &d) ||
>                  js_SkipWhiteSpace(ep, end) != end) &&
>                 (!js_strtointeger(cx, bp, end, &ep, 0, &d) ||
>                  js_SkipWhiteSpace(ep, end) != end)) {

No point in starting at bp here now that s has been advanced. OTOH, no point in const jschar *s given bp. May as well have just bp and not add s.

>                 break;
>             }

Note style to match here, indentation and bracing.

/be

Comment 5

[Saint Wesonga]

Attachment: Patch v2

Isn't checking for bp + 2 < end sufficient (instead of bp + 3 < end) since no valid number can begin with -0x according to the grammar. This would also avoid the need to further match hex digits

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 393966 [details] [diff] [review]
Patch v2

>diff --git a/js/src/jsnum.cpp b/js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp
>+++ b/js/src/jsnum.cpp
>@@ -919,16 +919,24 @@ js_ValueToNumber(JSContext *cx, jsval *v
>             /*
>              * Note that ECMA doesn't treat a string beginning with a '0' as
>              * an octal number here. This works because all such numbers will
>              * be interpreted as decimal by js_strtod and will never get
>              * passed to js_strtointeger (which would interpret them as
>              * octal).
>              */
>             str->getCharsAndEnd(bp, end);
>+
>+            /* ECMA doesn't allow negative hex numbers (bug 273467). */
>+            bp = js_SkipWhiteSpace(bp, end);
>+            if ((bp + 2 < end) && (*bp == '-') && (bp[1] == '0') &&
>+                (bp[2] == 'X' || bp[2] == 'x')) {
>+                break;
>+            }
>+

Great -- only nit: don't overparenthesize equality or relational ops against && and ||. Whoever lands this, please fix it (new patch fixing it is helpful, gets carry-over r+).

/be

Comment 7

[Saint Wesonga]

Attachment: Additonal Testcase

-"+0x01" should be NaN as well

Comment 8

[Saint Wesonga]

Attachment: Patch v3

Not sure if this should be filed as another bug, but requesting review again for change to handle + sign as well - see additional test case.

Comment 9

[Saint Wesonga]

Attachment: Patch v4

Fixed comment in Patch v3

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 394490 [details] [diff] [review]
Patch v4

Good catch, I often forget about unary + as operator or in string-to-number. Patch looks great to land as is, thanks!

/be

Comment 11

[Jason Orendorff [:jorendorff]]

And here it is:

http://hg.mozilla.org/tracemonkey/rev/a2ab5097c5e5

(Note: This bug will be marked RESOLVED FIXED in a few days, once the change finds its way into the mozilla-central repository.)

Comment 12

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/fdcb3fdf8ec9
js/tests/ecma/TypeConversion/9.3.1-3.js updated.

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a2ab5097c5e5

Comment 14

[Bob Clary [:bc] (inactive)]

should status1.9.2 be beta1-fixed?

Comment 15

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d649035695a3

Comment 16

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d649035695a3

Comment 17

[Nochum Sossonko [:Natch]]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091008 Minefield/3.7a1pre
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b1pre) Gecko/20091008 Namoroka/3.6b1pre

V. Fixed based on the first testcase. I get NaN now.