1702086 - Crash in [@ SharedLibraryInfo::GetInfoForSelf]

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P1)

Description

[Toshihito Kikuchi [:toshi]]

Crash report: https://crash-stats.mozilla.org/report/index/87dd4fbb-2c1c-4687-836a-309020210330

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top frames of crashing thread:

0 xul.dll static SharedLibraryInfo::GetInfoForSelf tools/profiler/core/shared-libraries-win32.cc:209
1 xul.dll mozilla::Telemetry::BatchProcessedStackGenerator::BatchProcessedStackGenerator toolkit/components/telemetry/other/ProcessedStack.cpp:72
2 xul.dll mozilla::UntrustedModulesProcessor::CompleteProcessing toolkit/xre/UntrustedModulesProcessor.cpp:862
3 xul.dll mozilla::MozPromise<mozilla::Maybe<mozilla::UntrustedModulesProcessor::ModulesMapResultWithLoads>, nsresult, 1>::ThenValue<`lambda at /builds/worker/checkouts/gecko/toolkit/xre/UntrustedModulesProcessor.cpp:548:9', `lambda at /builds/worker/checkouts/gecko/toolkit/xre/UntrustedModulesProcessor.cpp:557:9'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:846
4 xul.dll mozilla::MozPromise<RefPtr<mozilla::AllocPolicy::Token>, bool, 1>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:487

The crash happened because the module was unloaded while running GetPdbInfo. It seems that calling LoadLibraryEx does not contribute to locking the module loaded.

0:010> .excr
rax=00007ffc35faa560 rbx=0000000000000000 rcx=00007ffc50e8d074
rdx=0000000000000000 rsi=00007ffc35e80000 rdi=000000f9a35be960
rip=00007ffc18f0fc68 rsp=000000f9a35be790 rbp=00007ffc35e80000
 r8=000000f9a35be748  r9=000000000000003f r10=0000000000000000
r11=0000000000000246 r12=ffffffffffffffff r13=0003001100000000
r14=000000f9a35bec40 r15=000000f9a35be870
iopl=0         nv up ei pl nz na po nc
cs=0033  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00010206
xul!GetPdbInfo+0x43 [inlined in xul!SharedLibraryInfo::GetInfoForSelf+0x398]:
00007ffc`18f0fc68 83780c02        cmp     dword ptr [rax+0Ch],2 ds:00007ffc`35faa56c=????????

