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(macos): add reference count to layer to prevent use after free #304

Closed
wants to merge 1 commit into from

Conversation

wusyong
Copy link
Member

@wusyong wusyong commented Aug 4, 2024

On macOS, winit will try to release all related resources after a window is dropped. And surfman's macos system surface also contains wrappers of CALayer for its layer and superlayer field which will also try to free when dropping. If either winit window or Surfman surface decides to drop, it will cause use after free to each other.

If winit window is dropped first, surfman will get:

* thread #1, name = 'main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x8)
  * frame #0: 0x000000019ea66990 QuartzCore`CA::Layer::update_removed_sublayer(CA::Transaction*, unsigned int) + 60
    frame #1: 0x000000019ea60bec QuartzCore`CA::Layer::update_sublayers(CA::Transaction*, CALayerArray*, CALayerArray*) + 700
    frame #2: 0x000000019ea83130 QuartzCore`CA::Layer::destroy() + 124
    frame #3: 0x000000019ea8304c QuartzCore`-[CALayer dealloc] + 96

If surfman surface is dropped first, winit window will get:

* thread #1, name = 'main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x34)
  * frame #0: 0x000000019ec9d0c4 QuartzCore`CA::Layer::mark_visible(CA::Transaction*, bool, bool) + 28
    frame #1: 0x000000019ec9d22c QuartzCore`CA::Layer::mark_visible(CA::Transaction*, bool, bool) + 388
    frame #2: 0x000000019ea669bc QuartzCore`CA::Layer::update_removed_sublayer(CA::Transaction*, unsigned int) + 104
    frame #3: 0x000000019ea66848 QuartzCore`CA::Layer::remove_sublayer(CA::Transaction*, CALayer*) + 136
    frame #4: 0x000000019ea58eb0 QuartzCore`CA::Layer::remove_from_superlayer() + 172
    frame #5: 0x000000019a0d44e8 AppKit`-[NSView _removeLayerFromSuperlayer] + 252
    frame #6: 0x000000019a0d3ef0 AppKit`-[NSView _setSuperview:] + 280
    frame #7: 0x000000019a0f40c8 AppKit`-[NSView removeFromSuperview] + 156
    frame #8: 0x000000019a152a44 AppKit`-[NSView removeFromSuperviewWithoutNeedingDisplay] + 44
    frame #9: 0x000000019a0f8800 AppKit`-[NSView _finalize] + 680
    frame #10: 0x000000019a0f8440 AppKit`-[NSView dealloc] + 128
    frame #11: 0x000000019a331bb4 AppKit`-[NSFrameView dealloc] + 164
    frame #12: 0x000000019a331b00 AppKit`-[NSTitledFrame dealloc] + 72
    frame #13: 0x000000019a331a7c AppKit`-[NSThemeFrame dealloc] + 624
    frame #14: 0x0000000198082118 Foundation`_NSKVOPerformWithDeallocatingObservable + 172

This PR tries to add a reference count when creating the surface. This should set the correct reference count for both of them to drop.

@@ -196,6 +196,7 @@ impl Device {
transaction::set_disable_actions(true);

let superlayer = CALayer::new();
let _: () = msg_send![superlayer.id(), retain];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does CALayer::new return a CALayer with a reference? If so, won't this leak the CALayer? If that's the case, maybe the layer reference count increase should happen in the interface between surfman and winit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that could happen if the native widget is from other sources and they don't clean up their window entirely like winit.
How about a platform-specific method like retain_layer? In this case, I can call it for winit usage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be fixed by servo/core-foundation-rs#546.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thanks! I'll track the coacoa release and close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants