Closed Bug 1727364 Opened 3 years ago Closed 3 years ago

Assertion failure: args[0].isObject(), at builtin/intl/NumberFormat.cpp:1258

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
93 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox91 --- unaffected
firefox92 --- wontfix
firefox93 --- verified

People

(Reporter: decoder, Assigned: tcampbell)

References

(Regression)

Details

(4 keywords, Whiteboard: [bugmon:update,bisected,confirmed][adv-main93+r])

Attachments

(4 files)

The following testcase crashes on mozilla-central revision 20210824-7857f4c37a92 (debug build, run with --fuzzing-safe --no-threads --fast-warmup):

function main() {
  v3 = [0, 13.37];
  function v10(v11,v12) {
    try {
        v15 = v10(-3399895606,..."64",7);
        v16 = (-65537).toLocaleString();
    } catch(v17) {
        v18 = [...v3];
    }
  }
  v10();
}
main();

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x00005555571f1f9d in js::intl_FormatNumber(JSContext*, unsigned int, JS::Value*) ()
#1  0x0000555556c17801 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) ()
#2  0x0000555556c16f36 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) ()
#3  0x0000555556c18371 in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason) ()
#4  0x000055555766dd14 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICFallbackStub*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) ()
#5  0x0000385d3548f933 in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x5555557deb7c	93824994896764
rbx	0x7ffff5a4e800	140737314613248
rcx	0x555558128820	93825038190624
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffb6f0	140737488336624
rsp	0x7fffffffb3a0	140737488335776
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f98840	140737353713728
r10	0x0	0
r11	0x0	0
r12	0x7ffff5a4e800	140737314613248
r13	0x7fffffffb940	140737488337216
r14	0x7ffff6019000	140737320685568
r15	0xffff800000000000	-140737488355328
rip	0x5555571f1f9d <js::intl_FormatNumber(JSContext*, unsigned int, JS::Value*)+6749>
=> 0x5555571f1f9d <_ZN2js17intl_FormatNumberEP9JSContextjPN2JS5ValueE+6749>:	movl   $0x4ea,0x0
   0x5555571f1fa8 <_ZN2js17intl_FormatNumberEP9JSContextjPN2JS5ValueE+6760>:	callq  0x555556b06a2a <abort>

Marking this s-s since it requires the JIT and could be a type confusion.

Attached file Detailed Crash Information (deleted) —
Attached file Testcase (deleted) —

ni? Caroline due to IC fallback path.

Flags: needinfo?(ccullen)

Bugmon Analysis
Verified bug as reproducible on mozilla-central 20210824094724-7857f4c37a92.
The bug appears to have been introduced in the following build range:

Start: 8c52592cd83801dac261477f24d21c6c0ab31c47 (20210812164534)
End: 4792c28298b34db879dc527667f9f7820fbaffa8 (20210812171257)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8c52592cd83801dac261477f24d21c6c0ab31c47&tochange=4792c28298b34db879dc527667f9f7820fbaffa8

Whiteboard: [bugmon:update,bisect] → [bugmon:update,bisected,confirmed]

Less reduced testcase that doesn't reproduce without evaluate but within reprl (for investigation):

//evaluate(`
function placeholder(){}
function main() {
const v3 = [13.37,13.37,13.37];
const v5 = [1337];
const v6 = {__proto__:v3,b:v5,constructor:v3,e:"-65537",length:1337,valueOf:v5};
const v7 = {b:-2,c:"-65537",constructor:13.37,toString:v6};
function v10(v11,v12) {
    try {
        const v15 = v10(-3399895606,v11,..."64",7);
        const v16 = (-65537).toLocaleString();
    } catch(v17) {
        const v18 = [v17,v12,v11,v7,...v3,v10];
    }
}
const v19 = v10();
gc();
}
main();
//`);
Flags: needinfo?(jdemooij)

I'm able to trigger the assertion already at the parent commit (8c52592cd838) of the patch stack from bug 1725379, so I think the regression range from comment #4 is wrong.

The first test case starts triggering the assertion with https://hg.mozilla.org/integration/autoland/rev/c4541cb343ae0ee222dc89f5f09d7758f503fd89 for me, which doesn't really make sense. The second test case asserts starting with the changes from bug 1688794, which seems like a more plausible candidate.

I'm unable to reproduce this (both central tip and the revision in comment 0) with these tests. I'll be off Thu/Fri this week but can look at this more on Monday...

