Closed Bug 1506969 Opened 1 year ago Closed 1 year ago

Assertion failure: startOfChunkBytes < uncompressedBytes (chunk must refer to bytes not exceeding |uncompressedBytes|), at mozilla-central/js/src/vm/Compression.h:85

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: Waldo)

References

Details

(6 keywords, Whiteboard: [disclosure deadline Feb 12, 2019][post-critsmash-triage])

Attachments

(3 files, 1 obsolete file)

Found by Google's OSS-Fuzz, with 90 day disclosure:

[Environment] ASAN_OPTIONS = redzone=512:print_suppressions=0:strict_memcmp=0:allow_user_segv_handler=1:allocator_may_return_null=1:handle_sigfpe=1:handle_sigbus=1:detect_stack_use_after_return=0:alloc_dealloc_mismatch=0:print_scariness=1:max_uar_stack_size_log=16:detect_odr_violation=0:handle_sigill=1:use_sigaltstack=1:fast_unwind_on_fatal=1:detect_leaks=0:print_summary=1:handle_abort=1:check_malloc_usable_size=0:detect_container_overflow=1:symbolize=0:handle_segv=1
[Command line] /mnt/scratch0/clusterfuzz/slave-bot/builds/clusterfuzz-builds-no-engine_spidermonkey_6aad6e0d14f81d36f48dbd887aa56b38e87859f7/revisions/js --cpu-count=2 --disable-oom-functions --fuzzing-safe --ion-eager /mnt/scratch0/clusterfuzz/slave-bot/inputs/fuzzer-testcases/fuzz-15.js

991981: DataView.prototype.set* methods shouldn't misbehave horribly if index-argument conversion detaches the ArrayBuffer being modified
 PASSED!
Tests complete
BUGNUMBER: 2,4,8,16,32,2,4,8,16,32,2,4,8,16,32,2,4,8,16,32,2,4,8,16,32,
STATUS: TM: invalid results with setting a closure variable in a loop
STATUS: TM: invalid results with setting a closure variable in a loop
 FAILED!  TM: invalid results with setting a closure variable in a loop : Expected value '2,4,8,16,32,2,4,8,16,32,2,4,8,16,32,2,4,8,16,32,2,4,8,16,32,', Actual value '4,16,64,256,1024,4,16,64,256,1024,4,16,64,256,1024,4,16,64,256,1024,4,16,64,256,1024,'
