MOZ_CRASH(JSON.stringify mismatch between fast and slow paths) at builtin/JSON.cpp:1704
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr102 | --- | unaffected |
firefox-esr115 | --- | unaffected |
firefox116 | --- | unaffected |
firefox117 | --- | fixed |
firefox118 | --- | fixed |
People
(Reporter: lukas.bernhard, Assigned: sfink)
References
(Blocks 2 open bugs, Regression)
Details
(Keywords: regression)
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
Steps to reproduce:
The attached sample asserts in the js-shell on git commit 98ab80832fa5003245d709dd65f2b8a243eec4dd when invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --fuzzing-safe crash.js
Bisecting the issue points to commit 3fc396bcdd1f85002e4bd7bd06383f81393b5ca9 related to bug 1837410.
const v1 = this.representativeStringArray();
function f2() {
return v1;
}
v1.valueOf = f2;
JSON.stringify(v1);
#0 0x55e1f2ff3dd8 in js::Stringify(JSContext*, JS::MutableHandle<JS::Value>, JSObject*, JS::Value const&, js::StringBuffer&, js::StringifyBehavior) js/src/builtin/JSON.cpp:1704:7
#1 0x55e1f2ff86e2 in json_stringify(JSContext*, unsigned int, JS::Value*) js/src/builtin/JSON.cpp:2091:8
#2 0x55e1f2eeb90d in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) js/src/vm/Interpreter.cpp:486:13
#3 0x55e1f2eeb0ec in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:580:12
#4 0x55e1f2eec4e0 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) js/src/vm/Interpreter.cpp:647:10
#5 0x55e1f2eec2a4 in js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason) js/src/vm/Interpreter.cpp:652:10
#6 0x55e1f2efed13 in js::Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3395:16
#7 0x55e1f2eea82b in MaybeEnterInterpreterTrampoline(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:400:10
#8 0x55e1f2ee9d6f in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:458:13
#9 0x55e1f2eee09b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:845:13
#10 0x55e1f2eee944 in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:877:10
#11 0x55e1f31498cf in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) js/src/vm/CompilationAndEvaluation.cpp:493:10
#12 0x55e1f3149a1c in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) js/src/vm/CompilationAndEvaluation.cpp:517:10
#13 0x55e1f2d3046e in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) js/src/shell/js.cpp:1099:10
#14 0x55e1f2d2fd0b in Process(JSContext*, char const*, bool, FileKind) js/src/shell/js.cpp:1679:14
#15 0x55e1f2d08e56 in ProcessArgs(JSContext*, js::cli::OptionParser*) js/src/shell/js.cpp:10736:10
#16 0x55e1f2cf7762 in Shell(JSContext*, js::cli::OptionParser*) js/src/shell/js.cpp:10960:12
#17 0x55e1f2cf2515 in main js/src/shell/js.cpp:11392:12
#18 0x7eff41e23a8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#19 0x7eff41e23b48 in __libc_start_main csu/../csu/libc-start.c:360:3
#20 0x55e1f2cbfec8 in _start (obj-x86_64-pc-linux-gnu/dist/bin/js+0x1e8cec8)
Reporter | ||
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 1•1 year ago
|
||
Ugh, this is a pretty big category of problematic input. To make it more clear, the test could be written as:
const arr = [];
Object.defineProperty(arr, 0, { value: "hi", enumerated: true });
arr.named = 7;
JSON.stringify(arr);
The problem is that arr
has a indexed property("0") in its slots and a length of 1. When arr["0"]
is processed during FastStr
, it assumes that any elements up to getDenseInitializedLength()
are either in the elements or are holes and can be ignored. But that's not true, indexed elements can always be found in the slots instead.
I think the straightforward fix is to weaken what the fast path can handle, and only permit finding Array members in dense elements. Sad, but I want the most basic things to work before worrying about handling more with the fast path.
Assignee | ||
Updated•1 year ago
|
Assignee | ||
Comment 2•1 year ago
|
||
Note that I'll probably just disable the fast path entirely on beta.
Assignee | ||
Comment 3•1 year ago
|
||
Assignee | ||
Updated•1 year ago
|
Comment 4•1 year ago
|
||
Set release status flags based on info from the regressing bug 1837410
Updated•1 year ago
|
Updated•1 year ago
|
Comment 6•1 year ago
|
||
Backed out for causing Stringify marionette_harness test failures.
Failure log: https://treeherder.mozilla.org/logviewer?job_id=425428642&repo=autoland
Backout link: https://hg.mozilla.org/integration/autoland/rev/e42e5ee05fabc74bb6f4597c3ce0f26c494b232e
Comment 8•1 year ago
|
||
Backed out for causing SM bustages.
Backout link: https://hg.mozilla.org/integration/autoland/rev/580f632ee9e76c2eb61e73fb0d56e3b5a8ad4949
Failure log: https://treeherder.mozilla.org/logviewer?job_id=425613109&repo=autoland&lineNumber=4556
Comment 10•1 year ago
|
||
bugherder |
Updated•1 year ago
|
Assignee | ||
Comment 11•1 year ago
|
||
Comment on attachment 9347777 [details]
Bug 1847369 - only allow indexed properties in elements for both Arrays and non-Arrays
Beta/Release Uplift Approval Request
- User impact if declined: Incorrect behavior when calling JSON.stringify 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: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This patch disables the optimization in more cases, so should be relatively safe.
If an even safer option is desired, I could disable the optimization entirely for beta.
- String changes made/needed: none
- Is Android affected?: Yes
Comment 12•1 year ago
|
||
Comment on attachment 9347777 [details]
Bug 1847369 - only allow indexed properties in elements for both Arrays and non-Arrays
Approved for 117.0b8.
Comment 13•1 year ago
|
||
uplift |
Updated•1 year ago
|
Updated•11 months ago
|
Updated•11 months ago
|
Description
•