fix: personalization resource destroy callbacks had inverted null check#808
Open
felixonmars wants to merge 1 commit intolinuxdeepin:masterfrom
Open
fix: personalization resource destroy callbacks had inverted null check#808felixonmars wants to merge 1 commit intolinuxdeepin:masterfrom
felixonmars wants to merge 1 commit intolinuxdeepin:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: felixonmars The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes inverted null checks in all four Wayland personalization wl_resource destroy callbacks so that cleanup code (list removal and context deletion) now runs correctly and avoids heap corruption from dangling list links. Sequence diagram for wl_resource destroy callback fixsequenceDiagram
participant WaylandClient
participant wl_client as wl_client
participant wl_resource as wl_resource
participant Manager as treeland_personalization_manager_v1
WaylandClient->>wl_client: disconnect()
wl_client->>wl_resource: wl_resource_destroy(resource)
wl_resource->>Manager: treeland_personalization_manager_resource_destroy(resource)
alt resource_is_null
Manager-->>Manager: return
else resource_is_not_null
Manager->>Manager: treeland_personalization_manager_v1.from_resource(resource)
Manager->>Manager: wl_list_remove(resource_link)
Manager->>Manager: delete context
end
Flow diagram for corrected null check in destroy callbacksflowchart TD
A[wlr_destroy_callback called with resource] --> B{resource is null?}
B -- Yes --> C[Return immediately]
B -- No --> D[Lookup context via from_resource]
D --> E[Remove resource from manager resources list]
E --> F[Delete context object]
F --> G[Return from destroy callback]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since Wayland destroy callbacks are not invoked with a null wl_resource, consider either removing the null check entirely or replacing it with an assertion to make the intended invariant explicit.
- All four destroy callbacks share identical null-check and cleanup patterns; consider factoring this into a small helper or template to reduce duplication and avoid this class of inverted-condition bug in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since Wayland destroy callbacks are not invoked with a null wl_resource, consider either removing the null check entirely or replacing it with an assertion to make the intended invariant explicit.
- All four destroy callbacks share identical null-check and cleanup patterns; consider factoring this into a small helper or template to reduce duplication and avoid this class of inverted-condition bug in the future.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
58de62e to
dcc6c72
Compare
The wl_resource destroy callbacks in the personalization manager had multiple lifetime and teardown bugs introduced by commit 56c8376. The same class of bugs also existed in font_impl.cpp and appearance_impl.cpp where they had been copy-pasted to new context types: 1. Inverted null check: if (resource) return; caused the destroy handlers to return immediately, so cleanup never ran. (personalization_manager_impl.cpp only) 2. Double wl_list_remove: wl_list_remove(wl_resource_get_link(resource)) is invalid inside the destroy callback because libwayland core (remove_and_destroy_resource) already calls wl_list_remove on resource->link BEFORE invoking the destroy callback. Calling it a second time operates on stale prev/next pointers and corrupts the manager->resources list, producing a heap use-after-free write confirmed by Valgrind: "Invalid write of size 8 / Address is 32 bytes inside a block of size 128 free'd" in wl_list_remove <- remove_and_destroy_resource <- wl_client_destroy <- wl_display_flush_clients. Fixed in: personalization_manager_impl.cpp, font_impl.cpp, appearance_impl.cpp. 3. Missing wl_resource_set_user_data(resource, nullptr): without clearing user data before delete, any from_resource() call inside signal handlers emitted by beforeDestroy() could observe a dangling C++ object pointer. Fixed in: personalization_manager_impl.cpp, font_impl.cpp, appearance_impl.cpp. 4. Shared manager lifetime bug: the manager destroy callback must not delete treeland_personalization_manager_v1 from a single bound wl_resource destroy path, because the manager is shared across bindings/contexts and owned by the display lifetime. (personalization_manager_impl.cpp only) This keeps per-resource context destruction 1:1 with their wl_resource, while leaving the shared manager owned by create()/display teardown.
dcc6c72 to
bf3ecc6
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All 4 wl_resource destroy callbacks in personalization_manager_impl.cpp had an inverted null check introduced by commit 56c8376:
This meant wl_list_remove was never called, leaving freed wl_resource objects as dangling links in manager->resources. When a Wayland client disconnects, wl_client_destroy frees the resource memory while stale links remain in the list. Subsequent wl_list_insert/wl_list_remove operations on the same list then write to freed memory, corrupting the heap and causing crashes.
Fix by inverting the check to if (!resource).
Summary by Sourcery
Bug Fixes: