1722566 - Crash in [@ mozilla::FunctionRef<T>::FunctionRef<T>::<T>::__invoke] (Crash Address == 0xe)

Status: RESOLVED FIXED

(Core :: mozglue, defect)

Description

[Toshihito Kikuchi [:toshi]]

Crash report: https://crash-stats.mozilla.org/report/index/2f280fe3-3eff-4197-97b1-af5c60210726

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll static mozilla::FunctionRef<void  mfbt/FunctionRef.h:180
1 xul.dll static SharedLibraryInfo::GetInfoForSelf tools/profiler/core/shared-libraries-win32.cc:126
2 xul.dll mozilla::Telemetry::BatchProcessedStackGenerator::BatchProcessedStackGenerator toolkit/components/telemetry/other/ProcessedStack.cpp:72
3 xul.dll mozilla::UntrustedModulesProcessor::ProcessModuleLoadQueue toolkit/xre/UntrustedModulesProcessor.cpp:569
4 xul.dll mozilla::UntrustedModulesProcessor::BackgroundProcessModuleLoadQueue toolkit/xre/UntrustedModulesProcessor.cpp:451
5 xul.dll mozilla::detail::RunnableMethodImpl<D3DVsyncSource::D3DVsyncDisplay*, void  xpcom/threads/nsThreadUtils.h:1203
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1149
7 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:330
8 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:328
9 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:310

Seems like we're missing a null check.

Comment 1

[Toshihito Kikuchi [:toshi]]

According to https://www.mathies.com/mozilla/crashes/rddrelease.html, this is the top crasher (32%) in RDD.

Comment 2

[Toshihito Kikuchi [:toshi]]

Attachment: Bug 1722566 - Add null checks in PEHeaders::FindResourceLeaf. r=mhowell

We had crashes in PEHeaders::FindResourceLeaf where idDir was nullptr. This can
happen when the resource table is modified by a third-party application and an entry
in the table points to somewhere outside the executable.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e3f7369ddc2
Add null checks in PEHeaders::FindResourceLeaf. r=mhowell

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9e3f7369ddc2

Comment 5

[Toshihito Kikuchi [:toshi]]

I found crashes with the same signature involving SharedLibraryInfo::GetInfoForSelf, but the crashing address is not near-null:

• https://crash-stats.mozilla.org/report/index/d7cdd423-cc1c-4dd5-929b-d13920210728
• https://crash-stats.mozilla.org/report/index/9d3a3a81-b3f0-41cc-bbed-a0b210210729

Those need to be fixed, too.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

(In reply to Toshihito Kikuchi [:toshi] from comment #5)

Those need to be fixed, too.

Was the plan to reopen this bug or handle them in a follow-up?

Comment 7

[Toshihito Kikuchi [:toshi]]

I'll open a new bug. Let's keep this bug resolved.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR91 approval when you get a chance.

Comment 9

[Toshihito Kikuchi [:toshi]]

Comment on attachment 9233517 [details]
Bug 1722566 - Add null checks in PEHeaders::FindResourceLeaf. r=mhowell

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This signature was a top crasher of RDD process.
• User impact if declined: If a third-party application modified the resource table of a loaded module, the process may suddenly crash when we failed to parse the module.
• Fix Landed on Version: 92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The fix adds just a couple of null checks and we skip parsing a module if we detect a loaded image was modified.
• String or UUID changes made by this patch: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9233517 [details]
Bug 1722566 - Add null checks in PEHeaders::FindResourceLeaf. r=mhowell

Approved for 91.1esr.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/272617a59467