1943912 - Use after free at process shutdown

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Julien Wajsberg [:julienw]]

I've spotted this crash in a test:

https://treeherder.mozilla.org/logviewer?job_id=491704916&repo=try&lineNumber=2282

It looks like it's an use after free at https://searchfox.org/mozilla-central/rev/a965e3c683ecc035dee1de72bd33a8d91b1203ed/tools/profiler/gecko/ProfilerChild.cpp#149

Nazım, I think you touched the code to grab profiles at shutdown recently, might it be related?

Comment 1

[Nazım Can Altınova [:canova]]

It's not related to the code that I was touching but it's close enough as they are both shutdown issues.

Looking at the lifetime and ownership of the mChunkManager, it's a bit tricky, we have a single place that owns it, but we also keep pointers to it in 2 places, which is not great:

mChunkManager normally is stored inside the static core buffer as owned or as a pointer. But owned version is always used for backtrace buffer, never the core profiler buffer.

For the pointer, it's ownership is always in ActivePS, so we store this as a UniquePtr here. so we construct it when it's constructed, and we destroy it when it's destroyed.

Probably we are arriving to ProfilerChild::ResetChunkManager after we are done destructing ActivePS. So by the time we are at ProfilerChild::ResetChunkManager, the chunk manager will be already destroyed.

ProfilerChild::ResetChunkManager is used in 2 places:

• ProfilerChild::RecvStop
• ProfilerChild::Destroy

ProfilerChild::RecvStop totally makes sense as we are getting this stop signal from the ProfilerParent and we are SURE that we have ActivePS. But we can't say the same for ProfilerChild::Destroy. We will most likely arrive here after the profiler stopped and we are done profiling. So I think we can safely remove this call from ProfilerChild::Destroy because it doesn't matter if we clear the ChunkManager that will be destroyed (or already has been destroyed).

So I think removing this from here would solve this issue. On the other hand, we should also think about how not to use a raw pointer here. But this might be a bit more tricky to change.

Comment 2

[Nazım Can Altınova [:canova]]

Attachment: Bug 1943912 - Do not reset the chunk manager while shutdown r?#profiler

We don't need to reset it as it's already during process shutdown and
ChunkManager will be removed anyways.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bf0fade667c8
Do not reset the chunk manager while shutdown r=profiler-reviewers,aabh

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/bf0fade667c8

Comment 5

[Donal Meehan [:dmeehan]]

Sorry NI added in error

Comment 6

[Donal Meehan [:dmeehan]]

:canova could you add an esr128 uplift request on this?

Comment 7

[Nazım Can Altınova [:canova]]

Attachment: Bug 1943912 - Do not reset the chunk manager while shutdown

Original Revision: https://phabricator.services.mozilla.com/D235642

Comment 8

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: potential use after free during shutdown
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: little risk
• Explanation of risk level: it's fixing a uaf during shutdown
• String changes made/needed: none
• Is Android affected?: yes

Comment 9

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: potential use after free during shutdown
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: none
• Risk associated with taking this patch: little risk
• Explanation of risk level: it's fixing a uaf during shutdown, and only when the profiler is active
• String changes made/needed: none
• Is Android affected?: yes

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/ecc81ec1d4af