BUGNUMBER: 57631
STATUS: Testing new RegExp(pattern,flag) with illegal pattern or flag
Assertion failure: startOfChunkBytes < uncompressedBytes (chunk must refer to bytes not exceeding |uncompressedBytes|), at mozilla-central/js/src/vm/Compression.h:85
AddressSanitizer:DEADLYSIGNAL
=================================================================
==3221==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55934cf171fe bp 0x7ffe8be4ef50 sp 0x7ffe8be4ee20 T0)
==3221==The signal is caused by a WRITE memory access.
==3221==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x55934cf171fd in js::Compressor::chunkSize(unsigned long, unsigned long) mozilla-central/js/src/vm/Compression.h:79:9
    #1 0x55934cf171fd in char16_t const* js::ScriptSource::chunkUnits<char16_t>(JSContext*, js::UncompressedSourceCache::AutoHoldEntry&, unsigned long) mozilla-central/js/src/vm/JSScript.cpp:1599
    #2 0x55934ced65b0 in char16_t const* js::ScriptSource::units<char16_t>(JSContext*, js::UncompressedSourceCache::AutoHoldEntry&, unsigned long, unsigned long) mozilla-central/js/src/vm/JSScript.cpp:1719:29
    #3 0x55934ced5f92 in js::ScriptSource::PinnedUnits<char16_t>::PinnedUnits(JSContext*, js::ScriptSource*, js::UncompressedSourceCache::AutoHoldEntry&, unsigned long, unsigned long) mozilla-central/js/src/vm/JSScript.cpp:1755:22
    #4 0x55934cd54960 in JSFunction::createScriptForLazilyInterpretedFunction(JSContext*, JS::Handle<JSFunction*>) mozilla-central/js/src/vm/JSFunction.cpp:1803:53
    #5 0x55934c56ef3a in JSFunction::getOrCreateScript(JSContext*, JS::Handle<JSFunction*>) mozilla-central/js/src/vm/JSFunction.h:540:18
    #6 0x55934c6ba44d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) mozilla-central/js/src/vm/Interpreter.cpp:563:10
    #7 0x55934df6840d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jit/BaselineIC.cpp:3688:14
    #7 0x12a4e6619722  (<unknown module>)
    #8 0x62100077b387  (<unknown module>)
    #9 0x12a4e6607b03  (<unknown module>)
    #8 0x55934e6f5b74 in EnterJit(JSContext*, js::RunState&, unsigned char*) mozilla-central/js/src/jit/Jit.cpp:103:9
    #9 0x55934e6f5b74 in js::jit::MaybeEnterJit(JSContext*, js::RunState&) mozilla-central/js/src/jit/Jit.cpp:170
    #10 0x55934c67ff18 in js::RunScript(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:432:34
    #11 0x55934c6ba694 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) mozilla-central/js/src/vm/Interpreter.cpp:587:15
    #12 0x55934df6840d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jit/BaselineIC.cpp:3688:14
    #14 0x12a4e6619722  (<unknown module>)
    #15 0x62100077456f  (<unknown module>)
    #16 0x12a4e6607b03  (<unknown module>)
    #13 0x55934e6f5b74 in EnterJit(JSContext*, js::RunState&, unsigned char*) mozilla-central/js/src/jit/Jit.cpp:103:9
    #14 0x55934e6f5b74 in js::jit::MaybeEnterJit(JSContext*, js::RunState&) mozilla-central/js/src/jit/Jit.cpp:170
    #15 0x55934c67ff18 in js::RunScript(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:432:34
    #16 0x55934c6ba694 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) mozilla-central/js/src/vm/Interpreter.cpp:587:15
    #17 0x55934df6840d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jit/BaselineIC.cpp:3688:14
    #21 0x12a4e6619722  (<unknown module>)
    #22 0x621000708eb7  (<unknown module>)
    #23 0x12a4e6607b03  (<unknown module>)
    #18 0x55934e6f5b74 in EnterJit(JSContext*, js::RunState&, unsigned char*) mozilla-central/js/src/jit/Jit.cpp:103:9
    #19 0x55934e6f5b74 in js::jit::MaybeEnterJit(JSContext*, js::RunState&) mozilla-central/js/src/jit/Jit.cpp:170
    #20 0x55934c67ff18 in js::RunScript(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:432:34
    #21 0x55934c6ba694 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) mozilla-central/js/src/vm/Interpreter.cpp:587:15
    #22 0x55934df6840d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jit/BaselineIC.cpp:3688:14
    #28 0x12a4e6619722  (<unknown module>)
    #29 0x6210003fbb2f  (<unknown module>)
    #30 0x12a4e6607b03  (<unknown module>)
    #23 0x55934e6f5b74 in EnterJit(JSContext*, js::RunState&, unsigned char*) mozilla-central/js/src/jit/Jit.cpp:103:9
    #24 0x55934e6f5b74 in js::jit::MaybeEnterJit(JSContext*, js::RunState&) mozilla-central/js/src/jit/Jit.cpp:170
    #25 0x55934c67ff18 in js::RunScript(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:432:34
    #26 0x55934c6c0525 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) mozilla-central/js/src/vm/Interpreter.cpp:813:15
    #27 0x55934c6c135b in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) mozilla-central/js/src/vm/Interpreter.cpp:845:12
    #28 0x55934c9e9c04 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:394:12
    #29 0x55934c9ea08f in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) mozilla-central/js/src/vm/CompilationAndEvaluation.cpp:429:12
    #30 0x55934c54c14a in RunFile(JSContext*, char const*, _IO_FILE*, bool) mozilla-central/js/src/shell/js.cpp:922:14
    #31 0x55934c54c14a in Process(JSContext*, char const*, bool, FileKind) mozilla-central/js/src/shell/js.cpp:1399
    #32 0x55934c4ce6bb in ProcessArgs(JSContext*, js::cli::OptionParser*) mozilla-central/js/src/shell/js.cpp:10336:14
    #33 0x55934c4ce6bb in Shell(JSContext*, js::cli::OptionParser*, char**) mozilla-central/js/src/shell/js.cpp:10787
    #34 0x55934c4ce6bb in main mozilla-central/js/src/shell/js.cpp:11297
    #35 0x7f553b19a82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/libc-start.c:291
NI Waldo because this code changed in bug 1499192.
Flags: needinfo?(jwalden+bmo)
Given the assertion calling this sec-high out of caution, but maybe we catch it in the following code. Will wait for Waldo's feedback.
Whiteboard: [disclosure deadline Feb 12, 2019]
Hrm.

The Fickle Finger of Fate almost certainly points squarely at bug 1499192 -- no dependency added yet because I'm not 100% sure, and because this is a security bug and I prefer paranoia about revealing issues -- given that change touched the function/assertion in play here.

Gotta say, tho, it's super-non-obvious how those changes are wrong.  Investigating still...
Flags: needinfo?(jwalden+bmo)
When we're computing firstChunk/lastChunk and firstChunkOffset/lastChunkOffset in ScriptSource::units, bug 1499192 changed the provided overall end-offset from (begin + len - 1) to (begin + len).

