Skip to content

Fix menu-actions GTK callback pointer handling#4925

Merged
Alexays merged 3 commits intoAlexays:masterfrom
B2krobbery:fix-menu-actions-pointer
Mar 18, 2026
Merged

Fix menu-actions GTK callback pointer handling#4925
Alexays merged 3 commits intoAlexays:masterfrom
B2krobbery:fix-menu-actions-pointer

Conversation

@B2krobbery
Copy link
Copy Markdown
Contributor

@B2krobbery B2krobbery commented Mar 16, 2026

Fix pointer lifetime handling for menu-actions GTK callbacks.

The previous implementation passed a temporary pointer (c_str()) to the GTK callback, which could become invalid after the function scope ended.

This patch ensures the pointer remains valid by allocating a copy of the string using g_strdup, providing a stable memory reference for the callback.

Verified build locally and tested with Waybar.

@B2krobbery B2krobbery force-pushed the fix-menu-actions-pointer branch 3 times, most recently from 2c2f6f9 to 69b3e5e Compare March 16, 2026 18:13
@B2krobbery B2krobbery force-pushed the fix-menu-actions-pointer branch from 69b3e5e to 3cfb622 Compare March 16, 2026 18:16
@Alexays Alexays merged commit b77b181 into Alexays:master Mar 18, 2026
9 checks passed
@staticssleever668
Copy link
Copy Markdown
Contributor

@B2krobbery

could become invalid after the function scope ended

"could" in what case exactly? This is the constructor and menuActionsMap_ is a member variable never reassigned later.
This is just leaking memory for no gain. Try valgrind --leak-check=yes -- ./build/waybar and look for
waybar::ALabel::ALabel.
The code in this PR is also incorrectly formatted. The CI job clang-format / lint might be broken.
@Alexays sorry for the ping but this PR should be reverted.

@B2krobbery
Copy link
Copy Markdown
Contributor Author

The intention here was to be defensive about the data passed to the GTK callback.

While menuActionsMap_ does persist for the lifetime of the object, relying on implicit lifetime guarantees can be fragile depending on how the code evolves. The change was aimed at making the ownership explicit and avoiding potential issues if the storage pattern changes in the future.

That said, I understand your point regarding the current implementation.

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