I'm only able to reproduce with opt+debug. I don't get the assertion in non-opt debug builds. Both tests are using recursion, maybe that's also making it system dependent?

Using the rev from Comment 6 I'm now able to reproduce locally as well. I've simplified (for my local build at least) to:

function fn() {
    try {
        v15 = fn(..."  ");
        v16 = (0).toLocaleString();
    } catch(v17) {
        [...[0, 1]];
    }
}
fn();

Recursion, iterators, and intl-selfhosting seem to be required. The self-hosted stencil changes may be to blame here since they do special things when resolving some intl self-hosted code

I'm seeing surprising behaviour while debugging and I think this is a real regression from Bug 1688794.

Assignee: nobody → tcampbell

It could also be an WarpBuilder inlining issue, because I don't get an assertion when passing --ion-inlining=off.


It looks like somehow the numberFormatCache is wiped, because I get the following output after applying this patch:

Self-hosted JavaScript assertion info: [Latin 1]"./../builtin/Number.js:34: en-US -> undefined"
diff --git a/js/src/builtin/Number.js b/js/src/builtin/Number.js
--- a/js/src/builtin/Number.js
+++ b/js/src/builtin/Number.js
@@ -27,5 +27,9 @@ function Number_toLocaleString() {
         // locales and options.
-        if (!intl_IsRuntimeDefaultLocale(numberFormatCache.runtimeDefaultLocale)) {
+        var defaultLocale = numberFormatCache.runtimeDefaultLocale;
+        if (!intl_IsRuntimeDefaultLocale(defaultLocale)) {
             numberFormatCache.numberFormat = intl_NumberFormat(locales, options);
             numberFormatCache.runtimeDefaultLocale = intl_RuntimeDefaultLocale();
+        } else {
+          assert(defaultLocale === numberFormatCache.runtimeDefaultLocale,
+                 `${defaultLocale} -> ${numberFormatCache.runtimeDefaultLocale}`);
         }

If I add a re-entrancy check to GetComputedIntrinsic (which I should have added originally..), then the test case fails in debug builds. With the JITs we seem to trigger re-entrancy that does not occur without them. Still digging..

The good news is that this is not a JIT type-confusion bug. What is happening is that GetComputedIntrinsic can fail while running JS_ExecuteScript. In this test case, an unlucky stack-overflow is what causes the failure. This results in only some of the computed self-hosted values being initialized. As a result, when we later try to access one of the uninitialized values it causes GetComputedIntrinsic to run again which resets the runtimeDefaultLocale back to an empty object. This is occurs in the middle of running the format code which breaks things. I think that in practice the spectre XOR checks protect us by making this crash.

I'm not sure what the best fix is for this problem. A hack might be to have SetIntrinsic be a no-op if the value is already initialized. None of this is very elegant though.

Another approach might be to have SetIntrinsic store to a temporary object and then apply those values to the real holder after successfully executing the top-level.

Regressed by: 1688794
Has Regression Range: --- → yes
Flags: needinfo?(ccullen)
Component: JavaScript Engine: JIT → JavaScript Engine
Severity: -- → S3
Priority: -- → P1
Flags: needinfo?(jdemooij)

Capture SetIntrinsic values on a special computed-intrinsics-holder. If
running the top-level script fails (for OOM, etc), we can throw away the partial
results before they are exposed. Once the computed-intrinsics-holder if
successfully initialized, we can copy values to the normal intrinsics holder as
needed so there is only one cache to search. There are only about 20 values and
only ones that are actually used exist in both tables, so memory change is
minimal.

Attached file Bug 1727364 - Add testcase. r?jandem! (deleted) —

Depends on D123994

This bug results in a UndefinedValue being passed to a C++ internal helper method that assumes it receives ObjectValue. Due to our spectre mitigations, the JS::Value::toObject call will convert undefined into a nullptr (32-bit) or outside-address-space (64-bit) pointer that will derefence to a reliable crash. The regression was added to Fx92 and I would like to call this sec-low; mark 92 as wontfix; and land patch today-ish. The fix is a slight design change, so I'd rather ride trains than uplift if spectre mitigations are already protecting us. I will defer landing test case either way.

Andrew, is this reasonable?

Flags: needinfo?(continuation)

Sure, that sounds fine to me. Thanks for fixing this.

Flags: needinfo?(continuation)
Keywords: sec-low
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20210901034805-a14cf6b742de.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
Keywords: bugmon
Whiteboard: [bugmon:update,bisected,confirmed] → [bugmon:update,bisected,confirmed][adv-main93+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: