1610192 - Assertion failure: propIndex <= ATOM_INDEX_MASK, at js/src/frontend/ObjLiteral.h:307

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200107-e728bf01a2b6 (build with (buildFlags not available), run with --fuzzing-safe --no-threads):

var s0 = "";
for (var i43 = 0; i43 < 100; i43++)
    s0 += new Uint8Array(0x1000);
var s1 = "";
for (var i43 = 0; i43 < 30; i43++)
    s1 += s0;
eval("[" + s1 + "1]")

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555561ac534 in js::frontend::BytecodeEmitter::emitObjLiteralArray(js::frontend::ParseNode*, bool) ()
#1  0x000055555618fbb1 in js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*, js::frontend::ValueUsage, js::frontend::BytecodeEmitter::EmitLineNumberNote, bool) ()
#2  0x00005555561a57c0 in js::frontend::BytecodeEmitter::emitExpressionStatement(js::frontend::UnaryNode*) ()
#3  0x000055555618fedf in js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*, js::frontend::ValueUsage, js::frontend::BytecodeEmitter::EmitLineNumberNote, bool) ()
#4  0x000055555619012c in js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*, js::frontend::ValueUsage, js::frontend::BytecodeEmitter::EmitLineNumberNote, bool) ()
#5  0x000055555619e2c5 in js::frontend::BytecodeEmitter::emitLexicalScope(js::frontend::LexicalScopeNode*) ()
#6  0x000055555619004b in js::frontend::BytecodeEmitter::emitTree(js::frontend::ParseNode*, js::frontend::ValueUsage, js::frontend::BytecodeEmitter::EmitLineNumberNote, bool) ()
#7  0x0000555556193727 in js::frontend::BytecodeEmitter::emitScript(js::frontend::ParseNode*) ()
#8  0x00005555561b932d in js::frontend::ScriptCompiler<char16_t>::compileScript(js::frontend::BytecodeCompiler&, JS::Handle<JSObject*>, js::frontend::SharedContext*) ()
#9  0x0000555556180597 in js::frontend::CompileEvalScript(js::frontend::EvalScriptInfo&, JS::SourceText<char16_t>&) ()
#10 0x00005555559595aa in EvalKernel(JSContext*, JS::Handle<JS::Value>, EvalType, js::AbstractFramePtr, JS::Handle<JSObject*>, unsigned char*, JS::MutableHandle<JS::Value>) ()
#11 0x0000555555959db7 in js::DirectEval(JSContext*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#12 0x00005555563b1280 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) ()
#13 0x000029e33f2caf33 in ?? ()
#14 0x0000000000000000 in ?? ()
rax	0x555556f3aa65	93825019390565
rbx	0x7fffd4180988	140736751733128
rcx	0x555557f1d838	93825036048440
rdx	0x0	0
rsi	0x7ffff6efd770	140737336301424
rdi	0x7ffff6efc540	140737336296768
rbp	0x7fffffff8e40	140737488326208
rsp	0x7fffffff8c50	140737488325712
r8	0x7ffff6efd770	140737336301424
r9	0x7ffff7f98d00	140737353714944
r10	0x58	88
r11	0x7ffff6ba47a0	140737332791200
r12	0x100000000	4294967296
r13	0x800000	8388608
r14	0x7fffffff8c88	140737488325768
r15	0x7fffffff8c60	140737488325728
rip	0x5555561ac534 <js::frontend::BytecodeEmitter::emitObjLiteralArray(js::frontend::ParseNode*, bool)+1620>
=> 0x5555561ac534 <_ZN2js8frontend15BytecodeEmitter19emitObjLiteralArrayEPNS0_9ParseNodeEb+1620>:	movl   $0x133,0x0
   0x5555561ac53f <_ZN2js8frontend15BytecodeEmitter19emitObjLiteralArrayEPNS0_9ParseNodeEb+1631>:	callq  0x5555557f7fc2 <abort>

Marking s-s because the assertion looks dangerous (potential out-of-bounds?).

Comment 1

[Jan de Mooij [:jandem]]

Chris, could this be from your object literal changes?

Comment 2

[Chris Fallin [:cfallin]]

(Today is a US holiday, so I'll just leave this here then check back in tomorrow. Saw the secure-needinfo and figured I had to drop in.)

Yes, this is from the object-literal changes -- there's a limit on the number of elements that can be included in an ObjLiteral because of the bit-packing in its opcode format.

However, I think that modulo the assertion, behavior should actually be correct, because the index that's packed into the ObjLiteral-opcode isn't actually used (see here). So we have (i) the index is too big for the bitfield, and the assert catches that (this bug), but (ii) the index is never used, and the array construction just iterates over all objliteral-ops in order anyway.

So, I don't think this is a security bug, and I think the fix should be to simply not include the index in an array-mode ObjLiteral instruction sequence. (Or if we don't do that, then at least check whether the literal's length is too large; I had thought I had included that check, but apparently not in this case.)

Anyway, I'll check back in tomorrow (post holiday) with a patch.

Comment 3

[Chris Fallin [:cfallin]]

Attachment: Bug 1610192: Remove unneeded array-index in ObjLiteral array mode. r=mgaudet

The ObjLiteral format allows for names (as atom indices) or numeric
indices to be attached to each property in the literal. The numeric case
is limited to 23 bits by the packed format in memory.

The object-literal case ({}-literal with some numeric indices) already
checks for in-range values before deciding to use the ObjLiteral
infrastructure, but the array case did not check this.

Fortunately, the indices in the array case were only used as a sanity
check for debug-mode assertions, and never for any actual indexing
operation, so no out-of-bounds behavior could result, even before this
fix. However, the sanity-check assert will fire at
object-construction-from-ObjLiteral time in cases where the ObjLiteral
mode should not have been used, because no range check was done at
ObjLiteral-construction time.

This change removes the use of indices for array-mode ObjLiterals
entirely.

Comment 4

[Chris Fallin [:cfallin]]

Added patch; clearing needinfo.

Comment 5

[Jan de Mooij [:jandem]]

Thanks! Opening up because it's just a bogus assert.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ea36d82a283
Remove unneeded array-index in ObjLiteral array mode. r=mgaudet

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a56353f21094
Backed out changeset 5ea36d82a283 for causing SM bustages in bug1610192.js

Comment 8

[Alexandru Michis [:malexandru]]

Backed out changeset 5ea36d82a283 (Bug 1610192) for causing SM bustages in bug1610192.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/a56353f21094a2a55ae66c17ae762ce043bcc18e

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&tochange=5ea36d82a283c479423faaabf0c7f2eb622167ed&fromchange=1b24a124f0b4e727f168a6b20ec093102fed14b6&searchStr=sm&selectedJob=285815203

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285815203&repo=autoland&lineNumber=12893

[task 2020-01-21T18:54:47.485Z] TEST-PASS | js/src/jit-test/tests/sunspider/check-string-fasta.js | Success (code 0, args "") [12.7 s]
[task 2020-01-21T18:54:47.934Z] Exit code: -6
[task 2020-01-21T18:54:47.934Z] TIMEOUT - basic/bug1610192.js
[task 2020-01-21T18:54:47.934Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/bug1610192.js | Timeout (code -6, args "") [150.0 s]
[task 2020-01-21T18:54:47.934Z] INFO exit-status : -6
[task 2020-01-21T18:54:47.934Z] INFO timed-out : 0:00:00.018974
[task 2020-01-21T18:54:48.121Z] TEST-PASS | js/src/jit-test/tests/wasm/atomicity.js | Success (code 0, args "") [0.2 s]

Comment 9

[Chris Fallin [:cfallin]]

Yup, the test timed out on arm64 and on the cgc config. I've updated the patch to allow timeouts on these tests; here's the new try-run: link. Both of the failing configs (with timed-out tests) are green now. I'll try re-landing.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eeb6fd68c022
Remove unneeded array-index in ObjLiteral array mode. r=mgaudet

Comment 11

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/eeb6fd68c022

Comment 12

[Ryan VanderMeulen [:RyanVM]]

IIUC, this is a regression from bug 1580246. Is there a user impact here which justifies Beta uplift consideration or can this fix ride Fx74 to release?

Comment 13

[Chris Fallin [:cfallin]]

I don't think there is user impact: the assertion was an unnecessary check, because the value it was asserting on was never actually used. So in a release build where MOZ_ASSERT() is a no-op, this bug should be invisible and I don't think we need to uplift. (If we have any release builds with assertions enabled, though, then IMHO we should uplift this.)