Unloaded modules that overlapped the address in the past:
         Base Address       End Address              Size
        7ffc`35e80000     7ffc`35fcd000        0`0014d000 msvp9dec_store.dll

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Marking as high severity because this is a topcrasher in RDD.

Comment 2

[Toshihito Kikuchi [:toshi]]

The crashing stack includes UntrustedModulesProcessor, but the code we need to fix is under the profiler.

Comment 3

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1702086 - Make sure LoadLibraryEx really locks the mapped address. r=mstange

Before SharedLibraryInfo::GetInfoForSelf calls GetPdbInfo to parse a module's
PE header, it calls LoadLibraryEx to prevent the module from being unloaded
during GetPdbInfo. If the module was already unloaded before LoadLibraryEx,
however, LoadLibraryEx maps the module onto an address different from the original
mapped address, so that module.lpBaseOfDll becomes invalid.

This patch is to call LoadLibraryEx before GetModuleInformation to make sure
LoadLibraryEx increments the module's refcount and does not map the module onto
a new address. With this, module.lpBaseOfDll is always valid, thus we don't have
to call VirtualQuery.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bd7baadb78c
Make sure LoadLibraryEx really locks the mapped address. r=mstange

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6bd7baadb78c

Comment 6

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

The patch landed in nightly and beta is affected.
:toshi, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 7

[Toshihito Kikuchi [:toshi]]

Comment on attachment 9212831 [details]
Bug 1702086 - Make sure LoadLibraryEx really locks the mapped address. r=mstange

Beta/Release Uplift Approval Request

• User impact if declined: This was a top crasher of the rdd process, and theoretically the same crash can happen in the main or tab process depending on the timing of a module's unload. Without the fix, a user hits a crash when an injected module was unloaded while parsing its PE header to collect a telemetry ping. If it is a graphics driver, a video will be stopped.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The root cause was we assumed LoadLibraryEx would load a module as data onto the same address as before, but it was not correct. The fix makes sure we increment the module's refcount before parsing, and if unexpected unload happens, we skip parsing. The risk is low as we know the root cause and we have a targeted fix.
• String changes made/needed: None

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9212831 [details]
Bug 1702086 - Make sure LoadLibraryEx really locks the mapped address. r=mstange

Fix is looking good on Nightly so far, so let's see how it goes on Beta. Thanks for tracking this down! Approved for 88.0b8.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d8e52116ffb

Comment 10

[Toshihito Kikuchi [:toshi]]

The same crash is still happening with the fix, though the volume is small. Let's wait until this lands on release and see the impact of the patch.

Comment 11

[Toshihito Kikuchi [:toshi]]

I think my patch mistakenly reopened bug 1607574. This area is sensitive and I need to rethink a fix including bug 1704684.

Comment 12

[Alexandru Michis [:malexandru]]

Backed out changeset 6bd7baadb78c (Bug 1702086) for causing Bug 1607574.
Backout link: https://hg.mozilla.org/integration/autoland/rev/aae116d4fca8cbf096289bc89cc91d5eed4a06e5

Comment 13

[Alexandru Michis [:malexandru]]

Backout uplifted to beta: https://hg.mozilla.org/releases/mozilla-beta/rev/d3ad305b4fc7a741e8147c8205d8ad8e945c6b51

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9212831 [details]
Bug 1702086 - Make sure LoadLibraryEx really locks the mapped address. r=mstange

Clearing the Beta approval to get it off the needs-uplift radar.

Comment 15

[Alexandru Michis [:malexandru]]

Backout uplifted to release: https://hg.mozilla.org/releases/mozilla-release/rev/0bc827d318596182d607d7efa438332a7210cb80

Comment 16

[Ryan VanderMeulen [:RyanVM]]

The backout will ship in 88.0.1 going out next week.

Comment 17

[Iulian Moraru]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/aae116d4fca8

Comment 19

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1702086 - Part1: Introduce EnumerateProcessModules. r=gerald

This patch introduces EnumerateProcessModules to enumerate all loaded modules
in the local process so that Gecko profiler and baseprofiler can use it.

Comment 20

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1702086 - Part2: Get PDB information from the new handle. r=gerald

This patch is the actual fix for Bug 1702086. The problem of Bug 1702086 is that
LoadLibraryExW loaded the module onto an address different from the original
mapped addresss because it was unloaded after we started enumeration. Calling
GetPdbInfo with the original address module.lpBaseOfDll caused a crash.

The proposed fix consists of three parts.

The first part is to get PDB information from handleLock, which is always valid
even if the original address was unloaded. With this, we don't need a check
of VirtualQuery.

The second part is to add LOAD_LIBRARY_AS_IMAGE_RESOURCE along with
LOAD_LIBRARY_AS_DATAFILE to the call to LoadLibraryEx. This is needed
to read information from the sections outside the PE headers because
RVA (= relative virtual address) is an address after relocation.
Without LOAD_LIBRARY_AS_IMAGE_RESOURCE, a module is mapped without relocation,
so GetPdbInfo() accesses wrong memory resulting in a crash.

The third part is to introduce PEHeaders::GetPdbInfo, replacing two versions
of GetPdbInfo in Gecko profiler and baseprofiler.

Depends on D115252

Comment 21

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1702086 - Part3: Use PEHeaders::GetVersionInfo. r=gerald

This patch replaces two versions of GetVersion in Gecko profiler and
baseprofiler with PEHeaders::GetVersionInfo.

Depends on D115253

Comment 22

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1702086 - Part4: Use PEHeaders::GetBounds instead of GetModuleInformation. r=gerald

SharedLibraryInfo::GetInfoForSelf() can use PEHeaders::GetBounds instead of
GetModuleInformation to get the start/end address of a module's mapped region
in the local process. It's roughly 100x faster because GetModuleInformation
invokes two system calls NtQueryInformationProcess and NtReadVirtualMemory
while nt::PEHeaders does not.

Depends on D115254

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/33f3fa78a2ea
https://hg.mozilla.org/mozilla-central/rev/03e807219ea5
https://hg.mozilla.org/mozilla-central/rev/010efc776c85
https://hg.mozilla.org/mozilla-central/rev/8f9a61f6367c