Closed
Bug 1314401
Opened 4 years ago
Closed 4 years ago
Crash [@ intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*)] or Assertion failure: this->is<T>(), at js/src/jsobj.h:567
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla52
People
(Reporter: decoder, Assigned: arai)
References
Details
(6 keywords, Whiteboard: [jsbugmon:update][adv-main50.1+])
Crash Data
Attachments
(3 files)
4.62 KB,
patch
|
till
:
review+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
705 bytes,
patch
|
Details | Diff | Splinter Review | |
4.68 KB,
patch
|
arai
:
review+
dveditz
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 2c773b971672 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off --ion-eager): for (var ret of[null, {}, [], /a/]) { assertEq(RegExp.prototype[Symbol.match].call({ exec(S) { return ret; } }, "foo"), ret); } Backtrace: received signal SIGSEGV, Segmentation fault. 0x000000000095c6c6 in intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*) () at js/src/vm/SelfHosting.cpp:139 #0 0x000000000095c6c6 in intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*) () at js/src/vm/SelfHosting.cpp:139 #1 0x0000000000922570 in js::CallJSNative (args=..., native=<optimized out>, cx=0x7ffff695f000) at js/src/jscntxtinlines.h:239 #2 js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:458 #3 0x00000000009226b5 in InternalCall (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:503 #4 0x00000000009226e5 in js::CallFromStack (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:509 #5 0x000000000056237b in js::jit::DoCallFallback (cx=0x7ffff695f000, frame=0x7fffffffcc08, stub_=0x7ffff69d5540, argc=2, vp=0x7fffffffcb70, res=...) at js/src/jit/BaselineIC.cpp:6012 #6 0x00007ffff7e44daf in ?? () [...] #38 0x0000000000000000 in ?? () rax 0xfff8800000000002 -2111062325329918 rbx 0x7ffff695f000 140737330409472 rcx 0x7ffff0700560 140737227261280 rdx 0x7fffffffcb70 140737488341872 rsi 0x0 0 rdi 0x7ffff695f000 140737330409472 rbp 0x7fffffffc810 140737488341008 rsp 0x7fffffffc7e8 140737488340968 r8 0x7fffffffffff 140737488355327 r9 0xfffdffffffffffff -562949953421313 r10 0x7ffff03d2400 140737223926784 r11 0x7ffffffff000 140737488351232 r12 0x7fffffffc8f0 140737488341232 r13 0x95c6b0 9815728 r14 0x7ffff69d5540 140737330894144 r15 0x7ffff695f000 140737330409472 rip 0x95c6c6 <intrinsic_UnsafeGetInt32FromReservedSlot(JSContext*, unsigned int, JS::Value*)+22> => 0x95c6c6 <_ZL40intrinsic_UnsafeGetInt32FromReservedSlotP9JSContextjPN2JS5ValueE+22>: mov 0x10(%rsi),%esi 0x95c6c9 <_ZL40intrinsic_UnsafeGetInt32FromReservedSlotP9JSContextjPN2JS5ValueE+25>: shr $0x1b,%esi Not sure if this is s-s or not, but having a function with the name "unsafe" on the stack in combination with a crash is probably a reason to be cautious.
Updated•4 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•4 years ago
|
||
JSBugMon: Bisection requested, result: === Treeherder Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20161025030604" and the hash "00b6c424d4409ec920f4d9c206680dcad897d99e". The "bad" changeset has the timestamp "20161025031004" and the hash "1077b4771709b0df601d86e52f486d5e4345fb3f". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=00b6c424d4409ec920f4d9c206680dcad897d99e&tochange=1077b4771709b0df601d86e52f486d5e4345fb3f
autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/1077b4771709 user: Tooru Fujisawa date: Tue Oct 25 19:09:19 2016 +0900 summary: Bug 1263340 - Part 9: Add optimizable path for RegExpMatch with global flag. r=till Arai-san, is bug 1263340 a likely regressor?
Blocks: 1263340
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 3•4 years ago
|
||
I forgot to check IsRegExpObject in IsRegExpMethodOptimizable... RegExpInstanceOptimizable should be called only with RegExpObject, otherwise code in RegExpObject::isInitialShape doesn't make sense. the underlying issue is from bug 887016 (firefox 49, backed out from firefox 48 by bug 1265307), and affects all branches except esr45. here's the changes: * Added IsRegExpObject to IsRegExpMethodOptimizable now all RegExpInstanceOptimizable caller checks IsRegExpObject before calling it * Removed isNative() check from js::RegExpInstanceOptimizableRaw since it should be checked in IsRegExpObject * Changed type used in js::RegExpInstanceOptimizableRaw from NativeObject* to RegExpObject* test will be in another patch, that should be landed separately, after the fix is landed to all affected branches.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(arai.unmht)
Attachment #8806545 -
Flags: review?(till)
Comment 4•4 years ago
|
||
Comment on attachment 8806545 [details] [diff] [review] Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. Review of attachment 8806545 [details] [diff] [review]: ----------------------------------------------------------------- Makes sense.
Attachment #8806545 -
Flags: review?(till) → review+
Assignee | ||
Comment 5•4 years ago
|
||
thanks :) here's the testcase, that is the same as comment #0.
Assignee | ||
Updated•4 years ago
|
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox51:
--- → affected
status-firefox-esr45:
--- → unaffected
tracking-firefox49:
--- → ?
tracking-firefox50:
--- → ?
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
Assignee | ||
Comment 6•4 years ago
|
||
Comment on attachment 8806545 [details] [diff] [review] Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. [Security approval request comment] > How easily could an exploit be constructed based on the patch? It could easily hit the crash. Not sure how it's easy to do some more, but potentially it could. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, it says type check is insufficient. This patch doesn't contain test, since the test contains more info. will land the test after the fix is landed to all affected branches. > Which older supported branches are affected by this flaw? firefox49 > If not all supported branches, which bug introduced the flaw? underlying issue was from bug 887016, (that was landed to firefox48, but backed out from firefox48 aurora) and exposed to more by bug 1263340 etc. > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? not yet, but should be easy. will prepare shortly and attach them, and ask approval. > How likely is this patch to cause regressions; how much testing does it need? Less likely. Adds some more checks before applying optimization. Affected cases falls back to slow path.
Attachment #8806545 -
Flags: sec-approval?
Assignee | ||
Comment 7•4 years ago
|
||
this patch is applicable to mozilla-aurora/mozilla-beta/mozilla-release no actual changes from original one, just surrounding comment change. Approval Request Comment > [Feature/regressing bug #] underlying issue was from bug 887016, (that was landed to firefox48, but backed out from firefox48 aurora) and exposed to more by bug 1263340 etc. > [User impact if declined] crash, and possible more critical exploit, by dereferencing wrong memory block as pointer > [Describe test coverage new/current, TreeHerder] not tested in automated tests. passed part 2 testcase locally. > [Risks and why] Less likely. Adds some more checks before applying optimization. Affected cases falls back to slow path. > [String/UUID change made/needed] None
Attachment #8806690 -
Flags: review+
Attachment #8806690 -
Flags: approval-mozilla-release?
Attachment #8806690 -
Flags: approval-mozilla-beta?
Attachment #8806690 -
Flags: approval-mozilla-aurora?
Comment 10•4 years ago
|
||
Comment on attachment 8806545 [details] [diff] [review] Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. sec-approval=dveditz I'm not sure we can get this into 50 at this point, but if not it should go into 50.1
Attachment #8806545 -
Flags: sec-approval? → sec-approval+
Updated•4 years ago
|
Comment 11•4 years ago
|
||
Comment on attachment 8806690 [details] [diff] [review] (mozilla-aurora/mozilla-beta/mozilla-release) Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till a=dveditz for aurora. Leaving beta/release approvals for release managers. The 50.1 release is OK if it doesn't make 50.
Attachment #8806690 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 12•4 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/05773111312ff06c776ab82602204e84a13dbb2c Bug 1314401 - Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till sec-approval=dveditz
I agree with Dan's suggestion. We are only uplifting release blocking issues this late in RC 50 week. Let's uplift this in time for 50.1.0 release.
Comment 14•4 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/05773111312f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•4 years ago
|
Status: RESOLVED → VERIFIED
Comment 15•4 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 16•4 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/bb22a0218997
Flags: in-testsuite?
Updated•4 years ago
|
Group: javascript-core-security → core-security-release
Comment 17•4 years ago
|
||
Comment on attachment 8806690 [details] [diff] [review] (mozilla-aurora/mozilla-beta/mozilla-release) Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till Since merge day is passed, beta is 51 and release is 50. The patch has already been in 51 so I remove the approval‑mozilla‑beta? flag and keep approval‑mozilla‑release? flag.
Attachment #8806690 -
Flags: approval-mozilla-beta?
Comment on attachment 8806690 [details] [diff] [review] (mozilla-aurora/mozilla-beta/mozilla-release) Part 1: Eagerly check IsRegExpObject in IsRegExpMethodOptimizable. r=till Sec-high, let's uplift to 50.1.0
Attachment #8806690 -
Flags: approval-mozilla-release? → approval-mozilla-release+
Updated•4 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main50.1+]
Updated•4 years ago
|
Comment 20•4 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx51
Updated•4 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•