Closed Bug 1666285 Opened 4 years ago Closed 4 years ago

AddressSanitizer: use-after-poison [@ __asan_memcpy | mozilla::dom::CropAndCopyDataSourceSurface] with READ of size 8

Categories

(Core :: Graphics: Canvas2D, defect, P1)

Product:
Core
Component:
Graphics: Canvas2D
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
firefox-esr78 85+ fixed
firefox83 --- wontfix
firefox84 --- wontfix
firefox85 + fixed
firefox86 + fixed

People

(Reporter: truber, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [sec-survey][adv-main85+r][adv-esr78.7+r])

Attachments

(2 files, 1 obsolete file)

Bug 1666285, try to avoid slow shrinking GC during sync calls
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr78+
tjr
: sec-approval+
Details | Review
test_reduced.zip
14.47 KB, application/x-zip-compressed
Details

Crash observed while fuzzing mozilla-central rev a7cd0e635744. Testcase is still reducing; I'll add it as soon as it's finished.

==1063522==ERROR: AddressSanitizer: use-after-poison on address 0x197cfa821bc0 at pc 0x55bfee0e2ca7 bp 0x7ffefc3fbff0 sp 0x7ffefc3fb7b8
READ of size 8 at 0x197cfa821bc0 thread T0 (Web Content)
    #0 0x55bfee0e2ca6 in __asan_memcpy /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3
    #1 0x7fd9e0432f3c in mozilla::dom::CropAndCopyDataSourceSurface(mozilla::gfx::DataSourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /builds/worker/checkouts/gecko/dom/canvas/ImageBitmap.cpp:251:7
    #2 0x7fd9e0434272 in CreateSurfaceFromRawData /builds/worker/checkouts/gecko/dom/canvas/ImageBitmap.cpp:298:7
    #3 0x7fd9e0434272 in mozilla::dom::CreateImageFromRawData(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned int, mozilla::gfx::SurfaceFormat, unsigned char*, unsigned int, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&) /builds/worker/checkouts/gecko/dom/canvas/ImageBitmap.cpp:313:39
    #4 0x7fd9e046c640 in mozilla::dom::CreateImageFromRawDataInMainThreadSyncTask::MainThreadRun() /builds/worker/checkouts/gecko/dom/canvas/ImageBitmap.cpp:390:35
    #5 0x7fd9e2341bf0 in mozilla::dom::WorkerMainThreadRunnable::Run() /builds/worker/checkouts/gecko/dom/workers/WorkerRunnable.cpp:574:20
    #6 0x7fd9dab35753 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() /builds/worker/checkouts/gecko/xpcom/threads/ThrottledEventQueue.cpp:254:22
    #7 0x7fd9dab2874f in mozilla::ThrottledEventQueue::Inner::Executor::Run() /builds/worker/checkouts/gecko/xpcom/threads/ThrottledEventQueue.cpp:81:15
    #8 0x7fd9dab29eb9 in mozilla::RunnableTask::Run() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:244:16
    #9 0x7fd9daae8d53 in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:514:26
    #10 0x7fd9daae6737 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:373:15
    #11 0x7fd9daae6b8d in mozilla::TaskController::ProcessPendingMTTask(bool) /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:170:36
    #12 0x7fd9dab37a51 in operator() /builds/worker/checkouts/gecko/xpcom/threads/TaskController.cpp:84:37
    #13 0x7fd9dab37a51 in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() /builds/worker/workspace/obj-build/dist/include/nsThreadUtils.h:577:5
    #14 0x7fd9dab0c163 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1234:14
    #15 0x7fd9dab1625c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:513:10
    #16 0x7fd9dbdd435f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:87:21
    #17 0x7fd9dbcd8b51 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #18 0x7fd9dbcd8b51 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #19 0x7fd9dbcd8b51 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #20 0x7fd9e2a55007 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:137:27
    #21 0x7fd9e67511df in XRE_RunAppShell() /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:913:20
    #22 0x7fd9dbcd8b51 in RunInternal /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:334:10
    #23 0x7fd9dbcd8b51 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:327:3
    #24 0x7fd9dbcd8b51 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:309:3
    #25 0x7fd9e675077c in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/checkouts/gecko/toolkit/xre/nsEmbedFunctions.cpp:744:34
    #26 0x55bfee11601d in content_process_main(mozilla::Bootstrap*, int, char**) /builds/worker/checkouts/gecko/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #27 0x55bfee116457 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:304:18
    #28 0x7fd9f6efe151 in __libc_start_main (/usr/lib/libc.so.6+0x28151)
    #29 0x55bfee0699b9 in _start (/home/truber/builds/m-c-20200921141232-fuzzing-asan-opt/firefox+0x5c9b9)

