Repository navigation
fix(control): fix grouped VT leak on destroy request - #8
Merged
Merged
Conversation
|
Skipping CI for Draft Pull Request. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes loss of destroy_group_vt requests (causing grouped VT leaks) by reordering control client event handling to consume readable data before treating sockets as hung up and by removing an incorrect PID-based permission check in the grouped VT destroy path. Sequence diagram for updated control_client_handle_connection event orderingsequenceDiagram
participant EventLoop
participant ControlClient
participant Connection
participant Server
EventLoop->>ControlClient: control_client_handle_connection(fd, mask, client)
alt mask has EVENT_READABLE
ControlClient->>Connection: connection_read(&client->connection)
Connection-->>ControlClient: len > 0
ControlClient->>Server: handle_destroy_group_vt(client)
Server->>Server: seat_destroy_group_vt(seat, request.vt)
end
alt mask has EVENT_HANGUP
ControlClient->>ControlClient: goto fail
end
Flow diagram for handle_destroy_group_vt without PID ownership checkflowchart TD
A["handle_destroy_group_vt(client)"] --> B["seat = seat_get(client)"]
B --> C{seat == NULL?}
C -->|yes| D["control_send_error(client, ENOENT)"]
C -->|no| E["seat_destroy_group_vt(seat, request.vt)"]
E --> F{return value == -1?}
F -->|yes| G["control_send_error(client, errno)"]
F -->|no| H["control_send_success(client)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
glyvut
marked this pull request as ready for review
July 13, 2026 05:10
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Given the removal of the owner_pid permission check, consider adding a brief comment near handle_destroy_group_vt explaining why root-only control access and ddm ownership are sufficient for security, so future readers don’t reintroduce the check.
- In control_client_handle_connection, it may be helpful to clarify in a short comment or assertion that EVENT_HANGUP will not be processed before all readable data is consumed even when both bits are set, to make the intent around fire-and-forget requests explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Given the removal of the owner_pid permission check, consider adding a brief comment near handle_destroy_group_vt explaining why root-only control access and ddm ownership are sufficient for security, so future readers don’t reintroduce the check.
- In control_client_handle_connection, it may be helpful to clarify in a short comment or assertion that EVENT_HANGUP will not be processed before all readable data is consumed even when both bits are set, to make the intent around fire-and-forget requests explicit.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
一个恶意的且拿到了 root 的客户端是在这儿加个判定就能防住的吗 |
Process EVENT_READABLE before EVENT_HANGUP in the control client handler. Checking HANGUP first discards unread data when the peer closes the socket right after sending a fire-and-forget request, causing the destroy opcode to never be dispatched. Also remove the owner_pid != client->pid permission check in handle_destroy_group_vt: the control socket is already restricted to root, and ddm — not Treeland — is the lifecycle owner that issues destroy, so the check always failed with EPERM. 在 control 客户端事件处理中将 EVENT_READABLE 提前到 EVENT_HANGUP 之前。 先检查 HANGUP 会丢弃未读数据，导致 fire-and-forget 请求的 destroy 操作 码无法分发。同时移除 handle_destroy_group_vt 中的 owner_pid != client->pid 权限校验：control socket 已限制仅 root 可连接，且 ddm（非 Treeland）才是发起 destroy 的生命周期管理者，该校验必然返回 EPERM. Log: 修复 grouped VT 销毁请求丢失导致 VT 泄漏 PMS: BUG-367465 Influence: 修复后手动注销时 grouped VT 能正常释放，避免 VT 逐个泄漏耗尽导致登录无响应
deepin pr auto review★ 总体评分：40分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 diff --git a/seatd/control.c b/seatd/control.c
index ae9ce22..537c039 100644
--- a/seatd/control.c
+++ b/seatd/control.c
@@ -226,13 +226,6 @@ static int handle_destroy_group_vt(struct control_client *client) {
if (seat == NULL) {
return control_send_error(client, ENOENT);
}
+ /* 必须保留权限校验，防止越权销毁 VT */
+ pid_t owner_pid = -1;
+ if (seat_get_group_vt_owner_pid(seat, request.vt, &owner_pid) == -1) {
+ return control_send_error(client, errno);
+ }
+ if (owner_pid != client->pid) {
+ return control_send_error(client, EPERM);
+ }
if (seat_destroy_group_vt(seat, request.vt) == -1) {
return control_send_error(client, errno);
}
@@ -322,9 +315,6 @@ static int control_client_handle_connection(int fd, uint32_t mask, void *data) {
if (mask & EVENT_ERROR) {
goto fail;
}
- if (mask & EVENT_HANGUP) {
- goto fail;
- }
if (mask & EVENT_WRITABLE) {
int len = connection_flush(&client->connection);
@@ -337,6 +327,7 @@ static int control_client_handle_connection(int fd, uint32_t mask, void *data) {
}
}
+ /* Process readable data BEFORE acting on HANGUP. */
if (mask & EVENT_READABLE) {
int len = connection_read(&client->connection);
if (len == -1 && errno != EAGAIN) {
@@ -358,6 +349,11 @@ static int control_client_handle_connection(int fd, uint32_t mask, void *data) {
}
}
+ /* All readable data has been consumed. Now honour the hangup. */
+ if (mask & EVENT_HANGUP) {
+ goto fail;
+ }
+
return 0;
fail: |
|
@glyvut: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
zccrs
approved these changes
Jul 13, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: glyvut, zccrs 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 |
Contributor
Author
原因同上。 |
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.
Process EVENT_READABLE before EVENT_HANGUP in the control client handler. Checking HANGUP first discards unread data when the peer closes the socket right after sending a fire-and-forget request, causing the destroy opcode to never be dispatched. Also remove the owner_pid != client->pid permission check in handle_destroy_group_vt: the control socket is already restricted to root, and ddm — not Treeland — is the lifecycle owner that issues destroy, so the check always failed with EPERM.
在 control 客户端事件处理中将 EVENT_READABLE 提前到 EVENT_HANGUP 之前。 先检查 HANGUP 会丢弃未读数据，导致 fire-and-forget 请求的 destroy 操作 码无法分发。同时移除 handle_destroy_group_vt 中的 owner_pid != client->pid 权限校验：control socket 已限制仅 root 可连接，且 ddm（非 Treeland）才是发起 destroy 的生命周期管理者，该校验必然返回 EPERM。
Log: 修复 grouped VT 销毁请求丢失导致 VT 泄漏
PMS: BUG-367465
Influence: 修复后手动注销时 grouped VT 能正常释放，避免 VT 逐个泄漏耗尽导致登录无响应。
Summary by Sourcery
Handle control client readable events before hangup and relax grouped VT destroy permissions to ensure destroy requests are processed and resources freed correctly.
Bug Fixes:
Summary by Sourcery
Ensure grouped VT destroy requests from the control socket are reliably processed so associated VTs are correctly freed.
Bug Fixes: