1352875 - Crash in std::map<T>::_Try_emplace<T> called from TextureClientRecycleAllocator::CreateOrRecycle()

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

+++ This bug was initially created as a clone of Bug #1351993 +++

This is for crashes where Try_emplace is called from TextureClientRecycleAllocator::CreateOrRecycle()

These crashes started in 50.1.0 it appears, and there have been very few (~14 in the last 3 months).  There are no reports after 52 (though there are reports in 52 and ESR52).  More concerning is that this appears to be a clear UAF, and this might mean that bug 1351993 might also possibly be a UAF, though I've seen no evidence that it is.

See https://crash-stats.mozilla.com/report/index/c9fb0916-7d1b-49e7-9c96-2f3e72170327

Hopefully this is something which is fixed in 53, but it might just be a signature change (or dumb luck considering the frequency).

Comment 1

[Frederik Braun [:freddy]]

Please triage/assign.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

No value in working on all std::map bugs in parallel; the root cause is likely to be the same (or the result of the same pattern.)  Looking at bug 1352877 first.

Comment 3

[Wennie]

Hi Milan:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Current status: bug 1352877 reviewed... looks like we should have a fix landed soon. Assigning this bug to Nical for bookkeeping.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Nical, what about this bug?

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Crash called from CreateOrRecycle() in 63: https://crash-stats.mozilla.com/report/index/d0c87203-f735-4103-96eb-afbc10181118

Comment 7

[Maire Reavy [:mreavy]]

Adding Jessie (new graphics engineering manager) to all sec-crit and sec-high graphics bugs

Comment 8

[Nicolas Silva [:nical]]

I'm not going to look into this one until January at least.

Comment 9

[Jessie [:jbonisteel] pls NI]

Nical - anything we can do here?

Comment 10

[Nicolas Silva [:nical]]

Not in the short term. These map::try_emplace bugs are very low volume and quite hard to investigate. I'll keep the needinfo for a bit longer to remind myself to have a look whenever I'm waiting for something.

Comment 11

[Andrew McCreight [:mccr8]]

Around a dozen of crashes like this on the 5-29 Nightly for whatever reason. one example: bp-e4df3673-ecd8-47ae-b387-d92fd0190529

Comment 12

[Marcia Knous [:marcia]]

This seems much more visible in the 69 cycle overall - before we branched there were over 500 crashes/150 installs. Several of them I looked at had mozilla::layers::TextureClientRecycleAllocator::CreateOrRecycle(mozilla::layers::ITextureClientAllocationHelper&) in the stack.

Comment 13

[Jessie [:jbonisteel] pls NI]

Nical, curious if this is becoming more possible to investigate if there has been an uptick in crashes?

Comment 14

[Nicolas Silva [:nical]]

Attachment: Bug 1352875 - Ensure the recycle allocator's map is cleared when the lock is held in the destructor. r=sotaro

This shouldn't be necessary since when the destructor runs nothing has access to the map anymore but it at least clears any doubt.

Comment 15

[Sotaro Ikeda [:sotaro]]

I seems similar to Bug 1565255. Since Bug 1565255 was uplifted to beta. The crash of this bug also seemed to be addressed.

Comment 16

[Sotaro Ikeda [:sotaro]]

Since Bug 1565255 fix, a crash did not happen at TextureClientRecycleAllocator::CreateOrRecycle() until now.
There is one crash, Since Bug 1565255 fix https://crash-stats.mozilla.org/report/index/ad2b28bc-b428-4b38-8dac-a97410190730
It happened at VideoBridgeParent::LookupTexture(). TextureClientRecycleAllocator is not related to it.

Comment 17

[Sotaro Ikeda [:sotaro]]

:nical, do you still want to land the patch? The crash seemed to be addressed by Bug 1565255 fix.

Comment 18

[Nicolas Silva [:nical]]

No need to land the patch. Let's close this.

Comment 19

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

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is there anything we can/should do for ESR60/ESR68 yet?

Comment 21

[Nicolas Silva [:nical]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)

Is there anything we can/should do for ESR60/ESR68 yet?

ESR60 should not be affected and for 68 I'd rather not unless the crash volume is very high because the crashing code was removed recently but there's likely a chain of things that we would need to uplift to do the same there and I'm not sure I know the full list of them.