Closed Bug 1366903 Opened 3 years ago Closed 2 years ago

Crash [@ JSObject::finalize] or Assertion failure: obj->getElementsHeader()->ownerObject() != obj, at js/src/vm/NativeObject.cpp:977

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 55+ fixed
firefox53 --- wontfix
firefox54 + wontfix
firefox55 + verified
firefox56 + verified

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(6 keywords, Whiteboard: [jsbugmon:update][adv-main55+][adv-esr52.3+])

Attachments

(4 files)

The following testcase crashes on mozilla-central revision f9ca97a33429 (build with --enable-debug, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug1106719.js
gcparam("maxBytes", gcparam("gcBytes"));
// jsfunfuzz-generated
Array.prototype.toSource = Array.prototype.splice;
for (var i = 0; i < 2; i++) {
    try {
        new [1];
    } catch (e) {};
}

Backtrace:

#0  0x0000000000b6eb48 in js::NativeObject::CopyElementsForWrite (cx=cx@entry=0x7f491f372000, obj=0x7f491e598080) at js/src/vm/NativeObject.cpp:977
#1  0x00000000004f2668 in js::NativeObject::maybeCopyElementsForWrite (cx=0x7f491f372000, this=<optimized out>) at js/src/vm/NativeObject.h:1094
#2  js::ArraySetLength (cx=cx@entry=0x7f491f372000, arr=arr@entry=..., id=id@entry=..., attrs=196, value=..., value@entry=..., result=...) at js/src/jsarray.cpp:669
#3  0x0000000000b867d9 in SetExistingProperty (result=..., prop=..., pobj=..., receiver=..., v=..., id=..., obj=..., cx=0x7f491f372000) at js/src/vm/NativeObject.cpp:2526
#4  js::NativeSetProperty (cx=cx@entry=0x7f491f372000, obj=..., obj@entry=..., id=..., value=..., value@entry=..., receiver=..., qualified=qualified@entry=js::Qualified, result=...) at js/src/vm/NativeObject.cpp:2591
#5  0x00000000005039d6 in js::SetProperty (result=..., receiver=..., v=..., id=..., obj=..., cx=0x7f491f372000) at js/src/vm/NativeObject.h:1541
/snip

For detailed crash information, see attachment.

Apparently 0xe5e5e5e5 (weird memory address) is accessed when the testcase is run on opt builds, so setting s-s as a start.
Attached file opt crash stack
Debug build configuration parameters:

AR=ar sh ./configure --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Opt build configuration parameters:

AR=ar sh ./configure --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests
Possible regression range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=83ced53b4298&tochange=e7806c9c83f3

Looking through it, it seems bug 934450 may be a possible regressor.

Jan/Brian, what do you think?
Blocks: 934450
Flags: needinfo?(jdemooij)
Flags: needinfo?(bhackett1024)
I had tested that the testcase does not crash with the nightly js shell jsshell-linux-x86_64.zip on Ubuntu 16.04 in:

https://archive.mozilla.org/pub/firefox/nightly/2014/08/2014-08-20-mozilla-central-debug/

but does with the nightly js shell in:

https://archive.mozilla.org/pub/firefox/nightly/2014/08/2014-08-21-mozilla-central-debug/

The application.ini files show changeset revs that map to the hashes in comment 4.
I just confirmed that this reproduces nicely when using oomTest() instead of gcparam. We should check why we didn't hit this earlier. It doesn't seem too exotic to be entirely missed by oom testing.
This is pretty bad. Basically the expression decompiler is calling ValueToSource which does obj.toSource(). Usually that's fine, but here |obj| is one of the canonical object/array/regexp objects stored in the script and we really don't want these to escape to JS.

I'll write a patch.
Attached patch Part 1 - FixSplinter Review
Fix decompilation of JSOP_REGEXP and JSOP_NEWARRAY_COPYONWRITE to not use ValueToSource or anything else that can let the object escape to JS.

JSOP_OBJECT is okay because that object escapes to JS anyway.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8872623 - Flags: review?(nicolas.b.pierron)
Flags: needinfo?(bhackett1024)
Attachment #8872624 - Flags: review?(nicolas.b.pierron)
Attachment #8872623 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8872624 - Flags: review?(nicolas.b.pierron) → review+
We could still potentially land this for 54. Can you request sec-approval and uplift to 54, if you think it's worth the risk to land it for the RC build next Monday?
Flags: needinfo?(jdemooij)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> We could still potentially land this for 54. Can you request sec-approval
> and uplift to 54, if you think it's worth the risk to land it for the RC
> build next Monday?

It's a very old regression (3 years if it's from bug 934450) and the patch is not entirely trivial. I'll request sec-approval but it might be best to land this a few weeks from now...
Comment on attachment 8872623 [details] [diff] [review]
Part 1 - Fix

See comment 11. It's probably best to land this everywhere a few weeks from now.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's not clear if it's exploitable but we should assume it is.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be backportable.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely but some testing would be good.
Flags: needinfo?(jdemooij)
Attachment #8872623 - Flags: sec-approval?
OK, sounds like a wontfix for 54. Thanks Jan.
If this is won't fix for 54, it isn't going to get sec-approval to go in now. This can't land until at least two weeks into the next cycle on 6/27. So I'll give it sec-approval+ to land on June 27.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 6/27]
Attachment #8872623 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9489f5df7bea25619d19e2688fd7065cc724bad

This grafts cleanly to Beta and ESR52. Please request approval when you get a chance.
Flags: needinfo?(jdemooij)
Whiteboard: [jsbugmon:update][checkin on 6/27] → [jsbugmon:update]
https://hg.mozilla.org/mozilla-central/rev/d9489f5df7be
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Comment on attachment 8872623 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/Bug causing the regression]: Old bug.
[User impact if declined]: Security bugs.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[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?]: No.
[Why is the change risky/not risky?]: This code isn't used a lot and the patch is pretty simple.
[String changes made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8872623 - Flags: approval-mozilla-esr52?
Attachment #8872623 - Flags: approval-mozilla-beta?
Comment on attachment 8872623 [details] [diff] [review]
Part 1 - Fix

sec-crit fix for beta55, esr52.3
Attachment #8872623 - Flags: approval-mozilla-esr52?
Attachment #8872623 - Flags: approval-mozilla-esr52+
Attachment #8872623 - Flags: approval-mozilla-beta?
Attachment #8872623 - Flags: approval-mozilla-beta+
(In reply to Jan de Mooij [:jandem] from comment #18)
> [Is this code covered by automated tests?]: Yes.
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Jan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main55+][adv-esr52.3+]
JSBugMon: This bug has been automatically verified fixed on Fx55
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.