1564899 - Crash in [@ logging::LogMessage::~LogMessage] in base::win::internal::CloseHandleWrapper

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

[[:philipp]]

This bug is for crash report bp-0721c735-81ee-42b4-b6a8-f4c0c0190710.

Top 10 frames of crashing thread:

0 firefox.exe logging::LogMessage::~LogMessage security/sandbox/chromium-shim/base/logging.cpp:111
1 firefox.exe base::win::internal::CloseHandleWrapper security/sandbox/chromium/base/win/scoped_handle_verifier.cc:73
2 firefox.exe base::win::internal::ScopedHandleVerifier::CloseHandle security/sandbox/chromium/base/win/scoped_handle_verifier.cc:136
3 firefox.exe base::win::GenericScopedHandle<base::win::HandleTraits, base::win::VerifierTraits>::Close security/sandbox/chromium/base/win/scoped_handle.h:104
4 firefox.exe base::win::ScopedProcessInformation::~ScopedProcessInformation security/sandbox/chromium/base/win/scoped_process_information.cc:46
5 firefox.exe sandbox::PolicyBase::OnJobEmpty security/sandbox/chromium/sandbox/win/src/sandbox_policy_base.cc:555
6 firefox.exe static unsigned long sandbox::BrokerServicesBase::TargetEventsThread security/sandbox/chromium/sandbox/win/src/broker_services.cc:224
7 kernel32.dll BaseThreadInitThunk 
8 mozglue.dll static void patched_BaseThreadInitThunk mozglue/build/WindowsDllBlocklist.cpp:613
9 ntdll.dll __RtlUserThreadStart 

browser crash reports with this stack are starting to show up in firefox 69 - they're all coming from 32bit versions on windows 7/8 and may be related to the sandbox update from bug 1552160.

Comment 1

[Bob Owen (:bobowen) (PTO until 17th Apr)]

This appears to be a re-occurrence of bug 1449480.
In that bug I changed the CHECK to DCHECK and that's been undone by the update.

I remember from bug 1449480 that I was hoping that the handle verifier checks would help us identify where these handle closure issues were coming from. I'd watched for crashes from that for a long time but seen nothing, although it appears we may have some to go on now.

I'm not sure whether putting this back to DCHECK again is the right approach, it would be nice to get these reports in early beta, because otherwise we're missing information that could help us fix other underlying issues.

Comment 2

[Bob Owen (:bobowen) (PTO until 17th Apr)]

We're not seeing crashes when something else tries to close the handle and I can't see how the Job tracking code could be closing it twice.
There is possibly a race issue because the Job handle is used after it is closed as a lookup ID, so it could in theory have been re-used at that point, but as we currently have a policy per process (which holds the container for the lookup), I don't think that can cause any issues.

Anyway, I think this crash is too painful to have even on early Beta, so I'm going to change it back to DCHECK there but keep it as CHECK on Nightly.

Comment 3

[Bob Owen (:bobowen) (PTO until 17th Apr)]

Attachment: Bug 1564899: Make CloseHandleWrapper CHECK a DCHECK on non-Nightly builds. r=handyman!

This is because we are hitting it frequently during PolicyBase::OnJobEmpty and
currently we can't work out how this can happen.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80fff55bee77
Make CloseHandleWrapper CHECK a DCHECK on non-Nightly builds. r=handyman

Comment 5

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/80fff55bee77

Comment 6

[[:philipp]]

could you request an uplift to 69 beta, if you deem fit to do so?
thank you

Comment 7

[Bob Owen (:bobowen) (PTO until 17th Apr)]

Comment on attachment 9078186 [details]
Bug 1564899: Make CloseHandleWrapper CHECK a DCHECK on non-Nightly builds. r=handyman!

Beta/Release Uplift Approval Request

• User impact if declined: Large number of crashes will continue.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): A very similar change was made to stop this crash previously in bug 1449480.
  The only difference here is that we are leaving the crash on Nightly in the hope we can track down the issue.
• String changes made/needed: None

Comment 8

[Ryan VanderMeulen (PTO, back 6-April)]

Comment on attachment 9078186 [details]
Bug 1564899: Make CloseHandleWrapper CHECK a DCHECK on non-Nightly builds. r=handyman!

Fixes a topcrash on Beta. Approved for 69.0b6.

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a0bbef741c71