Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: re-entracy problem with InvalidateFrameSinkId(). #17658

Conversation

Projects
None yet
5 participants
@miniak
Copy link
Contributor

miniak commented Apr 2, 2019

Description of Change

Backport fix for https://bugs.chromium.org/p/chromium/issues/detail?id=907211
https://chromium-review.googlesource.com/c/chromium/src/+/1352525

Fix re-entracy problem with InvalidateFrameSinkId().

HostFrameSinkManager::InvalidateFrameSinkId() can make a synchronous IPC
to ensure whatever is drawing the platform window is destroyed before
the platform window gets destroyed. However, while waiting for the
synchronous IPC response other synchronous IPCs continue to get
processed. |frame_sink_data_map_| can get mutated in response to a
different synchronous IPC and |data| may no longer point to a valid
memory location when DestroyCompositorFrameSink() returns.

Change the order so that all modifications to |data| happen before the
synchronous IPC. Also add a comment to clarify that FrameSinkIds
shouldn't be reused after they are invalidated. We don't do this today
and it would cause more re-entrancy problems.

Also switch |frame_sink_data_map_| to use std::unordered_map instead of
base::flat_map. This map can get large (tens of elements per open tab)
so std::unordered_map is probably a better choice.

Checklist

Release Notes

Notes: Fixed re-entracy problem with InvalidateFrameSinkId().

/cc @electron/wg-security

@miniak miniak requested a review from electron/wg-security Apr 2, 2019

@miniak miniak self-assigned this Apr 2, 2019

@miniak miniak changed the title fix: Issue 907211: Heap-use-after-free in viz::HostFrameSinkManager::InvalidateFrameSinkId fix: backport 907211 Apr 2, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Apr 3, 2019

@codebytere codebytere changed the title fix: backport 907211 fix: re-entracy problem with InvalidateFrameSinkId(). Apr 4, 2019

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Apr 4, 2019

@miniak can you please fix conflicts so we can get this merged?

@alexeykuzmin alexeykuzmin force-pushed the miniak/fix_re-entracy_problem_with_invalidateframesinkid-4-1-x branch from 8cfa692 to 441a68c Apr 4, 2019

@alexeykuzmin

This comment has been minimized.

Copy link
Contributor

alexeykuzmin commented Apr 4, 2019

@MarshallOfSound MarshallOfSound merged commit c362411 into 4-1-x Apr 4, 2019

8 checks passed

Semantic Pull Request ready to be squashed
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Apr 4, 2019

Release Notes Persisted

Fixed re-entracy problem with InvalidateFrameSinkId().

@MarshallOfSound MarshallOfSound deleted the miniak/fix_re-entracy_problem_with_invalidateframesinkid-4-1-x branch Apr 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.