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

protocols: Fix IWaylandProtocol onDisplayDestroy m_pGlobal double-free #9507

Merged
merged 1 commit into from
Feb 28, 2025

Conversation

PlasmaPower
Copy link
Contributor

@PlasmaPower PlasmaPower commented Feb 27, 2025

Describe your PR, what does it fix/add?

This fixes a segfault on Hyprland shutdown caused by the wl_global_destroy call modified in this PR:

#0  0x00007a75f51443af in wl_list_remove (elm=elm@entry=0x5ec7ca5e15a8) at ../wayland-1.23.1/src/wayland-util.c:56
#1  0x00007a75f5145cb0 in wl_global_destroy (global=0x5ec7ca5e1580) at ../wayland-1.23.1/src/wayland-server.c:1399
#2  0x00005ec78c6d7017 in IWaylandProtocol::onDisplayDestroy (this=0x5ec7ca5e1620)
    at /home/lee/code/cpp/Hyprland/src/protocols/WaylandProtocol.cpp:17
#3  0x00005ec78c6d7707 in IWaylandProtocol::~IWaylandProtocol (this=0x5ec7ca5e1620)
    at /home/lee/code/cpp/Hyprland/src/protocols/WaylandProtocol.cpp:37

Looking at the code, m_pGlobal will be destroyed first when the display is destroyed by the onDisplayDestroy hook, and then a second time when the wayland protocol is destroyed (if that happens later) by the IWaylandProtocol destructor calling onDisplayDestroy a second time. This PR fixes that by setting m_pGlobal to nullptr after destroying it and adding null checks.

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

I don't think the m_pGlobal null check in removeGlobal is necessary, but better safe than sorry I figure.

The m_liDisplayDestroy.listener.link removal is fine as-is because it's already re-init'ed afterwards in the existing code, meaning that the second removal will be a no-op.

Is it ready for merging, or does it need work?

Ready for merging

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

luv me sum c, thanks!

@vaxerski vaxerski merged commit 445337d into hyprwm:main Feb 28, 2025
12 checks passed
@PlasmaPower PlasmaPower deleted the fix-display-destroy-global branch February 28, 2025 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants