1938968 - use atomic store in wasm::JumpTables::setJitEntry

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Tyson Smith [:tsmith]]

Attachment: tsan_log.txt

Found with m-c 20241222-7d5c498d3fc4 (--enable-thread-sanitizer --enable-fuzzing)

This was found by visiting a live website with a TSan build.

STR:

• Launch browser and visit: http://preview.illustrator.adobe.com/home

WARNING: ThreadSanitizer: data race (pid=80098)
  Atomic write of size 8 at 0x7ffe3706afd0 by thread T34:
    #0 setJitEntryIfNull /builds/worker/checkouts/gecko/js/src/wasm/WasmCode.h:902:11 (libxul.so+0xb089e2d) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #1 setJitEntryIfNull /builds/worker/checkouts/gecko/js/src/wasm/WasmCode.h:1088:17 (libxul.so+0xb089e2d)
    #2 js::wasm::Instance::getExportedFunction(JSContext*, unsigned int, JS::MutableHandle<JSFunction*>) /builds/worker/checkouts/gecko/js/src/wasm/WasmInstance.cpp:3694:16 (libxul.so+0xb089e2d)
    #3 js::WasmInstanceObject::getExportedFunction(JSContext*, JS::Handle<js::WasmInstanceObject*>, unsigned int, JS::MutableHandle<JSFunction*>) /builds/worker/checkouts/gecko/js/src/wasm/WasmJS.cpp:2003:19 (libxul.so+0xb0a6b20) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #4 js::wasm::Table::getFuncRef(JSContext*, unsigned int, JS::MutableHandle<JSFunction*>) const /builds/worker/checkouts/gecko/js/src/wasm/WasmTable.cpp:178:10 (libxul.so+0xb17c81b) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #5 js::wasm::Table::getValue(JSContext*, unsigned int, JS::MutableHandle<JS::Value>) const /builds/worker/checkouts/gecko/js/src/wasm/WasmTable.cpp:287:12 (libxul.so+0xb17d59d) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #6 js::WasmTableObject::getImpl(JSContext*, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/wasm/WasmJS.cpp:2891:16 (libxul.so+0xb0abe6a) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #7 CallNonGenericMethod<&IsTable, &js::WasmTableObject::getImpl> /builds/worker/workspace/obj-build/dist/include/js/CallNonGenericMethod.h:103:12 (libxul.so+0xb0ac00f) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #8 js::WasmTableObject::get(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/js/src/wasm/WasmJS.cpp:2897:10 (libxul.so+0xb0ac00f)
    #9 <null> <null> ([anon:js-executable-memory]+0x741a)
    #10 js::Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:2107:17 (libxul.so+0xa09e5f5) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    ...

  Previous write of size 8 at 0x7ffe3706afd0 by main thread (mutexes: write M0):
    #0 setJitEntry /builds/worker/checkouts/gecko/js/src/wasm/WasmCode.h:895:19 (libxul.so+0xb04884c) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #1 js::wasm::Code::createOneLazyEntryStub(js::RWExclusiveData<js::wasm::Code::ProtectedData>::WriteGuard const&, unsigned int, js::wasm::CodeBlock const&, void**) const /builds/worker/checkouts/gecko/js/src/wasm/WasmCode.cpp:720:17 (libxul.so+0xb04884c)
    #2 js::wasm::Code::getOrCreateInterpEntry(unsigned int, js::wasm::FuncExport const**, void**) const /builds/worker/checkouts/gecko/js/src/wasm/WasmCode.cpp:747:10 (libxul.so+0xb048b48) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #3 GetInterpEntryAndEnsureStubs /builds/worker/checkouts/gecko/js/src/wasm/WasmInstance.cpp:3213:24 (libxul.so+0xb09805d) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #4 js::wasm::Instance::callExport(JSContext*, unsigned int, JS::CallArgs const&, js::wasm::CoercionLevel) /builds/worker/checkouts/gecko/js/src/wasm/WasmInstance.cpp:3725:8 (libxul.so+0xb09805d)
    #5 WasmCall(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/js/src/wasm/WasmInstance.cpp:3602:19 (libxul.so+0xb097e9f) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #6 CallJSNative /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:532:13 (libxul.so+0xa098f3b) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #7 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:628:12 (libxul.so+0xa098f3b)
    #8 InternalCall /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:695:10 (libxul.so+0xa0a8559) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #9 CallFromStack /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:700:10 (libxul.so+0xa0a8559)
    #10 js::Interpret(JSContext*, js::RunState&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:3338:16 (libxul.so+0xa0a8559)
    ...

  Location is heap block of size 1918688 at 0x7ffe3702b000 allocated by thread T21:
    #0 calloc /builds/worker/fetches/llvm-project/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:686:5 (firefox-bin+0xd476b) (BuildId: 8a27122eb4cda3296791807759aec668e9089d96)
    #1 calloc /builds/worker/checkouts/gecko/memory/build/malloc_decls.h:52:1 (firefox-bin+0x15b88e) (BuildId: 8a27122eb4cda3296791807759aec668e9089d96)
    #2 moz_arena_calloc /builds/worker/checkouts/gecko/memory/build/malloc_decls.h:52:1 (firefox-bin+0x15b88e)
    #3 moz_arena_calloc /builds/worker/checkouts/gecko/memory/build/malloc_decls.h:159:1 (firefox-bin+0x15b88e)
    #4 js_arena_calloc /builds/worker/workspace/obj-build/dist/include/js/Utility.h:402:10 (libxul.so+0xb04cf25) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #5 js_pod_arena_calloc<void *> /builds/worker/workspace/obj-build/dist/include/js/Utility.h:616:26 (libxul.so+0xb04cf25)
    #6 js_pod_calloc<void *> /builds/worker/workspace/obj-build/dist/include/js/Utility.h:621:10 (libxul.so+0xb04cf25)
    #7 js::wasm::JumpTables::initialize(js::wasm::CompileMode, js::wasm::CodeMetadata const&, js::wasm::CodeBlock const&, js::wasm::CodeBlock const&) /builds/worker/checkouts/gecko/js/src/wasm/WasmCode.cpp:1197:23 (libxul.so+0xb04cf25)
    #8 js::wasm::Code::initialize(mozilla::Vector<js::wasm::FuncImport, 0ul, js::SystemAllocPolicy>&&, mozilla::UniquePtr<js::wasm::CodeBlock, JS::DeletePolicy<js::wasm::CodeBlock>>, mozilla::UniquePtr<js::wasm::LinkData, JS::DeletePolicy<js::wasm::LinkData>>, mozilla::UniquePtr<js::wasm::CodeBlock, JS::DeletePolicy<js::wasm::CodeBlock>>, mozilla::UniquePtr<js::wasm::LinkData, JS::DeletePolicy<js::wasm::LinkData>>) /builds/worker/checkouts/gecko/js/src/wasm/WasmCode.cpp:1249:20 (libxul.so+0xb04d8c2) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #9 js::wasm::ModuleGenerator::finishModule(js::wasm::ShareableVector<unsigned char, 0ul, js::SystemAllocPolicy> const&, RefPtr<js::wasm::ModuleMetadata>, JS::OptimizedEncodingListener*) /builds/worker/checkouts/gecko/js/src/wasm/WasmGenerator.cpp:1325:23 (libxul.so+0xb077b0e) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    #10 js::wasm::CompileStreaming(js::wasm::CompileArgs const&, mozilla::Vector<unsigned char, 0ul, js::SystemAllocPolicy> const&, mozilla::Vector<unsigned char, 0ul, js::SystemAllocPolicy> const&, js::ExclusiveWaitableData<unsigned char const*> const&, js::ExclusiveWaitableData<js::wasm::StreamEndData> const&, mozilla::Atomic<bool, (mozilla::MemoryOrdering)2, void> const&, mozilla::UniquePtr<char [], JS::FreePolicy>*, mozilla::Vector<mozilla::UniquePtr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>*) /builds/worker/checkouts/gecko/js/src/wasm/WasmCompile.cpp:1147:13 (libxul.so+0xb05669a) (BuildId: dbda6e8668f9693243cbfbc8006509b505f8cad0)
    ...

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:rhunt, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[Julian Seward [:jseward]]

This is a race on JumpTables::jit_ entries. which are not marked as
atomic<..>. We have JumpTables::setJitEntryIfNull racing against
JumpTables::setJitEntry.

The former uses __atomic_compare_exchange_n, which seems to imply at least
some kind of memory fencing going on under the hood. But ::setJitEntry does a
plain store with no fencing. That might be what tsan is complaining about.

I wonder if ::setJitEntry should instead do
__atomic_store_n(&jit_.get()[i], target, __ATOMIC_RELAXED)
so as to put a store-fence after the store.

I guess the readers of these values -- in generated code -- read these entries
without a read fence. We don't care about that, and tsan can't monitor those
loads, so it doesn't complain about them. Indeed, we don't really care even
about the ::setJitEntryIfNull vs ::setJitEntry race except for the fact that it
causes tsan to complain.

Unfortunately I don't have a machine with enough memory currently available to
run the test case. So I can't reproduce/fix. I will try to set one up.

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1938968-minimal.diff-1

This is a minimal fix, which, as tested by Tyson, stops TSan complaining. It
merely uses __atomic_store_n(.., __ATOMIC_RELAXED) as described in comment 2.

IMO this is still pretty bogus in that, according to the GCC docs,
https://gcc.gnu.org/onlinedocs/gcc-14.2.0/gcc/_005f_005fatomic-Builtins.html,
"__ATOMIC_RELAXED .. Implies no inter-thread ordering constraints." -- so it
shuts TSan up but has no effect on the final generated code?

Comment 4

[Julian Seward [:jseward]]

Attachment: bug1938968-full.diff-1

Here is perhaps a more convincing fix. It tries to create a release-acquire
happens-before edge from the store in setJitEntry to the (implied) load in
setJitEntryIfNull.

Unrelatedly .. I can't see how this bug has security implications; we should
open it up. At worst it could cause a (presumably redundant) update to a
JumpTables::jit_ entry to be performed when in fact it isn't necessary. This
would be in the case where setJitEntry happens first, but the write isn't
yet visible to a later setJitEntryIfNull to the same entry, so the entry is
updated needlessly.

Comment 5

[Tyson Smith [:tsmith]]

I can no longer reproduce the issue with bug1938968-full.diff-1 applied.

Comment 6

[Julian Seward [:jseward]]

(In reply to Tyson Smith [:tsmith] from comment #5)

I can no longer reproduce the issue with bug1938968-full.diff-1 applied.

Tyson, thanks for testing both patches. In the end we decided to go with
bug1938968-minimal.diff-1.

Comment 7

[Ryan Hunt [:rhunt]]

Julian and I talked about this and we don't believe there is a real security issue here. The issue is that we're using a bare store instruction instead of a relaxed atomic store, which gives us the t-san violation. But the actual codegen should be the same. So I'm going to open this one up.

Comment 8

[Julian Seward [:jseward]]

Attachment: Bug 1938968 - use atomic store in wasm::JumpTables::setJitEntry. r=rhunt.

JumpTables::setJitEntry doesn't use an atomic store to write the jit_ array,
and so TSan reports a race against ::setJitEntryIfNull, which does use suitable
atomic primitives. The resulting race (if any) is harmless since we are not
relying on the synchronisation here anyways, but the TSan complaint blocks
fuzzing. This patch fixes ::setJitEntry.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57fb9311c3c9
use atomic store in wasm::JumpTables::setJitEntry.  r=rhunt.

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/57fb9311c3c9

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jseward, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox135 to wontfix.

For more information, please visit BugBot documentation.

Comment 12

[Julian Seward [:jseward]]

I set status-firefox135 to wontfix. This bug doesn't have any effect
in practice, per comment 8; so it's just to keep TSan happy.