The problem is, when the begin/end correspond exactly to a chunk boundary -- and in this script, the function source being decompressed ends *exactly* at a 64K units boundary -- the lastChunk now points one later.  And lastChunk is supposed to be *inclusive*.

This is a bit of a mess.  Sigh.  Very fixable for sure, but finicky to get right.

I think there might not be consequences to this beyond the assertion, because a count of units to copy ends up being zero here.  But I'm not absolutely certain of that, and I'll have to take a look at it carefully before saying for sure.
Attached patch Patch (obsolete) — Splinter Review
So actually, it looks like the regression landed this cycle, so we should strike while the iron is hot and fix this before it metastasizes and we need sec-approval and all that goo.  Mind expediting this particular review, Ted?

The changes here are a little wider than they absolutely needed to be, but I find this writeup of stuff a ton clearer than the old way that intermingled the special handling of first and last chunk inside the One Loop.  Hard for me to see how anyone could disagree!

With this patch in place, there is exactly one use of Compressor::chunkSize.  It was not immediately clear how that use could most simply be replaced, to allow that function to be removed.  I timed out thinking about it.
Attachment #9027720 - Flags: review?(tcampbell)
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Comment on attachment 9027720 [details] [diff] [review]
Patch

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

::: js/src/vm/Compression.h
@@ +73,5 @@
> +    static void
> +    rangeToChunkAndOffset(size_t uncompressedStart, size_t uncompressedLimit,
> +                          size_t* firstChunk, size_t* firstChunkOffset, size_t* firstChunkSize,
> +                          size_t* lastChunk, size_t* lastChunkSize)
> +    {

// NOTE: chunkSize is ignored if first and last chunk are the same.

@@ +82,5 @@
> +        MOZ_ASSERT(uncompressedStart < uncompressedLimit,
> +                   "subtraction below requires a non-empty range");
> +
> +        *lastChunk = (uncompressedLimit - 1) / CHUNK_SIZE;
> +        *lastChunkSize = uncompressedLimit % CHUNK_SIZE;

This computes a lastChunkSize in range [0, CHUNK_SIZE).

I believe this should be:

> *lastChunk = (uncompressedLimit - 1) / CHUNK_SIZE
> size_t lastChunkOffset = (uncompressedLimit - 1) / CHUNK_SIZE;
> *lastChunkSize = lastChunkOffset + 1;

::: js/src/vm/JSScript.cpp
@@ +1703,5 @@
>  
>      MOZ_ASSERT(data.is<Compressed<Unit>>());
>  
> +    // Determine first/last chunks, the offset (in bytes) into the first chunk
> +    // of the requested units, and the number of bytes in the last unit.

s/last unit/last chunk/
Attachment #9027720 - Flags: review?(tcampbell)
(In reply to Ted Campbell [:tcampbell] from comment #6)
> I believe this should be:
> 
> > *lastChunk = (uncompressedLimit - 1) / CHUNK_SIZE
> > size_t lastChunkOffset = (uncompressedLimit - 1) / CHUNK_SIZE;
> > *lastChunkSize = lastChunkOffset + 1;

Pretty sure

  *lastChunkSize = ((uncompressedLimit - 1) % CHUNK_SIZE) + 1;

is what is wanted here.

Will get an updated patch once I can spin up some automated testcases for this stuff.  Clearly these different boundary conditions demand testing, ideally for 1/2/3-chunk-sized ranges for completeness.
(In reply to Jeff Walden [:Waldo] from comment #7)
>   *lastChunkSize = ((uncompressedLimit - 1) % CHUNK_SIZE) + 1;

Yeah, that's the one..
Feh, winging it without tests isn't gonna work here given I fail at writing a patch to fix this.  Have a bunch here.

This does the hacky thing of just GCing twice to "assure" compression.

Some number of these tests fail, if I just land this test without making any other changes.  With the next patch, they all pass.

The plan as far as CharT goes is to enable all this for UTF-8 once a salient UTF-8 entrypoint is added and UTF-8 is actually tested interestingly, but as of now there is no such entrypoint.  I will probably add one quite soon.
Attachment #9028575 - Flags: review?(tcampbell)
This fixes the last-chunk-size issue for boundary chunks, and it also fixes AutoHoldEntry mistakes that were made (but, uh, not not not not not not not not by me).
Attachment #9028578 - Flags: review?(tcampbell)
Attachment #9027720 - Attachment is obsolete: true
Priority: -- → P2
Comment on attachment 9028578 [details] [diff] [review]
Patch that actually does stuff right

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

Pretty easy to follow now.
Attachment #9028578 - Flags: review?(tcampbell) → review+
Priority: P2 → P1
Comment on attachment 9028575 [details] [diff] [review]
Add a jsapi-test for script source compression working correctly, with scripts around, crossing, and exactly matching compression chunk boundaries in various ways

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

What is tested seems reasonable. Adding a case for more-than-three chunks seems a wise precaution since it is easy. Comments are all nits about making code more readable (to me).

::: js/src/jsapi-tests/testScriptSourceCompression.cpp
@@ +81,5 @@
> +        return nullptr;
> +    }
> +
> +    JS::Rooted<JS::Value> rval(cx);
> +    if (!JS::EvaluateUtf8(cx, options, &functionName, 1, &rval)) {

Let's use a |char16_t functionNameStr[] = { functionName }| for more boring-ness. I'd like to reduce the variety of APIs we use in the test that are not themselves the target of test.

@@ +99,5 @@
> +
> +    // Hoodoo voodoo that presently compresses |fun|'s source.
> +    // XXX Be a mensch: replace this with targeted actions to do this!
> +    JS_GC(cx);
> +    JS_GC(cx);

Seems like the best approach for today.

@@ +157,5 @@
> +    }
> +
> +    JS::AutoAssertNoGC nogc(cx);
> +
> +    auto CheckContents = [&functionName, &len](const auto* chars) {

[functionName, len]

@@ +165,5 @@
> +                          FunctionStart + FunctionNameOffset + 1) &&
> +               std::all_of(chars + FunctionStartLength, chars + len - FunctionEndLength,
> +                          [](auto c) { return c == FillerWhitespace; }) &&
> +               std::equal(chars + len - FunctionEndLength, chars + len,
> +                          FunctionEnd);

// Check function in parts. "function ", "A", "() {", "\n\n\n", "return 42; }"

@@ +194,5 @@
> +    constexpr size_t len = MinimumCompressibleLength + 55;
> +    auto source = MakeSourceAllWhitespace<CharT>(cx, len);
> +    CHECK(source);
> +
> +    // Write out an 'a' or 'b' function that is long enough to be compressed,

You generate a 'b' or 'c' function..

@@ +243,5 @@
> +    CompressSourceSync(fun, cx);
> +
> +    JS::Rooted<JSString*> str(cx, DecompressSource(cx, fun));
> +    CHECK(str);
> +    CHECK(IsExpectedFunctionString(str, FunctionName, cx));

A bunch of this run() code should be shared in a helper function to make the unique part of the test more obvious. Main parameters seem to be funStart, funEnd, sourceEnd (and then __FUNCTION__, FunctionName I guess).

@@ +403,5 @@
> +template<typename CharT>
> +bool
> +run()
> +{
> +    // Exactly three chunks.

Might as well also have a more than 3 chunks test even if the loop for middle chunks is pretty boring.
Attachment #9028575 - Flags: review?(tcampbell) → review+
Landed the fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b26e28ce8cfebb91f1c5764606585b8b670938ff

Debating whether to land the test now -- before the tree style gets broken universally and possibly breaks all my patches -- or land it after and hope the rebase is not too much pain.  Ordinarily I tend to paranoia, even for trunk-only things, as a nicety to nightly users who haven't restarted recently.  But it is just a nicety, and that benefit to them seems possibly outweighed by the convenience to me  and the knowledge that nightly builds are offered on a best-effort basis and no more (and also who's going to be targeting nightly-only sec issues anyway?).

Probably I will land it tonight, before the closure.  But no guarantees, and I have another chore I can deal with now and punt on deciding.  :-)
https://hg.mozilla.org/mozilla-central/rev/b26e28ce8cfe made it to central.

The other change https://hg.mozilla.org/integration/mozilla-inbound/rev/5e65de3569fe got backed for causing spidermonkey bustages on testScriptSourceCompression.cpp:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Crunnable&group_state=expanded&revision=5e65de3569fe33856451396ab77f78ba9c03a8c8
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=214786926&repo=mozilla-inbound
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/js/src/jsapi-tests/testScriptSourceCompression.cpp:110:5 in CompressSourceSync(JS::Handle<JSFunction*>, JSContext*)

Leaving the test open because of the backout.
Flags: needinfo?(jwalden+bmo)
Looks like JS_GC(cx) twice does *not* guarantee compression has occurred in all cases.  Bah.  What the test tests, should hold whether or not compression has happened, so I relanded with the assertion commented out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad63f8b4f0cd9d460dd6368d19fc78f776f3b65b

I filed bug 1511595 to make the test reliably guarantee compression such that the assert can be reenabled.
Flags: needinfo?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/ad63f8b4f0cd
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: qe-verify-
Whiteboard: [disclosure deadline Feb 12, 2019] → [disclosure deadline Feb 12, 2019][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.