1463501 - Assertion failure: !IsInsideNursery(cell), at js/src/jit/VMFunctions.cpp:695

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision b75acf965293 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager --nursery-strings=on):

function f(x) {
    y = x.replace(/\s/g, " ");
    return true & y.match(/l/) & y.match(/.*/)
}
f("");
f("for (v       { for (let   for ");

Backtrace:

#0  0x0000557aff5102d0 in js::jit::PostWriteBarrier (rt=0x7f2988c19000, cell=0x7f2988b009e8) at js/src/jit/VMFunctions.cpp:695
#1  0x00003d72abe2b97e in ?? ()
#2  0x00007fff002bdd40 in ?? ()
#3  0x00007fff002bdd38 in ?? ()
#4  0x00007fff002bdd50 in ?? ()
/snip

For detailed crash information, see attachment.

Setting s-s because this contains GC stuff on the stack.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This has blown up jsfunfuzz with different crashes involving arena_dalloc, JS::Zone::~Zone, js::gc::TenuredCell::writeBarrierPre and js::gc::ArenaCellSet::putCell all likely related.

Comment 3

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 4

[Jon Coppeard (:jonco)]

Note this requires nursery strings enabled so is probably not a general problem.

Comment 5

[Steve Fink [:sfink] [:s:]]

Yummy fuzzbug. Thanks.

Looks like I still don't have the JIT postbarrier code logic quite right yet -- it seems to be trying to postbarrier a tenured -> tenured edge.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Fix string post barrier when writing string base field

Oops, I guess I should try actually running the code with nursery strings enabled.

Trivial fix. I would expect it to fail most of the time without this.

Comment 7

[Steve Fink [:sfink] [:s:]]

Oh, I meant to ask -- is there any way to avoid pushing all of the volatile MMX registers when calling PostWriteBarrier? I mean, I don't *know* that they won't be used. Is there some way to tell the compiler not to use them or something?

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8980146 [details] [diff] [review]
Fix string post barrier when writing string base field

Review of attachment 8980146 [details] [diff] [review]:
-----------------------------------------------------------------

Regarding volatile float registers: sadly there's no easy way to avoid that, AFAIK. Our best options are (1) ideally we'd add some kind of post-barrier trampoline where we can inline the most common post-barriers without calling into C++, a bit like what we did with the pre-barrier trampoline (bug 1419497 mentions slow pushing/popping of float registers as one of the reasons for adding that trampoline), (2) mark the post-barrier instruction as a call instruction so Ion's register allocator handles the spilling, or give the instruction a safepoint so we know the live registers. The latter might be more difficult for some cases.

Comment 9

[Jan de Mooij [:jandem]]

(So if you want to work on that: I think having a post-barrier trampoline would be cool. Maybe some fixed-size buffer where we can append N entries, or something. It would also eliminate some code bloat: instead of pushing registers, calling the post barrier, popping registers, we would just call the trampoline directly and it would take care of saving registers when it calls into C++.)

Comment 10

[Jan de Mooij [:jandem]]

(Oh the trampoline could look at the ArenaCellSet inline, similar to how the pre-barrier code checks for mark bits inline.. We'd want to collect some data on how frequently the "already in whole cell buffer" fast path would succeed/fail.)

Comment 11

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8980146 [details] [diff] [review]
Fix string post barrier when writing string base field

I'll request sec-approval, but I think this should be sec-none in the first place: it only crashes in a non-default configuration that requires either an environment variable to enable, or (JS shell only) passing a command-line option.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? not possible without also enabling nursery strings

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? none

How likely is this patch to cause regressions; how much testing does it need? it's a pretty obvious fix; it does dumb stuff left out, and skips the dumb stuff after adding the line.

Comment 12

[Andrew McCreight (out of office until 8/21) [:mccr8]]

If this is disabled by default on all branches, then you don't need sec-approval. And we could probably unhide the bug.

Comment 14

[Steve Fink [:sfink] [:s:]]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> If this is disabled by default on all branches, then you don't need
> sec-approval. And we could probably unhide the bug.

Yes, it is disabled by default everywhere it exists.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b865f2b5fb12
Fix string post barrier when writing string base field, r=jandem

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

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