1589002 - Hit MOZ_CRASH(*** Zone mismatch 0x7f7ec2bc3000 vs. 0x7f7ec2527000 at argument 0) at js/src/vm/JSContext-inl.h:49

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

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

The following testcase crashes on mozilla-central revision 4e1f8ca63a68 (build with --enable-debug --disable-optimize, run with --fuzzing-safe --no-threads --no-baseline --no-ion --more-compartments):

s = evalcx("");
s["evaluate"] = this["evaluate"];
evalcx('x = "12"; x += "3"; evaluate("", ({\
    global: this, \
    sourceMapURL: x \
}));', s);

Backtrace:

#0  0x000055e5865c7ee2 in MOZ_Crash (aFilename=0x55e584ff2d5e "js/src/vm/JSContext-inl.h", aLine=49, aReason=0x55e5888dd380 <sPrintfCrashReason> "*** Zone mismatch 0x7f7ec2bc3000 vs. 0x7f7ec2527000 at argument 0") at /home/ubuntu/shell-cache/js-dbg-optDisabled-64-linux-x86_64-4e1f8ca63a68/objdir-js/dist/include/mozilla/Assertions.h:332
#1  js::ContextChecks::fail (z1=0x7f7ec2bc3000, z2=0x7f7ec2527000, argIndex=0) at js/src/vm/JSContext-inl.h:48
#2  0x000055e5865c7dc3 in js::ContextChecks::check (this=0x7ffd9fc01640, z=0x7f7ec2527000, argIndex=0) at js/src/vm/JSContext-inl.h:66
#3  0x000055e5865c7a5f in js::ContextChecks::check (this=0x7ffd9fc01640, str=0xd5a1100698, argIndex=0) at js/src/vm/JSContext-inl.h:101
#4  0x000055e5867a528e in JSContext::checkImpl<JSString*> (this=0x7f7ec2d27000, argIndex=0, head=@0x7ffd9fc016d0: 0xd5a1100698) at js/src/vm/JSContext-inl.h:185
#5  0x000055e586783b6c in JSContext::check<JSString*> (this=0x7f7ec2d27000, args=@0x7ffd9fc016d0: 0xd5a1100698) at js/src/vm/JSContext-inl.h:193
#6  0x000055e586751bb5 in JS_CopyStringCharsZ (cx=0x7f7ec2d27000, str=0xd5a1100698) at js/src/jsapi.cpp:4394
/snip

For detailed crash information, see attachment.

Setting s-s as upcoming opt ASan stack seems to be scary.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: opt ASan stack

Comment 3

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/97a0a8d87329
user: Jan de Mooij
date: Mon Oct 14 20:38:30 2019 +0000
summary: Bug 1586991 part 15 - Use JS_CopyStringCharsZ in a number of places that need null-terminated strings. r=jwalden

Jan, is bug 1586991 a likely regressor?

Comment 4

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

Pernosco session:

https://pernos.co/debug/Su89co4HzvASzOqGriuoVQ/index.html

Please give feedback as to how this Pernosco session helps you, if possible?

Comment 5

[Jeff Walden [:Waldo]]

This is easily reproducible, and it's in highly-understandable straight-line code. Pernosco doesn't add any special value here.

The problem is, inside the evaluate function, we get the various properties off the options object as strings or what-have-you. Then, to set up the newly-created global and its associated data, we go into code like this:

  {
    JSAutoRealm ar(cx, global);
    RootedScript script(cx);

    // ...irrelevant code without this problem omitted...

    if (displayURL && !script->scriptSource()->hasDisplayURL()) {
      UniqueTwoByteChars chars = JS_CopyStringCharsZ(cx, displayURL);
      if (!chars) {
        return false;
      }
      if (!script->scriptSource()->setDisplayURL(cx, std::move(chars))) {
        return false;
      }
    }
    if (sourceMapURL && !script->scriptSource()->hasSourceMapURL()) {
      UniqueTwoByteChars chars = JS_CopyStringCharsZ(cx, sourceMapURL);
      if (!chars) {
        return false;
      }
      if (!script->scriptSource()->setSourceMapURL(cx, std::move(chars))) {
        return false;
      }
    }

In other words, we have to enter the new global's realm, then we do stuff. But that's a different compartment, different zone, and so when we try to act on sourceMapURL here (set before we enter the new realm) we fail compartment checks. (A similar problem exists for displayURL, obviously.)

JS_CopyStringCharsZ could just not do a compartment check if it wanted -- it just wants to create a null-terminated string of the string chars, and I don't believe it's problematic to perform its actions across a compartment boundary. I'll try to get a double-check on that before posting a patch.

Comment 6

[Jeff Walden [:Waldo]]

...or jandem can get on this tomorrow when he shows up, seeing as he probably overlaps with jonco's timeline better than I am right now and can get a sanity-check on accessing string characters across realms. :-) But if he doesn't get to it tomorrow before I show up, I'll get to it.

Comment 7

[Jeff Walden [:Waldo]]

This is a problem only in evaluate, i.e. it's shell-only and not a security problem. (And also it's probably just an unnecessary assertion so it's not covering anything up anyway.)

Comment 8

[Jan de Mooij [:jandem]]

I agree with Waldo, this looks like an overzealous assertion. I thought so when I saw the bug yesterday and decided to fix worse issues elsewhere first :-)

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1589002 - Remove overzealous assertion in JS_CopyStringCharsZ. r?jonco!

I added this API to copy a string's character to a new buffer, but we
can call this on strings from a different Zone. This should be fine because
ensureLinear doesn't use the context's Zone.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/add4a5be7b93
Remove overzealous assertion in JS_CopyStringCharsZ. r=jonco

Comment 11

[Cristina Coroiu [:ccoroiu]]

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