Closed
Bug 1355046
Opened 4 years ago
Closed 4 years ago
Assertion failure: ptrdiff_t(column) + colspan >= 0, at js/src/jsscript.cpp:3102
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: decoder, Assigned: shu)
References
Details
(6 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main55-][post-critsmash-triage])
Attachments
(1 file)
1.31 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 2a3ecdb7d1ea (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --ion-offthread-compile=off): var localstr = ""; for (var i = 0; i < 0xFFFC; ++i) localstr += ('\f') + i + "; "; var arg = "x"; var body = localstr + "for (var i = 0; i < 4; ++i) arr[i](x-1);"; (new Function(arg, body))(1000); Backtrace: received signal SIGSEGV, Segmentation fault. 0x08601271 in js::PCToLineNumber (startLine=<optimized out>, notes=0xf4786053 "\220\220\210\001\210\005\210\t\210\r\210\021\210\025\210\031\210\035\210!\210%\210)\210.\210\063\210\070\210=\210B\210G\210L\210Q\210V\210[\210`\210e\210j\210o\210t\210y\210~\210\200", code=0xf4786014 ">W", pc=0xf4786021 "\232", columnp=0xffffb798) at js/src/jsscript.cpp:3102 #0 0x08601271 in js::PCToLineNumber (startLine=<optimized out>, notes=0xf4786053 "\220\220\210\001\210\005\210\t\210\r\210\021\210\025\210\031\210\035\210!\210%\210)\210.\210\063\210\070\210=\210B\210G\210L\210Q\210V\210[\210`\210e\210j\210o\210t\210y\210~\210\200", code=0xf4786014 ">W", pc=0xf4786021 "\232", columnp=0xffffb798) at js/src/jsscript.cpp:3102 #1 0x086012c8 in js::PCToLineNumber (script=0xf55700f8, pc=0xf4786021 "\232", columnp=0xffffb798) at js/src/jsscript.cpp:3120 #2 0x088001c7 in js::FrameIter::computeLine (this=0xffffb570, column=0xffffb798) at js/src/vm/Stack.cpp:896 #3 0x085582f8 in PopulateReportBlame (cx=cx@entry=0xf791d000, report=report@entry=0xffffb78c) at js/src/jscntxt.cpp:314 #4 0x08570608 in js::ReportErrorNumberVA (cx=0xf791d000, cx@entry=0xf7900000, flags=flags@entry=0, callback=0x8549bc0 <js::GetErrorMessage(void*, unsigned int)>, callback@entry=0x39d37a00, userRef=0x0, errorNumber=1, ap=0xffffb820 "\224\200\225\367\364\377\321\bh\270\377\377{\200Y\bL\270\377\377", argumentsType=js::ArgumentsAreLatin1) at js/src/jscntxt.cpp:883 #5 0x08570915 in JS_ReportErrorNumberLatin1VA (cx=0x0, errorCallback=0x1, userRef=0xffffb820, errorNumber=1, ap=0xffffb820 "\224\200\225\367\364\377\321\bh\270\377\377{\200Y\bL\270\377\377") at js/src/jsapi.cpp:5784 #6 0x08570947 in JS_ReportErrorNumberLatin1 (cx=0xf791d000, errorCallback=0x8549bc0 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=1) at js/src/jsapi.cpp:5773 #7 0x08570c0f in js::ReportIsNotDefined (cx=0xf791d000, id=...) at js/src/jscntxt.cpp:951 #8 0x08570c98 in js::ReportIsNotDefined (cx=0xf791d000, name=...) at js/src/jscntxt.cpp:960 #9 0x081795a4 in js::FetchName<(js::GetNameMode)0> (cx=0xf791d000, receiver=..., holder=..., name=..., prop=..., vp=...) at js/src/vm/Interpreter-inl.h:187 #10 0x0816b84e in js::GetEnvironmentName<(js::GetNameMode)0> (vp=..., name=..., envChain=..., cx=<optimized out>) at js/src/vm/Interpreter-inl.h:257 #11 GetNameOperation (vp=..., pc=<optimized out>, fp=<optimized out>, cx=<optimized out>) at js/src/vm/Interpreter.cpp:218 #12 Interpret (cx=0xf791d000, state=...) at js/src/vm/Interpreter.cpp:3139 #13 0x0816f855 in js::RunScript (cx=0xf791d000, state=...) at js/src/vm/Interpreter.cpp:395 #14 0x0816fdcf in js::InternalCallOrConstruct (cx=0xf791d000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:473 #15 0x081700ac in InternalCall (cx=cx@entry=0xf791d000, args=...) at js/src/vm/Interpreter.cpp:500 #16 0x081701ff in js::CallFromStack (cx=0xf791d000, args=...) at js/src/vm/Interpreter.cpp:506 #17 0x08235b6f in js::jit::DoCallFallback (cx=0xf791d000, frame=0xffffc028, stub_=0xf53943a0, argc=1, vp=0xffffbfe8, res=...) at js/src/jit/BaselineIC.cpp:2353 #18 0x5105eb8c in ?? () #19 0xf53943a0 in ?? () #20 0x51057c66 in ?? () eax 0x0 0 ebx 0x7ffe5cf2 2147376370 ecx 0xf7da4864 -136689564 edx 0x0 0 esi 0xf47a3b24 -193316060 edi 0x0 0 ebp 0xffffb4f8 4294948088 esp 0xffffb4d0 4294948048 eip 0x8601271 <js::PCToLineNumber(unsigned int, unsigned char*, unsigned char*, unsigned char*, unsigned int*)+401> => 0x8601271 <js::PCToLineNumber(unsigned int, unsigned char*, unsigned char*, unsigned char*, unsigned int*)+401>: movl $0x0,0x0 0x860127b <js::PCToLineNumber(unsigned int, unsigned char*, unsigned char*, unsigned char*, unsigned int*)+411>: ud2 This issue seems to be 32-bit only, could be an integer overflow of some sort. Marking s-s because it is 32-bit only and the assertion sounds potentially dangerous.
Comment 1•4 years ago
|
||
This just sounds like an overflow in computing the column number, which isn't a big deal, but I guess it could mean that there are bogus values floating around that get used in some other place, so I'm going to leave this closed, but mark it sec-audit. Please remove that rating or assign a new one if something more bad is happening.
Keywords: csectype-intoverflow,
sec-audit
Updated•4 years ago
|
Flags: needinfo?(nihsanullah)
Not sure who else to needinfo? here about column numbers. Setting needinfo? from Shu-yu to see how to move this forward.
Flags: needinfo?(shu)
Assignee | ||
Comment 3•4 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #1) > This just sounds like an overflow in computing the column number, which > isn't a big deal, but I guess it could mean that there are bogus values > floating around that get used in some other place, so I'm going to leave > this closed, but mark it sec-audit. Please remove that rating or assign a > new one if something more bad is happening. This is indeed a *signed* 32bit overflow in computing the column number, but the actual bug is that our column handling is a piece of shit. The bug is this: for each statement we emit in the bytecode emitter, we emit srcnotes for encoding line/col information as a delta from the previous srcnote. The previous line and column offsets are kept in variables called |currentLine| and |lastColumn|. The BCE also tries to be smart and not emit useless expressions. This fuzz test is full of useless expressions where the statement is just a number literal like |6231;| When it decides to skip emitting a useless expression, it resets |lastColumn| to 0. I don't know why it does this, it doesn't roll back the already-encoded srcnote or anything. If the next immediate statement is *also* useless, it'll encode a srcnote with the absolute column number as the column delta, since |lastColumn| is now 0. Do this 0xFFFC times, and we have a series of column deltas that easily sum up to > UINT32_MAX. This means column numbers right now for scripts that have useless expression statements are just totally bogus. I'm too bemused right now to figure out a fix. I'll look more tomorrow.
Flags: needinfo?(shu)
(In reply to Shu-yu Guo [:shu] from comment #3) > I'll look more tomorrow. Assigning to Shu-yu based on this. Is bug 857052 also likely related?
Assignee | ||
Comment 5•4 years ago
|
||
This looks like a dupe of bug 857052, yes.
Assignee | ||
Comment 7•4 years ago
|
||
See comment 3 for explanation. I can't figure out why we would reset line/column info when emitting useless statements right now. I removed that code and tests still pass.
Attachment #8867899 -
Flags: review?(jorendorff)
Comment 8•4 years ago
|
||
Comment on attachment 8867899 [details] [diff] [review] Don't reset column and line info when emitting useless statements in BCE. Review of attachment 8867899 [details] [diff] [review]: ----------------------------------------------------------------- r=me. But please update the comments on currentLine/lastColumn to warn that if they get out of sync with the already-emitted source notes, we can get undefined behavior... or else find some better way to stop this happening again. I don't know what that would be though. Anyway, for sure we should assign to those fields only in the 1-2 methods that emit SRC_NEWLINE/SETLINE/COLSPAN, and nowhere else. I don't know why we emit SRC_SETLINE manually in emitCStyleFor. Seems horrible; isn't it better to call updateLineNumberNotes()?
Attachment #8867899 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 9•4 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #8) > Comment on attachment 8867899 [details] [diff] [review] > Don't reset column and line info when emitting useless statements in BCE. > > Review of attachment 8867899 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me. But please update the comments on currentLine/lastColumn to warn that > if they get out of sync with the already-emitted source notes, we can get > undefined behavior... or else find some better way to stop this happening > again. I don't know what that would be though. > > Anyway, for sure we should assign to those fields only in the 1-2 methods > that emit SRC_NEWLINE/SETLINE/COLSPAN, and nowhere else. I don't know why we > emit SRC_SETLINE manually in emitCStyleFor. Seems horrible; isn't it better > to call updateLineNumberNotes()? I imagine the emitCStyleFor case is due to its emitting parts out of line order, and updateLineNumberNotes only handles things in line number order with the delta computation, or something?
Comment 10•4 years ago
|
||
Backed out from inbound for failures in its own test. BTW, pulsebot doesn't comment in sec bugs, so you'll need to mark the bug manually when pushing in the future. https://hg.mozilla.org/integration/mozilla-inbound/rev/1d58c6daabeda497d1505f7af1fc61baa0e3e6d5 https://treeherder.mozilla.org/logviewer.html#?job_id=101592200&repo=mozilla-inbound
![]() |
||
Updated•4 years ago
|
Flags: needinfo?(shu)
Assignee | ||
Comment 11•4 years ago
|
||
Re-pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/f5bcb4ddb655
Flags: needinfo?(shu)
Comment 12•4 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f5bcb4ddb655 Setting Beta and ESR52 to wontfix per IRC discussion with Shu.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox54:
--- → wontfix
status-firefox-esr52:
--- → wontfix
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•4 years ago
|
status-firefox53:
--- → wontfix
Updated•4 years ago
|
Group: javascript-core-security → core-security-release
Updated•4 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main55-]
Updated•4 years ago
|
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect][adv-main55-] → [jsbugmon:update,bisect][adv-main55-][post-critsmash-triage]
Updated•3 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•