552899 - JSString::initFlat JS_Assert length <= MAX_LENGTH

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[timeless]

bug 550676 comment 2
warnings and then eventually dies in:

#0  0x001a9610 in JS_Assert (s=0x2549ae "length <= MAX_LENGTH", file=0x23f320
"/Users/kinetik/work/mozilla-central/js/src/jsstr.h", ln=187) at
/Users/kinetik/work/mozilla-central/js/src/jsutil.cpp:73
#1  0x00095e0e in JSString::initFlat (this=0x127ec000, chars=0xc0000008,
length=335539830) at jsstr.h:187

It's unfortunate that kinetik didn't include a more complete stack trace, however, i'd like to propose a patch anyway. because I don't like where this api restriction is exposed, it's way too far away from callers. Our asserts should generally be exposed at the entry points (jsapi.cpp), if they were, then the second stack frame here would have been at least the jsapi that was being abused instead of some internal blob.

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Matthew Gregan [:kinetik]]

Assertion failure: length <= MAX_LENGTH, at /Users/kinetik/work/mozilla-central/js/src/jsstr.h:187

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x001a9610 in JS_Assert (s=0x2549ae "length <= MAX_LENGTH", file=0x23f320 "/Users/kinetik/work/mozilla-central/js/src/jsstr.h", ln=187) at /Users/kinetik/work/mozilla-central/js/src/jsutil.cpp:73
73	    *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  0x001a9610 in JS_Assert (s=0x2549ae "length <= MAX_LENGTH", file=0x23f320 "/Users/kinetik/work/mozilla-central/js/src/jsstr.h", ln=187) at /Users/kinetik/work/mozilla-central/js/src/jsutil.cpp:73
#1  0x00095e0e in JSString::initFlat (this=0x19c17000, chars=0x5c2af008, length=335541006) at jsstr.h:187
#2  0x00085f7f in JS_NewExternalString (cx=0x1b5ec00, chars=0x5c2af008, length=335541006, type=1) at /Users/kinetik/work/mozilla-central/js/src/jsapi.cpp:2476
#3  0x1204414c in XPCStringConvert::ReadableToJSVal (cx=0x1b5ec00, readable=@0xbfffc698) at /Users/kinetik/work/mozilla-central/js/src/xpconnect/src/xpcstring.cpp:108
#4  0x1202d3b4 in XPCConvert::NativeData2JS (lccx=@0xbfffc278, d=0xbfffc530, s=0xbfffc414, type=@0xbfffc4bc, iid=0xbfffc72c, scope=0x2142f400, pErr=0xbfffc5b4) at /Users/kinetik/work/mozilla-central/js/src/xpconnect/src/xpcconvert.cpp:333
#5  0x12032abb in XPCConvert::NativeData2JS (ccx=@0xbfffc7ec, d=0xbfffc530, s=0xbfffc414, type=@0xbfffc4bc, iid=0xbfffc72c, scope=0x2142f400, pErr=0xbfffc5b4) at xpcprivate.h:3023
#6  0x1205e9a1 in XPCWrappedNative::CallMethod (ccx=@0xbfffc7ec, mode=XPCWrappedNative::CALL_GETTER) at /Users/kinetik/work/mozilla-central/js/src/xpconnect/src/xpcwrappednative.cpp:2818
#7  0x1206cadf in XPCWrappedNative::GetAttribute (ccx=@0xbfffc7ec) at xpcprivate.h:2573
#8  0x12065d69 in XPC_WN_GetterSetter (cx=0x1b5ec00, obj=0x2142f400, argc=0, argv=0x1add26c, vp=0xbfffc8f0) at /Users/kinetik/work/mozilla-central/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1814
#9  0x00114abc in js_Invoke (cx=0x1b5ec00, argc=0, vp=0x1add264, flags=2) at jsinterp.cpp:1370
#10 0x001150d3 in js_InternalInvoke (cx=0x1b5ec00, obj=0x2142f400, fval=601445760, flags=0, argc=0, argv=0x0, rval=0xbfffd004) at jsinterp.cpp:1435
#11 0x001151ec in js_InternalGetOrSet (cx=0x1b5ec00, obj=0x2142f400, id=356260196, fval=601445760, mode=JSACC_READ, argc=0, argv=0x0, rval=0xbfffd004) at jsinterp.cpp:1472
#12 0x001358ac in JSScopeProperty::get (this=0x13b1230, cx=0x1b5ec00, obj=0x2142f400, pobj=0x214308e0, vp=0xbfffd004) at jsscope.h:882
#13 0x0012a1ba in js_NativeGet (cx=0x1b5ec00, obj=0x2142f400, pobj=0x214308e0, sprop=0x13b1230, getHow=1, vp=0xbfffd004) at /Users/kinetik/work/mozilla-central/js/src/jsobj.cpp:4998
#14 0x0012a7d1 in js_GetPropertyHelper (cx=0x1b5ec00, obj=0x2142f400, id=356260196, getHow=1, vp=0xbfffd004) at /Users/kinetik/work/mozilla-central/js/src/jsobj.cpp:5174
#15 0x000fc538 in js_Interpret (cx=0x1b5ec00) at jsops.cpp:1521
#16 0x00114b05 in js_Invoke (cx=0x1b5ec00, argc=1, vp=0x1add220, flags=0) at jsinterp.cpp:1378
#17 0x12050c75 in nsXPCWrappedJSClass::CallMethod (this=0x159f3890, wrapper=0x1a12d0e0, methodIndex=3, info=0x18f3db8, nativeParams=0xbfffd5a0) at /Users/kinetik/work/mozilla-central/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1696
#18 0x12047613 in nsXPCWrappedJS::CallMethod (this=0x1a12d0e0, methodIndex=3, info=0x18f3db8, params=0xbfffd5a0) at /Users/kinetik/work/mozilla-central/js/src/xpconnect/src/xpcwrappedjs.cpp:570
#19 0x0041a540 in PrepareAndDispatch (self=0x1a16fcb0, methodIndex=3, args=0xbfffd6c4) at /Users/kinetik/work/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_unixish_x86.cpp:93
#20 0x00415083 in nsXPTCStubBase::Stub3 (this=0x1a16fcb0) at xptcstubsdef.inc:1
#21 0x1381ff4f in nsDOMEventListenerWrapper::HandleEvent (this=0x1a146210, aEvent=0x2383c4a0) at /Users/kinetik/work/mozilla-central/content/events/src/nsDOMEventTargetHelper.cpp:65
#22 0x137dcd37 in nsEventListenerManager::HandleEventSubType (this=0x1a1305c0, aListenerStruct=0x1a1305e8, aListener=0x1a146210, aDOMEvent=0x2383c4a0, aCurrentTarget=0x1a130500, aPhaseFlags=6, aPusher=0xbfffdaa8) at /Users/kinetik/work/mozilla-central/content/events/src/nsEventListenerManager.cpp:1082
#23 0x137dd202 in nsEventListenerManager::HandleEvent (this=0x1a1305c0, aPresContext=0x0, aEvent=0x2383c4f0, aDOMEvent=0xbfffda98, aCurrentTarget=0x1a130500, aFlags=6, aEventStatus=0xbfffda9c, aPusher=0xbfffdaa8) at /Users/kinetik/work/mozilla-central/content/events/src/nsEventListenerManager.cpp:1198
#24 0x1380e0b5 in nsEventTargetChainItem::HandleEvent (this=0x19ca900, aVisitor=@0xbfffda90, aFlags=6, aMayHaveNewListenerManagers=0, aPusher=0xbfffdaa8) at /Users/kinetik/work/mozilla-central/content/events/src/nsEventDispatcher.cpp:201
#25 0x1380c9de in nsEventTargetChainItem::HandleEventTargetChain (this=0x19ca900, aVisitor=@0xbfffda90, aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=0xbfffdaa8) at /Users/kinetik/work/mozilla-central/content/events/src/nsEventDispatcher.cpp:326
#26 0x1380d607 in nsEventDispatcher::Dispatch (aTarget=0x1a130500, aPresContext=0x0, aEvent=0x2383c4f0, aDOMEvent=0x2383c4a0, aEventStatus=0x0, aCallback=0x0, aTargets=0x0) at /Users/kinetik/work/mozilla-central/content/events/src/nsEventDispatcher.cpp:601
#27 0x1380d981 in nsEventDispatcher::DispatchDOMEvent (aTarget=0x1a130500, aEvent=0x0, aDOMEvent=0x2383c4a0, aPresContext=0x0, aEventStatus=0x0) at /Users/kinetik/work/mozilla-central/content/events/src/nsEventDispatcher.cpp:664
#28 0x1382045d in nsDOMEventTargetHelper::DispatchDOMEvent (this=0x1a130500, aEvent=0x0, aDOMEvent=0x2383c4a0, aPresContext=0x0, aEventStatus=0x0) at /Users/kinetik/work/mozilla-central/content/events/src/nsDOMEventTargetHelper.cpp:229
#29 0x136f8245 in nsDOMFileReader::DispatchProgressEvent (this=0x1a130500, aType=@0xbfffdc70) at /Users/kinetik/work/mozilla-central/content/base/src/nsDOMFileReader.cpp:574
#30 0x136fa20a in nsDOMFileReader::OnStopRequest (this=0x1a130500, aRequest=0x1a130810, aContext=0x0, aStatus=0) at /Users/kinetik/work/mozilla-central/content/base/src/nsDOMFileReader.cpp:473
#31 0x1229dd49 in nsBaseChannel::OnStopRequest (this=0x1a1307e0, request=0x1a130a00, ctxt=0x0, status=0) at /Users/kinetik/work/mozilla-central/netwerk/base/src/nsBaseChannel.cpp:680
#32 0x122b1cd0 in nsInputStreamPump::OnStateStop (this=0x1a130a00) at /Users/kinetik/work/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:576
#33 0x122b1dfa in nsInputStreamPump::OnInputStreamReady (this=0x1a130a00, stream=0x1a130afc) at /Users/kinetik/work/mozilla-central/netwerk/base/src/nsInputStreamPump.cpp:401
#34 0x003c0914 in nsInputStreamReadyEvent::Run (this=0x2375ef90) at /Users/kinetik/work/mozilla-central/xpcom/io/nsStreamUtils.cpp:112
#35 0x003f5938 in nsThread::ProcessNextEvent (this=0x909d20, mayWait=0, result=0xbfffde34) at /Users/kinetik/work/mozilla-central/xpcom/threads/nsThread.cpp:527

