1817767 - Assertion failure: hasLatin1Chars(), at js/src/vm/StringType.h:1885

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20230220-dd93eb14eeca (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

function a(b) {
  arguments[b]
}
c = "aaaaaaaaaaaaaaaaaa111aaaa";
d = "foo" + c;
e = "bar" + d;
f = "\u1200" + d;
f.lastIndexOf();
a(e);

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555556f09712 in js::AtomizeString(JSContext*, JSString*) ()
#1  0x0000555556d43123 in js::GetObjectElementOperation(JSContext*, JSOp, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) ()
#2  0x0000555556d249c3 in Interpret(JSContext*, js::RunState&) ()
#3  0x0000555556d1daca in js::RunScript(JSContext*, js::RunState&) ()
#4  0x0000555556d32cee in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) ()
#5  0x0000555556d33231 in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) ()
#6  0x0000555556e5c1df in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) ()
#7  0x0000555556e5c41c in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) ()
#8  0x0000555556bf4c2d in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) ()
#9  0x0000555556bf3f67 in Process(JSContext*, char const*, bool, FileKind) ()
#10 0x0000555556b9290b in Shell(JSContext*, js::cli::OptionParser*) ()
#11 0x0000555556b8afe0 in main ()
rax	0x555555808958	93824995068248
rbx	0x3	3
rcx	0x5555582d9b98	93825039965080
rdx	0x0	0
rsi	0x7ffff6abd770	140737331844976
rdi	0x7ffff6abc540	140737331840320
rbp	0x7fffffffc120	140737488339232
rsp	0x7fffffffbd80	140737488338304
r8	0x7ffff6abd770	140737331844976
r9	0x7ffff7fe3840	140737354020928
r10	0x0	0
r11	0x0	0
r12	0x3	3
r13	0x1f	31
r14	0x2aac41400638	46919317456440
r15	0x7ffff572f100	140737311338752
rip	0x555556f09712 <js::AtomizeString(JSContext*, JSString*)+1938>
=> 0x555556f09712 <_ZN2js13AtomizeStringEP9JSContextP8JSString+1938>:	movl   $0x75d,0x0
   0x555556f0971d <_ZN2js13AtomizeStringEP9JSContextP8JSString+1949>:	callq  0x555556c1ef34 <abort>

Marking s-s as this assertion is known to be security-sensitive in some contexts.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

[Bugmon [:jkratzer for issues]]

Verified bug as reproducible on mozilla-central 20230220094640-dd93eb14eeca.
The bug appears to have been introduced in the following build range:

Start: accad456fe79798fe7524fd7d0ca45d3f29a66e2 (20230131230246)
End: 5b90fc9fed427a15a6cbc8466b612a30cd3c5f0b (20230131230339)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=accad456fe79798fe7524fd7d0ca45d3f29a66e2&tochange=5b90fc9fed427a15a6cbc8466b612a30cd3c5f0b

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Setting Regressed by field after analyzing regression range found by bugmon in comment #3.

Comment 5

[Nicolas B. Pierron [:nbp]]

Olli, can you Investigate this issue?
Based on comment 3, this might be related to the MruCache.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 7

[Ted Campbell [:tcampbell]]

This is discussed here, but the short answer is that the hasLatin1Chars flag on a JSRope means that we can linearize into a Latin1 string, and not the the current storage tree is purely Latin1.

  A   B
 / \ / \
d   C   e'
   / \
  f   g


A,B,C are JSRope
d,f,g are Latin1 linear string
e' is char16_t linear string

In this scenario, we have JSString A and B that have a common child node, C. The A string is Latin1, while the B string is overall char16_t because of e'. When a call to ensureLinear on B happens, we make a new flat char16_t string for B and then replace children with "dependent strings" that point to a substring of the new B. The issue is that C is now promoted to char16_t and A now needs to use LossyConvertUtf16toLatin1.

Comment 8

[Ted Campbell [:tcampbell]]

In release builds, we will end up copying chars out of a char16_t buffer so there are no bounds check issues. Certain strings will change their contents but still remain legal strings (since embedded null characters are allowed in JS). There are risks of breaking websites so we should ensure we uplift to beta as well.

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1817767 - Use CopyChars for rope atomization cache. r?jandem

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: Bug 1817767 - Add testcase. r?jandem

Comment 11

[Ted Campbell [:tcampbell]]

Landing to mozilla-central and will then request uplift to beta. Current release/esr channels are unaffected by this recent regression.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/096c85e62bc6

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Thanks Ted! (I was on vacation)

Hopefully hasLatin1Chars could be document to have this rather unexpected behavior.

Comment 14

[Bugmon [:jkratzer for issues]]

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

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:tcampbell, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Ted Campbell [:tcampbell]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #13)

Thanks Ted! (I was on vacation)

Hopefully hasLatin1Chars could be document to have this rather unexpected behavior.

I'll update the comments in the test patch which we can land in a few weeks.

Comment 17

[Ted Campbell [:tcampbell]]

Comment on attachment 9319528 [details]
Bug 1817767 - Use CopyChars for rope atomization cache. r?jandem

Beta/Release Uplift Approval Request

• User impact if declined: Subtle correctness bugs in JS that may break websites. It does not appear to be exploitable in general as a security issue.
• 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): Low risk change. Replacing a line of custom code with a pre-existing helper function used by similar cases already. This code is very hot so the "normal" case is continues to be very well tested and this patch additionally fixes the edge case. I see no new related crashes in crash-stats for nightly.
• String changes made/needed:
• Is Android affected?: Yes

Comment 18

[Donal Meehan [:dmeehan]]

Comment on attachment 9319528 [details]
Bug 1817767 - Use CopyChars for rope atomization cache. r?jandem

Approved for 111.0b7

Comment 19

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/552339945440