Closed Bug 1589002 Opened Last month Closed Last month

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

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- fixed

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 1 open bug, Regression)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(3 files)

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.

Attached file opt ASan stack

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?

Flags: needinfo?(jdemooij)
Regressed by: 1586991

Pernosco session:

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

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

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.

...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.

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.)

Group: javascript-core-security

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 :-)

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.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Priority: -- → P1
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/add4a5be7b93
Remove overzealous assertion in JS_CopyStringCharsZ. r=jonco
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.