Address 0x197cfa821bc0 is a wild pointer.
SUMMARY: AddressSanitizer: use-after-poison /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cpp:22:3 in __asan_memcpy
Shadow bytes around the buggy address:
  0x03301f4fc320: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc330: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc340: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc350: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc360: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
=>0x03301f4fc370: f7 f7 f7 f7 f7 f7 f7 f7[f7]f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc380: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc390: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc3a0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc3b0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x03301f4fc3c0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==1063522==ABORTING
Flags: needinfo?(jschwartzentruber)

Testcase reproduces, but is too flaky to be reduced. Tyson, can you add a Pernosco trace?

Flags: needinfo?(jschwartzentruber) → needinfo?(twsmith)
Keywords: sec-high

This test case seems to trigger hundreds of process to launch and is breaking Pernosco at the moment. I have no idea what would cause this. Jason have you added anything to the fuzzer that might trigger this behavior?

Flags: needinfo?(jkratzer)
Attached file test_reduced.zip (obsolete) —

Adding the semi-reduced testcase, since Pernosco can't handle it, and further reduction is not practical.

This is a multi-part testcase, and can be replayed using Grizzly using something like: python -m grizzly.replay --xvfb --repeat 3 --relaunch 1 /path/to/firefox test_reduce.zip

The issue with Pernosco may be audio related. I got a message from khuey saying that it appears that the extra processes are attempting to exec /usr/bin/pulseaudio, failing with ENOENT, and then exiting. I can log another issue using this test case once we get this fixed I guess.

Installing pulseaudio unblocked me.

A Pernosco session is available here: https://pernos.co/debug/hu2RvBKtINoPbOhVLwEHgw/index.html

Flags: needinfo?(twsmith)
Flags: needinfo?(jkratzer)
Group: dom-core-security → gfx-core-security
Blocks: gfx-triage

Tyson, can you refresh the pernosco session?

Flags: needinfo?(twsmith)

Should be good to go now.

Flags: needinfo?(twsmith)
Severity: -- → S3
Priority: -- → P3

Seeing the ImageBitmap in the call stack, looks like something Andrea could be interested in

Flags: needinfo?(amarchesini)

This seems to be the stack where the memory pointed by srcBufferPtr is poisoned:

#0  __memset_avx2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:201
#1  0x0000563d7fc0f6d4 in __asan_poison_memory_region () at /builds/worker/fetches/llvm-project/llvm/projects/compiler-rt/lib/asan/asan_poisoning.cpp:140
#2  0x00007f66d3fa0da0 in SetMemCheckKind (ptr=0x373119821040, bytes=4032, kind=MemCheckKind::MakeNoAccess) at /home/twsmith/code/mozilla-central/js/src/util/Poison.h:113
#3  js::AlwaysPoison (ptr=0x373119821040, value=73 'I', num=4032, kind=MemCheckKind::MakeNoAccess) at /home/twsmith/code/mozilla-central/js/src/util/Poison.h:152
#4  0x00007f66d3fb7e6b in js::gc::GCRuntime::clearRelocatedArenasWithoutUnlocking (this=0x6290000dc6f8, arenaList=0x373119869000, reason=JS::GCReason::DOM_WORKER, lock=...) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:2667
#5  0x00007f66d3fb7d52 in js::gc::GCRuntime::clearRelocatedArenas (this=0x6290000dc6f8, arenaList=0x373119855000, reason=JS::GCReason::DOM_WORKER) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:2643
#6  0x00007f66d3fd5f8f in js::gc::GCRuntime::compactPhase (this=0x6290000dc6f8, reason=JS::GCReason::DOM_WORKER, sliceBudget=..., session=...) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:6397
#7  0x00007f66d3fd98b8 in js::gc::GCRuntime::incrementalSlice (this=0x6290000dc6f8, budget=..., gckind=..., reason=JS::GCReason::DOM_WORKER, session=...) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:6875
#8  0x00007f66d3fdc197 in js::gc::GCRuntime::gcCycle (this=0x6290000dc6f8, nonincrementalByAPI=true, budget=..., gckind=..., reason=JS::GCReason::DOM_WORKER) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:7238
#9  0x00007f66d3fde8c3 in js::gc::GCRuntime::collect (this=0x6290000dc6f8, nonincrementalByAPI=true, budget=..., gckindArg=..., reason=JS::GCReason::DOM_WORKER) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:7473
#10 0x00007f66d3f9e274 in js::gc::GCRuntime::gc (this=0x6290000dc6f8, gckind=GC_SHRINK, reason=JS::GCReason::DOM_WORKER) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:7549
#11 0x00007f66d3fe4703 in JS::NonIncrementalGC (cx=0x61d000190080, gckind=GC_SHRINK, reason=JS::GCReason::DOM_WORKER) at /home/twsmith/code/mozilla-central/js/src/gc/GC.cpp:8380
#12 0x00007f66c822bf5a in mozilla::dom::WorkerPrivate::GarbageCollectInternal (this=0x61b000074d80, aCx=0x61d000190080, aShrinking=true, aCollectChildren=false) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:4882
#13 0x00007f66c821df33 in mozilla::dom::(anonymous namespace)::IdleGCTimerCallback (aTimer=0x6030004e95b0, aClosure=0x61b000074d80) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:634
#14 0x00007f66ba2b7d1d in nsTimerImpl::Fire (this=0x60d0000b0060, aGeneration=276) at /home/twsmith/code/mozilla-central/xpcom/threads/nsTimerImpl.cpp:562
#15 0x00007f66ba2b734a in nsTimerEvent::Run (this=0x6210000c0960) at /home/twsmith/code/mozilla-central/xpcom/threads/TimerThread.cpp:251
#16 0x00007f66c81d0557 in mozilla::dom::(anonymous namespace)::WrappedControlRunnable::WorkerRun (this=0x606000063980, aCx=0x61d000190080, aWorkerPrivate=0x61b000074d80) at /home/twsmith/code/mozilla-central/dom/workers/WorkerEventTarget.cpp:36
#17 0x00007f66c8237cda in mozilla::dom::WorkerRunnable::Run (this=0x606000063980) at /home/twsmith/code/mozilla-central/dom/workers/WorkerRunnable.cpp:370
#18 0x00007f66c8219a65 in mozilla::dom::WorkerPrivate::ProcessAllControlRunnablesLocked (this=0x61b000074d80) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:3530
#19 0x00007f66c822288f in mozilla::dom::WorkerPrivate::RunCurrentSyncLoop (this=0x61b000074d80) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:3908
#20 0x00007f66c5250443 in mozilla::dom::AutoSyncLoopHolder::Run (this=0x7f669f258180) at /home/twsmith/code/mozilla-central/objdir-ff-ubsan/dist/include/mozilla/dom/WorkerPrivate.h:1367
#21 0x00007f66c8200987 in mozilla::dom::WorkerMainThreadRunnable::Dispatch (this=0x60b0000f8600, aFailStatus=mozilla::dom::Canceling, aRv=...) at /home/twsmith/code/mozilla-central/dom/workers/WorkerRunnable.cpp:556
#22 0x00007f66c42d34d6 in mozilla::dom::ImageBitmap::CreateInternal (aGlobal=0x6130001107f8, aImageData=..., aCropRect=..., aRv=...) at /home/twsmith/code/mozilla-central/dom/canvas/ImageBitmap.cpp:905
#23 0x00007f66c42d5c23 in mozilla::dom::ImageBitmap::Create (aGlobal=0x6130001107f8, aSrc=..., aCropRect=..., aRv=...) at /home/twsmith/code/mozilla-central/dom/canvas/ImageBitmap.cpp:1249
#24 0x00007f66c823faa4 in mozilla::dom::WorkerGlobalScope::CreateImageBitmap (this=0x613000110780, aImage=..., aRv=...) at /home/twsmith/code/mozilla-central/dom/workers/WorkerScope.cpp:566
#25 0x00007f66c33aead7 in mozilla::dom::WorkerGlobalScope_Binding::createImageBitmap (cx_=0x61d000190080, obj=..., void_self=0x613000110780, args=...) at WorkerGlobalScopeBinding.cpp:1589
#26 0x00007f66c33ad495 in mozilla::dom::WorkerGlobalScope_Binding::createImageBitmap_promiseWrapper (cx=0x61d000190080, obj=..., void_self=0x613000110780, args=...) at WorkerGlobalScopeBinding.cpp:1668
#27 0x00007f66c40cd1be in mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ConvertExceptionsToPromises> (cx=0x61d000190080, argc=1, vp=0x6210004a3a60) at /home/twsmith/code/mozilla-central/dom/bindings/BindingUtils.cpp:3229
#28 0x00007f66d23d5d1a in CallJSNative (cx=0x61d000190080, native=0x7f66c40cb690 <mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ConvertExceptionsToPromises>(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:508
#29 js::InternalCallOrConstruct (cx=0x61d000190080, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:600
#30 0x00007f66d23d72b1 in InternalCall (cx=0x61d000190080, args=..., reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:665
#31 0x00007f66d23d6f42 in js::CallFromStack (cx=0x61d000190080, args=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:669
#32 0x00007f66d23a77ec in Interpret (cx=0x61d000190080, state=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:3337
#33 0x00007f66d2363e17 in js::RunScript (cx=0x61d000190080, state=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:469
#34 0x00007f66d23d63a0 in js::InternalCallOrConstruct (cx=0x61d000190080, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:637
#35 0x00007f66d23d72b1 in InternalCall (cx=0x61d000190080, args=..., reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:665
#36 0x00007f66d23d7565 in js::Call (cx=0x61d000190080, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:682
#37 0x00007f66d2b74098 in js::PromiseObject::create (cx=0x61d000190080, executor=..., proto=..., needsWrapping=false) at /home/twsmith/code/mozilla-central/js/src/builtin/Promise.cpp:2445
#38 0x00007f66d2bddc80 in PromiseConstructor (cx=0x61d000190080, argc=1, vp=0x6210004a39a0) at /home/twsmith/code/mozilla-central/js/src/builtin/Promise.cpp:2366
#39 0x00007f66d23d871d in CallJSNative (cx=0x61d000190080, native=0x7f66d2bdc6a0 <PromiseConstructor(JSContext*, unsigned int, JS::Value*)>, reason=js::CallReason::Call, args=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:508
#40 CallJSNativeConstructor (cx=0x61d000190080, native=0x7f66d2bdc6a0 <PromiseConstructor(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:524
#41 InternalConstruct (cx=0x61d000190080, args=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:709
#42 0x00007f66d23d7898 in js::ConstructFromStack (cx=0x61d000190080, args=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:755
#43 0x00007f66d23a750c in Interpret (cx=0x61d000190080, state=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:3327
#44 0x00007f66d2363e17 in js::RunScript (cx=0x61d000190080, state=...) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:469
#45 0x00007f66d23d63a0 in js::InternalCallOrConstruct (cx=0x61d000190080, args=..., construct=js::NO_CONSTRUCT, reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:637
#46 0x00007f66d23d72b1 in InternalCall (cx=0x61d000190080, args=..., reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:665
#47 0x00007f66d23d7565 in js::Call (cx=0x61d000190080, fval=..., thisv=..., args=..., rval=..., reason=js::CallReason::Call) at /home/twsmith/code/mozilla-central/js/src/vm/Interpreter.cpp:682
#48 0x00007f66d27e8f8e in JS::Call (cx=0x61d000190080, thisv=..., fval=..., args=..., rval=...) at /home/twsmith/code/mozilla-central/js/src/jsapi.cpp:2821
#49 0x00007f66c3ab011f in mozilla::dom::Function::Call (this=0x6060002e3520, cx=..., aThisVal=..., arguments=..., aRetVal=..., aRv=...) at FunctionBinding.cpp:45
#50 0x00007f66c0f2507a in mozilla::dom::Function::Call<nsCOMPtr<nsIGlobalObject> > (this=0x6060002e3520, thisVal=..., arguments=..., aRetVal=..., aRv=..., aExecutionReason=0x7f66b4f26e40 <str> "setInterval handler", aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions, aRealm=0x0) at /home/twsmith/code/mozilla-central/objdir-ff-ubsan/dist/include/mozilla/dom/FunctionBinding.h:73
#51 0x00007f66c0f08794 in mozilla::dom::CallbackTimeoutHandler::Call (this=0x6060002e3580, aExecutionReason=0x7f66b4f26e40 <str> "setInterval handler") at /home/twsmith/code/mozilla-central/dom/base/TimeoutHandler.cpp:167
#52 0x00007f66c8229bee in mozilla::dom::WorkerPrivate::RunExpiredTimeouts (this=0x61b000074d80, aCx=0x61d000190080) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:4664
#53 0x00007f66c824ded1 in mozilla::dom::(anonymous namespace)::TimerRunnable::WorkerRun (this=0x6060002b6280, aCx=0x61d000190080, aWorkerPrivate=0x61b000074d80) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:567
#54 0x00007f66c8237cda in mozilla::dom::WorkerRunnable::Run (this=0x6060002b6280) at /home/twsmith/code/mozilla-central/dom/workers/WorkerRunnable.cpp:370
#55 0x00007f66c824defc in mozilla::dom::(anonymous namespace)::TimerRunnable::Notify (this=0x6060002b6280, aTimer=0x6030004f6360) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:571
#56 0x00007f66ba2b7e01 in nsTimerImpl::Fire (this=0x60d0000b0470, aGeneration=98) at /home/twsmith/code/mozilla-central/xpcom/threads/nsTimerImpl.cpp:565
#57 0x00007f66ba2b734a in nsTimerEvent::Run (this=0x6210000c09a0) at /home/twsmith/code/mozilla-central/xpcom/threads/TimerThread.cpp:251
#58 0x00007f66ba2ce35c in nsThread::ProcessNextEvent (this=0x615000225e00, aMayWait=false, aResult=0x7f669f2965e0) at /home/twsmith/code/mozilla-central/xpcom/threads/nsThread.cpp:1234
#59 0x00007f66ba2d8696 in NS_ProcessNextEvent (aThread=0x615000225e00, aMayWait=false) at /home/twsmith/code/mozilla-central/xpcom/threads/nsThreadUtils.cpp:513
#60 0x00007f66c8218da0 in mozilla::dom::WorkerPrivate::DoRunLoop (this=0x61b000074d80, aCx=0x61d000190080) at /home/twsmith/code/mozilla-central/dom/workers/WorkerPrivate.cpp:2982
#61 0x00007f66c81a34bb in mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run (this=0x606000297c80) at /home/twsmith/code/mozilla-central/dom/workers/RuntimeService.cpp:2218
#62 0x00007f66ba2ce35c in nsThread::ProcessNextEvent (this=0x615000225e00, aMayWait=true, aResult=0x7f669f298200) at /home/twsmith/code/mozilla-central/xpcom/threads/nsThread.cpp:1234
#63 0x00007f66ba2d8696 in NS_ProcessNextEvent (aThread=0x615000225e00, aMayWait=true) at /home/twsmith/code/mozilla-central/xpcom/threads/nsThreadUtils.cpp:513
#64 0x00007f66bc815d8c in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0x606000299120, aDelegate=0x7f669f2987d0) at /home/twsmith/code/mozilla-central/ipc/glue/MessagePump.cpp:332
#65 0x00007f66bc51e5a0 in MessageLoop::RunInternal (this=0x7f669f2987d0) at /home/twsmith/code/mozilla-central/ipc/chromium/src/base/message_loop.cc:334
#66 0x00007f66bc51e4f5 in MessageLoop::RunHandler (this=0x7f669f2987d0) at /home/twsmith/code/mozilla-central/ipc/chromium/src/base/message_loop.cc:327
#67 0x00007f66bc51e460 in MessageLoop::Run (this=0x7f669f2987d0) at /home/twsmith/code/mozilla-central/ipc/chromium/src/base/message_loop.cc:309
#68 0x00007f66ba2c554c in nsThread::ThreadFunc (aArg=0x7ffc161aefe0) at /home/twsmith/code/mozilla-central/xpcom/threads/nsThread.cpp:442
#69 0x00007f66e6b365bd in _pt_root (arg=0x6120000bd040) at /home/twsmith/code/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:201
#70 0x00007f66eafe86db in start_thread (arg=0x7f669f299700) at pthread_create.c:463
#71 0x00007f66e9fc6a3f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

It's pretty wild so it's believable that something is going wrong here.

I found this by setting a watch point on *(char*)(((unsigned long)srcBufferPtr >> 3) + 0x07fff8000) which should be the shadow buffer for srcBufferPtr.

It looks like what's happening here is that data in the 'dom::Uint8ClampedArray array' in ImageBitmap::CreateInternal is being freed in a gc driven by a nested event loop that happens before CreateImageFromRawDataInMainThreadSyncTask can run.

I have no idea who's responsibility it is to keep the buffer alive.

Olli, you were the superreviewer for this code. Do you have any recollection of who owns what and who's supposed to keep stuff alive?

Flags: needinfo?(bugs)

Hmm, is something missing from the static analysis.
I think there should be
RootedSpiderMonkeyInterface<Uint8ClampedArray> array(RootingCx());
for https://searchfox.org/mozilla-central/rev/1d34bd022de0b55c81d9db6026f69bda1d4a86d2/dom/canvas/ImageBitmap.cpp#880
Similar to
https://searchfox.org/mozilla-central/rev/1d34bd022de0b55c81d9db6026f69bda1d4a86d2/dom/canvas/CanvasRenderingContext2D.cpp#5102-5103

Flags: needinfo?(bugs) → needinfo?(sphink)

That seems very believable

Jesse, any chance you could try the patch?

(Tried to manually trigger GC at a rather bad time, but didn't succeed to crash even without the patch)

Flags: needinfo?(jschwartzentruber)
Flags: needinfo?(amarchesini)

302 Tyson

Flags: needinfo?(jschwartzentruber) → needinfo?(twsmith)

The attached patch does not fix the issue.

Flags: needinfo?(twsmith)

Tyson, is it possible to get a pernosco trace with the patch applied?

Flags: needinfo?(twsmith)

A Pernosco session (with patch applied) is available here: https://pernos.co/debug/gZpe_NT5nOaGGFtxVCqERQ/index.html

Flags: needinfo?(twsmith)

Hmm, can shrinking GC move the data typed arrays point to?

Flags: needinfo?(jcoppeard)

I was told it is possible. So I think we need to do a memory copy or prevent somehow GC to run.

Flags: needinfo?(jcoppeard)

Jon, can you give some more information on what the rules are GC wise and a recommendation for how to prevent the array contents from moving?

Flags: needinfo?(jcoppeard)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #23)
Yes, GC can move typed array data that is stored inline. There are some more details at the bottom of this section:

https://searchfox.org/mozilla-central/source/js/src/vm/ArrayBufferObject.h#90-104

Usually this is not a problem because you would get the data pointer every time you needed to use it, rather than caching it and storing it somewhere.

I looked at the stacks and it seems the data pointer is being passed between threads (from a worker to main thread). SpiderMonkey objects are only designed to be used from the a single thread, so this is a bit questionable.

I guess you could disable GC while waiting for this task to run on the main thread, but I'm not sure what else can run on the worker in the meantime. You don't want to disable GC while running user tasks as you could easily exhaust memory that way.

If you really do need to access objects from multiple threads like this, a better solution might be to disable compacting GC while it is happening. We don't currently have a public API to do this but we could expose one.

Flags: needinfo?(jcoppeard)
Attachment #9189194 - Attachment description: Bug 1666285, wrap array in RootedSpiderMonkeyInterface → Bug 1666285, try to avoid slow shrinking GC during sync calls

Updated the patch to avoid shrinking GC at bad times

(In reply to Jon Coppeard (:jonco) from comment #24)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #23)

I looked at the stacks and it seems the data pointer is being passed between threads (from a worker to main thread). SpiderMonkey objects are only designed to be used from the a single thread, so this is a bit questionable.

Oh, that's what is going on here? I worry then that any specific solution might be papering things over, and future changes could reintroduce problems.

Where is the data coming from? Could it transfer out the data? Specifically, if you used JS::StealArrayBufferContents, then compacting GC would not be a problem. If the data is stored inline, it would be copied. Otherwise, you would get ownership of the original data pointer. In either case, the originating buffer would be detached. This would also remove any cross-thread usage of the GC heap.

But I don't know if this would work for this scenario; perhaps the originating ArrayBuffer needs to be able to access the data afterwards.

(Note that separately I'm looking into why the hazard analysis missed this case.)

ImageData object in the worker owns the data, so we can't really steal it, as far as I see.

I'm still trying to reproduce the crash without the patch so that I could test whether the patch helps with the issue.

Currently: Performing replay (48/100)...
and no luck reproducing, and it runs slowly.

The hazard analysis did not see this as a problem because the variable array that has a type that is known to hold GC pointers (dom::Uint8ClampedArray) is not live across a GC function call. Here's a simplified version of the code:

  dom::Uint8ClampedArray array;
  array.Init(aImageData.GetDataObject());
  RefPtr<layers::Image> data = CreateImageFromRawData(array.Data(), dataLength);
  RefPtr<ImageBitmap> ret = new ImageBitmap(data);
  return ret.forget();

The variable array is live from the call to Init() up until the call to array.Data() in the parameter list of CreateImageFromRawData. That function can GC, but not until after array.Data() returns, so it isn't considered to be within the live range of array. The problem here is that array.Data() returns an interior pointer to a GC thing -- namely, a pointer to its data. That type is not considered to be a GC pointer (it's just a glorified char*). Not that it would help locally if it were; the temporary containing that pointer is consumed by the function call anyway, so its live range ends at the call and there is no usage of its potentially invalid value after that call. But the problem would still be caught, presumably, because the callee would have an argument whose live range would extend across the internal GC call.

Interior pointers in general are very difficult to handle safely, and very tough for the analysis to check. Holding them in types that are known to point into the GC heap doesn't help much, because you'll end up converting them to something else whenever you actually use the data somehow.

The problem I see here is that the dom::Uint8ClampedArray type is, as I understand it, really intended to only be used in autogenerated DOM bindings code. It is a very unsafe type that pokes holes in our static checking setup. If we're going to allow its use in handwritten code, then at the very least we should add our usual safety precautions to it -- eg, add a JS::AutoRequireNoGC& parameter to its constructor, so that you can only use it in a scope that forbids GC from happening. (Or it could get extra fancy, and copy the data if it might move due to a GC. See AutoStableStringChars.) And add a JS::AutoRequireNoGC& parameter to the rooted variant's Data() accessor method.

This is currently an attractive footgun.

Assignee: nobody → bugs
Status: NEW → ASSIGNED
Attached file test_reduced.zip —

This is a slightly more reduced test case that much more reliable.

Attachment #9177909 - Attachment is obsolete: true

sfink, can some random externally scheduled GC end up being a 'last ditch' GC?

<smaug> jonco: "last ditch" GC happens only when JS is running and needs to do GC, right?
<smaug> not when some externally scheduled GC will be triggered
<jonco> smaug: right, last ditch is when we fail to allocate a chunk or we hit the heap size limit
<jonco> so when running JS
Flags: needinfo?(sphink)

I have a spot-fix that I think would work for this, but it feels a little iffy:

uint8_t* JS::GetFixedArrayBufferData(JSObject* obj, uint8_t* buffer, size_t bufSize) would return the usual data pointer for non-inline (large) ArrayBuffers. For inline data, it would copy the data into the provided buffer and return it instead. The intended use would be:

    size_t maxInline = aobj->maxInlineBytes(); // new API. Might be done differently.
    char buffer[maxInline]; // note: this is an alloca() unless maxInlineBytes() is constexpr.
    char* data = JS::GetFixedArrayBufferData(aobj, buffer, maxInline);

That doesn't actually work for a drop-in replacement here, because dom::Uint8ClampedArray grabs the data pointer in its ComputeState() and hangs onto it. But the general idea could work. At least, I think the data won't be needed outside of this scope (even though it calls down into synchronous event loop processing or whatever).

Comment on attachment 9189194 [details]
Bug 1666285, try to avoid slow shrinking GC during sync calls

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: The patch does pin point where the issue is, but web page controllable JS shouldn't be running at that time.
  • 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?: All
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • If not, how different, hard to create, and risky will they be?: The code shouldn't have been changed recently, so the patch should work everywhere.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions
Attachment #9189194 - Flags: sec-approval?
No longer blocks: gfx-triage
See Also: → 1682068

I filed bug 1682068 for the remaining problem.

Comment on attachment 9189194 [details]
Bug 1666285, try to avoid slow shrinking GC during sync calls

approved to land. Flagging because it seems like we should uplift this?

Attachment #9189194 - Flags: sec-approval?
Attachment #9189194 - Flags: sec-approval+
Attachment #9189194 - Flags: approval-mozilla-esr78?
Attachment #9189194 - Flags: approval-mozilla-beta?

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Priority: P3 → P1

https://hg.mozilla.org/mozilla-central/rev/8dc09fd5f768

Group: gfx-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox86: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch

Comment on attachment 9189194 [details]
Bug 1666285, try to avoid slow shrinking GC during sync calls

approved for 85.0b3

Attachment #9189194 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(bugs)
Whiteboard: [sec-survey]

(I don't feel super comfortable to fill information about security bug to an external form.)

Flags: needinfo?(bugs)

Comment on attachment 9189194 [details]
Bug 1666285, try to avoid slow shrinking GC during sync calls

Approved for 78.7esr.

Attachment #9189194 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
QA Whiteboard: post-critsmash-triage
Flags: qe-verify-
Whiteboard: [sec-survey] → [sec-survey][adv-main85+r]
Whiteboard: [sec-survey][adv-main85+r] → [sec-survey][adv-main85+r][adv-esr78.7+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: