503402 - Assertion failure: "STRING_BUFFER_OK(sb)" - Negative values for space (third) param to JSON.stringify causes 3.5 to hang [@ js_json_stringify]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Luke Smith]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.11) Gecko/2009060214 Firefox/3.0.11

Calling e.g. JSON.stringify({ x: 1 }, null, -4) is at least stopping the js processing in the current threat.  Possibly hanging the page.  No error occurs, but processing stops.

Reproducible: Always

Steps to Reproduce:
1. document.body.innerHTML = JSON.stringify({x:1},null,-4);

Actual Results:  
A whole lotta nothing

Expected Results:  
The body contains only
{"x":1}

since the ECMA 5 spec for JSON says only values between 0 and 100 (possibly less than 100--I heard of an update but haven't investigated) are supported.  Anything else is treated as empty string.

This was discovered by the test suite I built for YUI when adding native browser support to the JSON utility.  I abstracted the test suite out to just exercise native implementations.  It is available here for your reference:
http://lucassmith.name/pub/browser_bugs/ff/native_json.html

It may come in handy for your tests.  If there are tests in there that misinterpret the spec, I'd love to know.

Comment 1

[John P Baker]

javascript:alert(JSON.stringify({x:1},null,-4))

crashes for me (unfortunately I've never been able to submit reports)

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090709 Minefield/3.6a1pre ID:20090709091211

Comment 2

[Andreas Gal :gal]

I don't know the code in question but this smell sufficiently like a buffer overrun to hide it. Sayrer, can you take a look and unhide if its harmless?

Comment 3

[Jeff Walden [:Waldo]]

ToUint32(-4) => bignum, that goes to js_RepeatChar, ENSURE_STRING_BUFFER(sb, bignum) includes an integer overflow then goes into GrowStringBuffer with bignum, which doesn't check for integer overflows at all (note newlength += sb->ptr - sb->base + 1, immediate wraparound if space=-1), and I think at that point it's an exercise for the reader.  Unclear at first glance where the overflow check(s, most likely) need to be added...

This is also a spec bug, as my candidate-*.pdf doesn't really deal with negative values (it has verbiage that if space < 1 it's an empty string, but that seems more explanatory than prescriptive, and it's preceded by "Set gap to a string containing |space| space characters").

Also note: JSON.stringify({}, null, -s) asserts for any s greater than 1 (1 crashes) in shell.

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

Sorry about the old trap here, into which json.cpp walked with my r+.

/be

Comment 5

[Brendan Eich [:brendan]]

There's a bug in json.cpp still, but I will let sayrer file and fix it. ES5 draft in 15.12.3 stringify ( value [ , replacer [ , space ] ] ) says

4.6. If Type(space) is number
     a. Let space be min(10, ToInteger(space))the lesser of space and 100.

number is double and ToInteger converts the double to an integral value but still an IEEE double. In no case should space be JS_ValueToECMAUint32 be called without the above min() being computed.

/be

Comment 6

[Brendan Eich [:brendan]]

Sorry, left out 4.6(b) of 15.12.3:

     b. Set gap to a string containing space space characters. This will be the empty string if space is less than 1.

This is from the latest draft with edits, found at

http://wiki.ecmascript.org/doku.php?id=es3.1:es3.1_proposal_working_draft

/be

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> There's a bug in json.cpp still, but I will let sayrer file

Had a minute, so I filed bug 503524.

/be

Comment 8

[Samuel Sidler (old account; do not CC)]

We need to fix this in 1.9.1.1.

Comment 9

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/3ac0a33eec85

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

JSON.stringify({x:1},null,-4)


A (hacked) autoBisect shows this is probably related to bug 442059 :

The first bad revision is:
changeset:   28053:274140a44a2d
user:        Robert Sayre
date:        Thu May 07 13:28:21 2009 -0700
summary:     Bug 442059 -  [native JSON] allow to blacklist keys by name when encoding to JSON. r=brendan

Comment 11

[Samuel Sidler (old account; do not CC)]

Can we get this on mozilla-central and an approval request for 1.9.1.1?

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3ac0a33eec85

Comment 13

[Robert Sayre]

Attachment: branch patch

only difference is PTRDIFF macro on the branch.

Comment 14

[Samuel Sidler (old account; do not CC)]

Comment on attachment 388499 [details] [diff] [review]
branch patch

Approved for 1.9.1.1. a=ss

Comment 15

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4c0647472a57

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Why don't I get an alert box on 1.9.1 but on trunk? Tested on all platforms with a build like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090715 Shiretoko/3.5.1pre (.NET CLR 3.5.30729) ID:20090715044728

Comment 17

[Robert Sayre]

(In reply to comment #16)
> Why don't I get an alert box on 1.9.1 but on trunk? Tested on all platforms
> with a build like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
> rv:1.9.1.1pre) Gecko/20090715 Shiretoko/3.5.1pre (.NET CLR 3.5.30729)
> ID:20090715044728

I don't know what this comment means.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I tried the test mentioned in comment 1:
javascript:alert(JSON.stringify({x:1},null,-4))

In Minefield I get an alert box but not on 1.9.1.

Comment 19

[Robert Sayre]

(In reply to comment #18)
> I tried the test mentioned in comment 1:
> javascript:alert(JSON.stringify({x:1},null,-4))
> 
> In Minefield I get an alert box but not on 1.9.1.

I think it is because bug 503524 is not yet fixed on branch.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob/Whimboo: can you verify that this is fixed in latest-mozilla1.9.1 nightly or better yet the 3.5.1 release candidate: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/3.5.1-candidates/build1/

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on 1.9.1 with the debug build 20090716100323 on OS X. No assertion or crash is visible.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with the debug build 20090716110626 on OS X. 

Now that I was able to checked the fix with a fresh trunk debug build too I see a difference to 1.9.1. No idea if this is related to bug 503524.

Running the testcase in Shiretoko I see:

js> JSON.stringify({x:1},null,-4)
typein:1: out of memory

Comment 23

[Brendan Eich [:brendan]]

(In reply to comment #22)
> Verified fixed with the debug build 20090716110626 on OS X. 
> 
> Now that I was able to checked the fix with a fresh trunk debug build too I see
> a difference to 1.9.1. No idea if this is related to bug 503524.

Yes, the fix for bug 503524 makes m-c and tracemonkey builds clamp the third argument to be in [1,10] per draft ES5. The OOM in 1.9.1 branch builds is not per spec but it beats a crash!

/be

Comment 24

[Josh Aas]

This was fixed a long time ago, can we open this up?