Closed Bug 1508416 Opened 3 years ago Closed 3 years ago

sAlreadyHandlingTrap is not safe to access during thread init


(Core :: Javascript: WebAssembly, defect)

Not set



Tracking Status
firefox65 --- fixed


(Reporter: away, Assigned: away)



(1 file)

If you use the gflags tool that comes with WinDbg to set "Show loader snaps" on js.exe, the NT thread init code will attempt to do some OutputDebugString logging (which internally works by raising an exception) before thread_local storage space is initialized. This will lead to endless exception-recursion when WasmTrapHandler tries to read sAlreadyHandlingTrap.
Since the TEB's ThreadLocalStoragePointer is not officially documented, this is as close to proper code as I could come up with -- at least I managed to avoid asm and intrinsics. Also this works on arm64 where I'm not even sure what the equivalent of __readgsqword would be.

I'm not super proud about the magic number, but we ought to have a reasonable guarantee of its stability because the TEB offset is baked into binaries that use thread_local.
Assignee: nobody → dmajor
Attachment #9026211 - Flags: review?(luke)
Comment on attachment 9026211 [details] [diff] [review]
Bug 1508416: Check TLS initialization before reading sAlreadyHandlingTrap.

Review of attachment 9026211 [details] [diff] [review]:

Beautiful, thanks!

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +494,5 @@
>  static LONG WINAPI
>  WasmTrapHandler(LPEXCEPTION_POINTERS exception)
>  {
> +    // Check for TLS initialization before reading sAlreadyHandlingThread.

nit: even though it's only used once, could you hoist the magic constant into:

  // Derived from: [whatever URL you think looks right]
  // Compiled in all user binaries, so should be stable over time.
  static const unsigned sThreadLocalArrayPointerIndex.
Attachment #9026211 - Flags: review?(luke) → review+
Pushed by
Check TLS initialization before reading sAlreadyHandlingTrap. r=luke
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.