1811304 - processtools::ProcessToolsService::crash relies on Rust undefined behavior and can fail to produce crashes if CreateRemoteThread fails, e.g. if the process is exiting

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Yannis Juglaret [:yannis]]

processtools::ProcessToolsService::crash is a Rust function that we rely on to crash utility processes. The current implementation for Windows relies on a Rust undefined behavior. We are hoping that Rust generates an invalid instruction for processtools::crash_illegal_instruction, but there is no actual guarantee of that.

The implementation also relies on the unrelated fact that the function we use as a thread start address, processtools::crash_illegal_instruction, will be at the same address in the two processes. But as far as I know, this property is only guaranteed by Windows for a few DLLs like ntdll and kernel32.

We should at least not rely on Rust undefined behavior, and ideally also not rely on our DLL being loaded at the same address across processes.

Comment 1

[Yannis Juglaret [:yannis]]

I believe this could share code with bug 1793525.

Comment 2

[Yannis Juglaret [:yannis]]

As a first step, we could remove processtools::crash_illegal_instruction and make processtools::ProcessToolsService::crash create a remote thread that calls into ntdll!DbgBreakpoint instead of processtools::crash_illegal_instruction , just like the current implementation used for bug 1793525.

CreateRemoteThread is not the best way to do what is required here though and bug 1793525 has had problems with it, in particular bug 1806050 which highlights that it's forbidden to create a remote (or local) thread in an exiting process -- and exiting processes can hang in a DLL_PROCESS_DETACH. Hence the next goal in bug 1793525 is to replace the call to CreateRemoteThread by a dump-then-terminate approach, and we could probably do the same here.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

FWIW this component (processtools) appears to be implementing a fairly simple XPCOM component in Rust which is doing nothing other than calling back into C++ to invoke Windows API or libc API methods - this is one of the uncommon situations where it would probably be simpler & safer to actually implement this component in C++, as that's the language which XPCOM components and the methods being called are native to.

I mostly note this because doing the dump-then-terminate approach may be easier to do from C++ code.

Comment 4

[Yannis Juglaret [:yannis]]

Attachment: Bug 1811304 - Stop relying on undefined behavior in processtools. r=gerard-majax,gsvelto

Dereferencing a null pointer is undefined behavior in Rust, so
crash_illegal_instruction is not guaranteed to crash with an illegal
instruction. We can force a crash by calling into DbgBreakPoint from
ntdll instead, like in bug 1793525.

Comment 5

[Yannis Juglaret [:yannis]]

Attachment: Bug 1811304 - Manage CloseHandle through Drop in processtools. r=gerard-majax,gsvelto

This patch proposes to use a RAII (Drop) wrapper for handles, so that
CloseHandle is automatically called where needed.

Depends on D197998

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41a0c093dc26
Stop relying on undefined behavior in processtools. r=gerard-majax,gsvelto
https://hg.mozilla.org/integration/autoland/rev/ac7e0f183f8f
Manage CloseHandle through Drop in processtools. r=gerard-majax,gsvelto

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/41a0c093dc26
https://hg.mozilla.org/mozilla-central/rev/ac7e0f183f8f

Comment 8

[Yannis Juglaret [:yannis]]

These patches only remove the undefined behavior. We are still using CreateRemoteThread. We can probably refer to bug 1806050 for future work on the dump-then-terminate approach.