Crash [@ js::jit::JSJitFrameIter::checkInvalidation] with OOM

RESOLVED FIXED in Firefox 57

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: decoder, Assigned: jandem)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla58
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57+ fixed, firefox58+ fixed)

Details

(Whiteboard: [jsbugmon:update,bisect][adv-main57+][post-critsmash-triage], crash signature)

Attachments

(2 attachments)

The following testcase crashes on mozilla-central revision 8db0c4ecd94c (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 --cpu-count=2 --ion-aa=flow-sensitive):

See attachment.


Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf5affb40 (LWP 20965)]
js::jit::JSJitFrameIter::checkInvalidation (this=0xf5aec7ec, ionScriptOut=0xf5aec768) at js/src/jit/JSJitFrameIter.cpp:64
#0  js::jit::JSJitFrameIter::checkInvalidation (this=0xf5aec7ec, ionScriptOut=0xf5aec768) at js/src/jit/JSJitFrameIter.cpp:64
#1  0x083757ad in js::jit::JSJitFrameIter::checkInvalidation (this=0xf5aec7ec) at js/src/jit/JSJitFrameIter.cpp:42
#2  0x082f5e77 in InvalidateActivation (fop=fop@entry=0xf5fc3d60, activations=..., invalidateAll=invalidateAll@entry=false) at js/src/jit/Ion.cpp:3029
#3  0x082f6efb in js::jit::Invalidate (types=..., fop=0xf5fc3d60, invalid=..., resetUses=true, cancelOffThread=true) at js/src/jit/Ion.cpp:3184
#4  0x0833150a in js::jit::Invalidate (cancelOffThread=true, resetUses=true, invalid=..., cx=0xf5d82800) at js/src/jit/Ion.cpp:3223
#5  js::jit::Invalidate (cx=0xf5d82800, script=0xf5709438, resetUses=true, cancelOffThread=true) at js/src/jit/Ion.cpp:3269
#6  0x08afb64b in InvalidateAfterBailout (cx=0xf5d82800, outerScript=..., reason=<optimized out>) at js/src/jit/BaselineBailouts.cpp:1727
#7  0x08b00280 in HandleBoundsCheckFailure (innerScript=..., outerScript=..., cx=<optimized out>) at js/src/jit/BaselineBailouts.cpp:1740
#8  js::jit::FinishBailoutToBaseline (bailoutInfo=0x0) at js/src/jit/BaselineBailouts.cpp:2046
#9  0x2a047504 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)
eax	0xd3ff0000	-738263040
ebx	0x2a08b2ae	705213102
ecx	0xf5722360	-177069216
edx	0xf573ab20	-176968928
esi	0x8dceff4	148697076
edi	0xf5aec7ec	-173094932
ebp	0xf5aec748	4121872200
esp	0xf5aec740	4121872192
eip	0x83756c3 <js::jit::JSJitFrameIter::checkInvalidation(js::jit::IonScript**) const+99>
=> 0x83756c3 <js::jit::JSJitFrameIter::checkInvalidation(js::jit::IonScript**) const+99>:	mov    -0x4(%ebx,%eax,1),%edx
   0x83756c7 <js::jit::JSJitFrameIter::checkInvalidation(js::jit::IonScript**) const+103>:	mov    (%edx),%ecx


The testcase is not 100% deterministic but it reproduces fairly often for me. It seems to be sensitive to a lot of things though, like output redirection. I can't reduce it further without turning it into a 0.1% testcase :/
Posted file Testcase
Flags: needinfo?(jdemooij)
Posted patch PatchSplinter Review
Excellent find.

In CodeGenerator::link, we call InvalidateCompilerOutputsForScript if linking fails, to mark all CompilerOutputs for the script as invalid. When we're recompiling an Ion script, however, the script may still have a valid IonScript and we incorrectly invalidate its CompilerOutput.

The bug this code was trying to fix is actually already handled correctly (nbp added this code later):

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/js/src/jit/CodeGenerator.cpp#9875-9878

So we can just remove InvalidateCompilerOutputsForScript I think. That fixes the crashes for me.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8912247 - Flags: review?(nicolas.b.pierron)
Attachment #8912247 - Flags: review?(nicolas.b.pierron) → review+
I'll mark this sec-moderate; we should backport this but it seems hard to exploit.
Keywords: sec-moderate
Comment on attachment 8912247 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Crashes.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Just removes some code from an OOM-handling path.
[String changes made/needed]: None.
Attachment #8912247 - Flags: approval-mozilla-esr52?
Attachment #8912247 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/c1a158ca2b1c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8912247 [details] [diff] [review]
Patch

Fix a sec moderate, taking it.
For esr, I don't think it matches the expectation (sec-high or critical only)
Attachment #8912247 - Flags: approval-mozilla-esr52?
Attachment #8912247 - Flags: approval-mozilla-esr52-
Attachment #8912247 - Flags: approval-mozilla-beta?
Attachment #8912247 - Flags: approval-mozilla-beta+
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main57+]
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect][adv-main57+] → [jsbugmon:update,bisect][adv-main57+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.