AddressSanitizer: heap-use-after-free [@ js::GeckoProfiler::beginPseudoJS] with READ of size 4 or Assertion failure: *size_ > 0, at vm/GeckoProfiler.cpp:316 with enableGeckoProfiling

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
7 months ago
4 months ago

People

(Reporter: decoder, Assigned: jonco)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla55
x86_64
Linux
assertion, crash, jsbugmon, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 months ago
The following testcase crashes on mozilla-central revision 8df9fabf2587 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2, run with --fuzzing-safe --ion-offthread-compile=off):

evalInWorker("enableGeckoProfiling()");



Backtrace:

==10808==ERROR: AddressSanitizer: heap-use-after-free on address 0x62d00006c2e8 at pc 0x00000169b134 bp 0x7f896319cd10 sp 0x7f896319cd08
READ of size 4 at 0x62d00006c2e8 thread T14
    #0 0x169b133 in js::GeckoProfiler::beginPseudoJS(char const*, void*) js/src/vm/GeckoProfiler.cpp:263:24
    #1 0x169b133 in js::AutoGeckoProfilerEntry::AutoGeckoProfilerEntry(JSRuntime*, char const*, js::ProfileEntry::Category) js/src/vm/GeckoProfiler.cpp:464
    #2 0x12d12c4 in js::gc::AutoTraceSession::AutoTraceSession(JSRuntime*, JS::HeapState) js/src/jsgc.cpp:5822:5
    #3 0x1d6ec46 in js::Nursery::doCollection(JS::gcreason::Reason, js::gc::TenureCountCache&) js/src/gc/Nursery.cpp:663:22
    #4 0x1d6db53 in js::Nursery::collect(JS::gcreason::Reason) js/src/gc/Nursery.cpp:590:25
    #5 0x12ddeb1 in js::gc::GCRuntime::minorGC(JS::gcreason::Reason, js::gcstats::Phase) js/src/jsgc.cpp:6821:5
    #6 0x12d6d68 in js::gc::GCRuntime::evictNursery(JS::gcreason::Reason) js/src/gc/GCRuntime.h:1367:9
    #7 0x12d6d68 in js::EvictAllNurseries(JSRuntime*, JS::gcreason::Reason) js/src/gc/Nursery-inl.h:81
    #8 0x12d6d68 in js::gc::GCRuntime::gcCycle(bool, js::SliceBudget&, JS::gcreason::Reason) js/src/jsgc.cpp:6398
    #9 0x12db479 in js::gc::GCRuntime::collect(bool, js::SliceBudget, JS::gcreason::Reason) js/src/jsgc.cpp:6596:25
    #10 0x12a96b4 in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:6662:5
    #11 0x17bbc01 in JSRuntime::destroyRuntime() js/src/vm/Runtime.cpp:321:9
    #12 0x11b6a17 in js::DestroyContext(JSContext*) js/src/jscntxt.cpp:228:9
    #13 0x593bcc in WorkerMain(void*)::$_2::operator()() const js/src/shell/js.cpp:3563:17
    #14 0x593bcc in mozilla::ScopeExit<WorkerMain(void*)::$_2>::~ScopeExit() /dist/include/mozilla/ScopeExit.h:112
    #15 0x593bcc in WorkerMain(void*) js/src/shell/js.cpp:3646
    #16 0x5a9e55 in void js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::callMain<0ul>(mozilla::IndexSequence<0ul>) js/src/threading/Thread.h:234:5
    #17 0x5a9e55 in js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::Start(void*) js/src/threading/Thread.h:227
    #18 0x7f89669206f9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76f9)
    #19 0x7f896578fb5c in clone /build/glibc-GKVZIf/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

0x62d00006c2e8 is located 32488 bytes inside of 32648-byte region [0x62d000064400,0x62d00006c388)
freed by thread T14 here:
    #0 0x5188d0 in __interceptor_free /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38
    #1 0x593bb8 in js_free(void*) /dist/include/js/Utility.h:257:5
    #2 0x593bb8 in void js_delete<ShellContext>(ShellContext const*) /dist/include/js/Utility.h:384
    #3 0x593bb8 in JS::DeletePolicy<ShellContext>::operator()(ShellContext const*) /dist/include/js/Utility.h:485
    #4 0x593bb8 in mozilla::UniquePtr<ShellContext, JS::DeletePolicy<ShellContext> >::reset(ShellContext*) /dist/include/mozilla/UniquePtr.h:343
    #5 0x593bb8 in mozilla::UniquePtr<ShellContext, JS::DeletePolicy<ShellContext> >::~UniquePtr() /dist/include/mozilla/UniquePtr.h:288
    #6 0x593bb8 in WorkerMain(void*) js/src/shell/js.cpp:3646
    #7 0x5a9e55 in void js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::callMain<0ul>(mozilla::IndexSequence<0ul>) js/src/threading/Thread.h:234:5
    #8 0x5a9e55 in js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::Start(void*) js/src/threading/Thread.h:227
    #9 0x7f89669206f9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76f9)

previously allocated by thread T14 here:
    #0 0x518c18 in __interceptor_malloc /srv/repos/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52
    #1 0x592d11 in js_malloc(unsigned long) /dist/include/js/Utility.h:229:12
    #2 0x592d11 in ShellContext* js_new<ShellContext, JSContext*&>(JSContext*&) /dist/include/js/Utility.h:346
    #3 0x592d11 in js::detail::UniqueSelector<ShellContext>::SingleObject js::MakeUnique<ShellContext, JSContext*&>(JSContext*&) /dist/include/js/UniquePtr.h:48
    #4 0x592d11 in WorkerMain(void*) js/src/shell/js.cpp:3577
    #5 0x5a9e55 in void js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::callMain<0ul>(mozilla::IndexSequence<0ul>) js/src/threading/Thread.h:234:5
    #6 0x5a9e55 in js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::Start(void*) js/src/threading/Thread.h:227
    #7 0x7f89669206f9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76f9)


SUMMARY: AddressSanitizer: heap-use-after-free js/src/vm/GeckoProfiler.cpp:263:24 in js::GeckoProfiler::beginPseudoJS(char const*, void*)
Shadow bytes around the buggy address:
  0x0c5a80005840: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c5a80005850: fd fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd fd
  0x0c5a80005860: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Freed heap region:       fd
==10808==ABORTING



Not sure why this is popping up right now, it might be on file somewhere else already. Judging from the stack it is very likely shell-only, but marking as fuzzblocker because it keeps popping up quite frequently.
(Reporter)

Comment 1

7 months ago
If you replace evalInWorker with evalInCooperativeThread, an assertion pops up instead on debug builds.
Summary: AddressSanitizer: heap-use-after-free [@ js::GeckoProfiler::beginPseudoJS] with READ of size 4 with enableGeckoProfiling → AddressSanitizer: heap-use-after-free [@ js::GeckoProfiler::beginPseudoJS] with READ of size 4 or Assertion failure: *size_ > 0, at vm/GeckoProfiler.cpp:316 with enableGeckoProfiling
Looking at the stack and the code, this seems to be a regression from bug 1337209. In WorkerMain, the ShellContext used to be destroyed after the JS_DestroyContext call but now we destroy it before that call.
Flags: needinfo?(sphink)

Updated

6 months ago
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:bisect]

Comment 3

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

Updated

6 months ago
Whiteboard: [fuzzblocker] [jsbugmon:bisect] → [fuzzblocker] [jsbugmon:]
Adding another needinfo.

Steve, Jon: this blocks fuzzing of some important shell functions (evalInWorker etc) so it would be great to have it fixed soon.
Flags: needinfo?(jcoppeard)
(Assignee)

Updated

5 months ago
Blocks: 1337209
Flags: needinfo?(jcoppeard)
(Assignee)

Comment 5

5 months ago
Created attachment 8870499 [details] [diff] [review]
bug1352507-profiler

The problem is that the shell context data structure owns the profiling stack memory and this runtime retains a pointer to it after the shell context is destroyed.

The patch only allows one shell context to set the profiling stack at a time and resets the profiling stack when the shell context dies.
Assignee: nobody → jcoppeard
Attachment #8870499 - Flags: review?(nfitzgerald)
Comment on attachment 8870499 [details] [diff] [review]
bug1352507-profiler

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

The patch looks sane to me given the description of the bug (thanks for that!) but I'm also not super familiar with this code. If you want more thorough review than this sanity check, I'd recommend asking for additional review from someone else.
Attachment #8870499 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 7

5 months ago
Comment on attachment 8870499 [details] [diff] [review]
bug1352507-profiler

I'm not particularly familiar with this stuff either and I don't know who is.  Jan would you be a good person to take a look?
Attachment #8870499 - Flags: review?(jdemooij)
(Assignee)

Comment 8

5 months ago
Created attachment 8872983 [details] [diff] [review]
bug1352507-profiler v2

This does the same as the patch in comment 5 but was updated following the changes in this area.

njn, I'm redirecting the review request to you since you seem to be a better person to look at this.
Attachment #8870499 - Attachment is obsolete: true
Attachment #8870499 - Flags: review?(jdemooij)
Flags: needinfo?(sphink)
Attachment #8872983 - Flags: review?(n.nethercote)
(Assignee)

Comment 9

5 months ago
Created attachment 8872989 [details] [diff] [review]
bug1352507-profiler v3

Updated to fix test failures.
Attachment #8872983 - Attachment is obsolete: true
Attachment #8872983 - Flags: review?(n.nethercote)
Attachment #8872989 - Flags: review?(n.nethercote)
Comment on attachment 8872989 [details] [diff] [review]
bug1352507-profiler v3

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

I'm happy to review this, but shu and djvj are also good reviewers for changes affecting the connection between Spidermonkey and the Gecko Profiler.

::: js/src/shell/js.cpp
@@ +393,5 @@
>  
> +ShellContext::~ShellContext()
> +{
> +    js_free(geckoProfilingStack);
> +}

This is allocated with js_new() but freed with js_free(), which means the PseudoStack destructor won't run. Please use js_delete() instead. Even better would be to use UniquePtr<PseudoStack, JS::DeletePolicy<PseudoStack>> for geckoProfilingStack, which avoids the need for the explicit ~ShellContext().
Attachment #8872989 - Flags: review?(n.nethercote) → review+

Comment 11

5 months ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20250e1b5b64
Reset the profiling stack when the shell context owning it dies r=njn

Comment 12

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/20250e1b5b64
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.