Comment 3

[Igor Bukanov]

(In reply to comment #1)
> Created an attachment (id=433008) [details]
> proposal

For now I suggests just to add the asserts without penalizing the optimized builds with any extra checks and consider enabling the check in another bug. Also it is better to use a separated macro to do the check so it would be trivial to change its non-debug semantics later.

Comment 4

[Igor Bukanov]

Comment on attachment 433008 [details] [diff] [review]
proposal

A new patch is necessary per comments above.

Comment 5

[Jason Orendorff [:jorendorff]]

The story so far: Many JSAPI functions, including JS_NewString, can assert due to an undocumented restriction on string length. XPConnect can actually trigger the assertion in practice.

Clearly this is bad. Possible fixes:

1. Document the restriction for all affected functions, expose the limit as a constant in jsapi.h, enforce it with assertions at the API boundary (that is, in jsapi.cpp and jsdbgapi.cpp), audit our existing JSAPI-using code to see that those functions are used correctly, and warn about this on the m.d.t.js-engine newsgroup.

2. Add run-time checks at the API boundary.

3. Change the implementation to remove the restriction. (We may not really need it anymore. JSString no longer uses bitfields.)

Either attempting to create huge strings is valid API use or it isn't; so we should either check or assert, not both.

This bug needs an owner.

Comment 6

[Jason Orendorff [:jorendorff]]

A few comments on the patch:

namelen can be size_t(-1); the check needs to allow that.

Checking only the UC APIs isn't enough. The char-based js_Atomize ends up JSString::flatInit too.

Comment 7

[Igor Bukanov]

(In reply to comment #5)
> 3. Change the implementation to remove the restriction. (We may not really need
> it anymore. JSString no longer uses bitfields.)

The restriction prevents some trivial DOS that would otherwise exists. Also it makes spraying heap slightly more difficult. For this reason I would even suggest to set the limit to the lowest value possible given the web compatibility. But that clearly requires runtime checks.

Comment 8

[Robert Sayre]

Igor, can you take this?

Comment 9

[Luke Wagner [:luke]]

All *String::new_ functions now contain validateLength check.