Closed Bug 1847369 Opened 11 months ago Closed 11 months ago

MOZ_CRASH(JSON.stringify mismatch between fast and slow paths) at builtin/JSON.cpp:1704

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
118 Branch
Tracking Status
firefox-esr102 --- unaffected
firefox-esr115 --- unaffected
firefox116 --- unaffected
firefox117 --- fixed
firefox118 --- fixed

People

(Reporter: lukas.bernhard, Assigned: sfink)

References

(Depends on 1 open bug, Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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) 
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Flags: needinfo?(sphink)

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.

Flags: needinfo?(sphink)
Assignee: nobody → sphink
Status: NEW → ASSIGNED

Note that I'll probably just disable the fast path entirely on beta.

Keywords: regression
Regressed by: 1837410

Set release status flags based on info from the regressing bug 1837410

Severity: -- → S3
Depends on: sm-runtime
Priority: -- → P1
Attachment #9347777 - Attachment description: Bug 1847369 - never allow indexed properties in slots → Bug 1847369 - only allow indexed properties in elements for both Arrays and non-Arrays
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/62c1fc4ecc8d
only allow indexed properties in elements for both Arrays and non-Arrays r=jandem
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de8c85a84de9
only allow indexed properties in elements for both Arrays and non-Arrays r=jandem
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9adf1877ada7
only allow indexed properties in elements for both Arrays and non-Arrays r=jandem
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → 118 Branch
Flags: needinfo?(sphink)

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
Attachment #9347777 - Flags: approval-mozilla-beta?

Comment on attachment 9347777 [details]
Bug 1847369 - only allow indexed properties in elements for both Arrays and non-Arrays

Approved for 117.0b8.

Attachment #9347777 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+
Duplicate of this bug: 1845671
Duplicate of this bug: 1847668
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: