Closed Bug 1508416 Opened 11 months ago Closed 11 months ago

sAlreadyHandlingTrap is not safe to access during thread init

Categories

(Core :: Javascript: WebAssembly, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: dmajor, Assigned: dmajor)

Details

Attachments

(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 dmajor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a55e6ff3190
Check TLS initialization before reading sAlreadyHandlingTrap. r=luke
https://hg.mozilla.org/mozilla-central/rev/3a55e6ff3190
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.