Closed Bug 1120677 Opened 5 years ago Closed 5 years ago

Assertion failure: retAddr != nullptr, at jit/JitFrames.cpp

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

// Randomly chosen test: js/src/jit-test/tests/debug/bug1001372.js
var g = newGlobal();
g.parent = this;
g.eval('Debugger(parent).onExceptionUnwind = function() {};');
// Randomly chosen test: js/src/jit-test/tests/basic/bug792239.js
// -- reduced away --
// Randomly chosen test: js/src/jit-test/tests/baseline/bug847446.js
function h() {
    yield;
}
function f() {
    for (var i in h()) {
        f();
    }
}
f();

asserts js debug shell on m-c changeset cac64af410a1 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: retAddr != nullptr, at jit/JitFrames.cpp.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20150110105930" and the hash "a55f093defe1".
The "bad" changeset has the timestamp "20150110111030" and the hash "c213d9d53886".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a55f093defe1&tochange=c213d9d53886

Setting s-s to be safe as this is probably related to profiling judging from the assertion name, and setting needinfo? from Jan. Jan, is bug 1118826 a likely regressor?
Flags: needinfo?(jdemooij)
Attached file stack
(lldb) bt 5
* thread #1: tid = 0x7b8da, 0x0000000100382b39 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`js::jit::GetPcScript(cx=<unavailable>, scriptRes=<unavailable>, pcRes=<unavailable>) + 1529 at JitFrames.cpp:1484, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x0000000100382b39 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`js::jit::GetPcScript(cx=<unavailable>, scriptRes=<unavailable>, pcRes=<unavailable>) + 1529 at JitFrames.cpp:1484
    frame #1: 0x0000000100534583 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`JSContext::findVersion() const [inlined] JSContext::currentScript(this=<unavailable>, ppc=<unavailable>, allowCrossCompartment=<unavailable>) const + 85 at jscntxtinlines.h:463
    frame #2: 0x000000010053452e js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`JSContext::findVersion(this=0x0000000101f03c60) const + 14 at jscntxt.cpp:1212
    frame #3: 0x00000001005419c8 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`JS::CompileOptions::CompileOptions(this=0x00007fff5f9ffaa8, cx=0x0000000101f03c60, version=<unavailable>) + 280 at jsapi.cpp:3977
    frame #4: 0x0000000100641ce4 js-dbg-opt-64-dm-nsprBuild-darwin-cac64af410a1`js::CloneScript(cx=0x0000000101f03c60, newKind=<unavailable>, enclosingScope=<unavailable>, fun=<unavailable>, src=<unavailable>) + 4052 at jsscript.cpp:3080
(lldb)
if this is a null deref does it need to be a s-s bug?
Flags: needinfo?(gary)
I don't think so, moreover it's debugger-related so it probably isn't that severe. Was just being cautious. Will just leave it for Jan to open up.
Flags: needinfo?(gary)
Attached patch PatchSplinter Review
This patch fixes GetPcScript to not use the PcScriptCache for BaselineFrames with an override pc.

I had to rewrite GetPcScript to use it.type() instead of it.prevType(). Although this may require an extra ++it, that shouldn't really matter and I think it's more readable and more similar to what we use elsewhere.

We can simplify GetPcScript a bit more by allocating PcScriptCache eagerly, so that we don't need to check it's non-null, but I want to backport this patch so I'll do that separately.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8550252 - Flags: review?(shu)
Comment on attachment 8550252 [details] [diff] [review]
Patch

Review of attachment 8550252 [details] [diff] [review]:
-----------------------------------------------------------------

Looks sensible enough to me.

::: js/src/jit/JitFrames.cpp
@@ +1455,5 @@
>  GetPcScript(JSContext *cx, JSScript **scriptRes, jsbytecode **pcRes)
>  {
>      JitSpew(JitSpew_IonSnapshots, "Recover PC & Script from the last frame.");
>  
> +    // Recover the return address.

I would like to expand this comment and explain that whole purpose of recovering the return address is to lookup in a retaddr->pc cache, as pc computation is expensive. But if we have an override pc, which is cheap to get, we definitely don't need the return address. Moreover, sometimes when an override pc is present during exception handling, the return address is nullptr as a sanity check since we do not return to the frame that threw the exception.

@@ +1459,5 @@
> +    // Recover the return address.
> +    JitFrameIterator it(cx);
> +    uint8_t *retAddr;
> +    if (it.type() == JitFrame_Exit) {
> +        ++it;

I agree, I think avoiding using prevType() makes this function easier to read and reason about anyhow.
Attachment #8550252 - Flags: review?(shu) → review+
Opening up because it's not security sensitive, but we should backport it to 37.
Group: core-security
NI myself to request approval tomorrow.
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/491f9b6be0ee
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8550252 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1118826 part 1 (landed in 37 right before the merge).
[User impact if declined]: Maybe crashes or bugs in some cases. I'm not sure and it's a simple patch so better safe than sorry.
[Describe test coverage new/current, TreeHerder]: Landed on m-c yesterday, fixes reported testcase.
[Risks and why]: Low risk.
[String/UUID change made/needed]: No.
Flags: needinfo?(jdemooij)
Attachment #8550252 - Flags: approval-mozilla-aurora?
Attachment #8550252 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.