Closed Bug 1601074 Opened 7 months ago Closed 7 months ago

Hit MOZ_CRASH(There should be room for registering the new object) at js/src/vm/TypeInference.cpp:3494

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla73
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 --- fixed
firefox73 --- fixed

People

(Reporter: gkw, Assigned: cfallin)

References

(Regression)

Details

(4 keywords, Whiteboard: [fuzzblocker][jsbugmon:update])

Crash Data

Attachments

(3 files)

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

for (let y of [
    { x: 1 },
    { x: 2 },
    { x: 3 },
    { x: 4 },
    { x: 5 },
    { x: 6 },
    { x: 7 },
    { x: 8 },
    { x: 9 },
    { x: 10 },
    { x: 11 },
]) {}

Backtrace:

#0  0x0000561deba5b0b1 in js::PreliminaryObjectArray::registerNewObject (this=0x7fa67da1fdf0, res=0x2de229e7c580) at js/src/vm/TypeInference.cpp:3494
#1  0x0000561deb3b0f0e in js::NewObjectOperation (cx=0x7fa67dc27000, script=..., pc=0x7fa67d4418c4 "\362\v", newKind=js::GenericObject) at js/src/vm/Interpreter.cpp:5266
#2  0x0000561deb3a0325 in Interpret (cx=0x7fa67dc27000, state=...) at js/src/vm/Interpreter.cpp:3801
#3  0x0000561deb38dffa in js::RunScript (cx=0x7fa67dc27000, state=...) at js/src/vm/Interpreter.cpp:424
#4  0x0000561deb3a8939 in js::ExecuteKernel (cx=0x7fa67dc27000, script=..., envChainArg=..., newTargetValue=..., evalInFrame=..., result=0x0) at js/src/vm/Interpreter.cpp:811
#5  0x0000561deb3a8d0d in js::Execute (cx=0x7fa67dc27000, script=..., envChainArg=..., rval=0x0) at js/src/vm/Interpreter.cpp:844
#6  0x0000561deb65bbd3 in ExecuteScript (cx=0x7fa67dc27000, scope=..., script=..., rval=0x0) at js/src/vm/CompilationAndEvaluation.cpp:453
/snip

For detailed crash information, see attachment.

Setting s-s as a start because opt ASan builds show an error too, log coming up.

Setting [fuzzblocker] as this is happening very often.

Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/7bd0784e7a6f
user: Chris Fallin
date: Wed Nov 27 22:49:33 2019 +0000
summary: Bug 1598347, part 2: pass "inner singleton" to NEWOBJECT group logic. r=djvj,iain

Chris, is bug 1598347 a likely regressor?

Flags: needinfo?(cfallin)
Regressed by: 1598347
Assignee: nobody → cfallin
Flags: needinfo?(cfallin)

Yes, this is almost certainly due to the recent change for bug 1598347. Looking into it!

The recent addition of the JSOP_NEWOBJECT_WITHGROUP opcode for bug
1598347 (itself a regression fix for 1580246) has led to an issue when
more than a certain number of array elements with the same group are
created within an array literal. In particular, when too many objects
are created, the preliminary-objects-set for the ObjectGroup becomes
full and hits a MOZ_CRASH. This patch checks whether the set is full
before adding to it.

Is this a security issue, or is this just going to be a MOZ_CRASH() any time we get in this bad state? Thanks.

Flags: needinfo?(cfallin)

This is just a MOZ_CRASH, not a security issue -- the PreliminaryObjectArray will not overflow its bounds. (See https://searchfox.org/mozilla-central/source/js/src/vm/TypeInference.cpp#3481 for its implementation.) The commit that caused this issue had changed some of the conditions under which objects are added to this array, or which array they are added to, hence the possibility of trying to add to an already-full array. (It's perfectly fine to not include objects, as they only improve analysis precision, so the fix is just to check if full first and do nothing.)

Flags: needinfo?(cfallin)
Group: javascript-core-security
Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4c1a6acc2b1
Fix preliminary-objects-set issue wrt NEWOBJECT_WITHGROUP. r=iain
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla73

Comment on attachment 9113328 [details]
Bug 1601074: Fix preliminary-objects-set issue wrt NEWOBJECT_WITHGROUP. r=iain,djvj

Beta/Release Uplift Approval Request

  • User impact if declined: Slight regression in performance (fewer JS functions are inlined) in some cases.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1598347
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): (This is meant to go along with 1598347, so is the same text reproduced below:)

Changes to the guts of the JS engine are always slightly risky, but this has been on m-c for a few days, and the one issue that arose (1601074) was caught quickly by the fuzzing team and fixed. Given that no further issues have arisen, and given that the engine is well-covered by tests, we believe that this risk is not unacceptably high.

  • String changes made/needed:
Attachment #9113328 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1601657
Crash Signature: [@ js::PreliminaryObjectArray::registerNewObject]

Comment on attachment 9113328 [details]
Bug 1601074: Fix preliminary-objects-set issue wrt NEWOBJECT_WITHGROUP. r=iain,djvj

companion fix for bug 1598347, approved for 72.0b4

Attachment #9113328 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.