Skip to content

fix: Fix client crash when created via security context#555

Merged
zccrs merged 1 commit intolinuxdeepin:masterfrom
zccrs:security_context
Sep 15, 2025
Merged

fix: Fix client crash when created via security context#555
zccrs merged 1 commit intolinuxdeepin:masterfrom
zccrs:security_context

Conversation

@zccrs
Copy link
Member

@zccrs zccrs commented Sep 12, 2025

  1. Fixed a crash issue where clients created through the security context could not obtain the corresponding WClient object.
  2. Clients created via security context use a new socket connection, which is not managed by WSocket by default.
  3. The wlroots protocol provides limited information for monitoring such wl_clients.
  4. This commit adapts the wlroots implementation: in security_context_handle_commit, a new WSocket object is created for the listening socket, and the new wl_client object created from this socket is added to WClient for management.

Influence:

  1. Test the creation of clients using the security context.
  2. Verify that the created clients can correctly obtain the WClient object.
  3. Ensure that newly created sockets are correctly managed.

修复:修复通过安全上下文创建的客户端崩溃问题

  1. 修复了通过安全上下文创建的客户端无法获取对应的 WClient 对象导致崩溃的 问题。
  2. 通过安全上下文创建的客户端使用新的套接字连接,默认情况下不由 WSocket 管理。
  3. wlroots 协议为监控此类 wl_clients 提供的相关信息有限。
  4. 本次提交采纳了 wlroots 的实现:在 security_context_handle_commit 中,为监听的套接字创建一个新的 WSocket 对象,并将由此套接字创建的新
    wl_client 对象添加到 WClient 中进行管理。

Influence:

  1. 测试使用安全上下文创建客户端的过程。
  2. 验证创建的客户端能否正确获取 WClient 对象。
  3. 确保新创建的套接字能够得到正确的管理。

Summary by Sourcery

Support the security-context-v1 Wayland protocol, fix client creation crash in security contexts, and extend socket/client classes to propagate security metadata

New Features:

  • Implement WSecurityContextManager to handle security-context-v1 protocol and bind listener sockets
  • Create new WSocket instances for committed security contexts and manage their client connections
  • Expose sandbox engine, app ID, and instance ID properties on WSocket and WClient

Bug Fixes:

  • Fix crash when clients are created via a security context by ensuring they receive a managed WClient object

Enhancements:

  • Add WSocket constructors and Q_PROPERTYs for parentSocket, rootSocket, sandboxEngine, appId, and instanceId
  • Register security context manager attachment in server initialization

Build:

  • Add security-context-v1 XML to CMakeLists and generate corresponding protocol sources

@zccrs zccrs requested review from ComixHe, wineee and zzxyb September 12, 2025 07:39
@sourcery-ai
Copy link

sourcery-ai bot commented Sep 12, 2025

Reviewer's Guide

This PR adds full support for the wp_security_context_v1 protocol by generating its Wayland bindings, introducing WSecurityContextManager, extending WSocket and WClient to carry security context metadata, and updating the commit handler to create and bind dedicated sockets under WClient management to prevent crashes for clients created via a security context.

File-Level Changes

Change Details Files
Extend WSocket and WClient to carry security context metadata
  • Add sandboxEngine, appId, instanceId fields to WSocketPrivate
  • Expose new metadata via Q_PROPERTY in WSocket and WClient headers
  • Implement getters in WSocket and WClient source files
waylib/src/server/kernel/wsocket.h
waylib/src/server/kernel/wsocket.cpp
Refactor WSocket constructors and properties for parent/child sockets
  • Remove parentSocket parameter from primary constructor
  • Add overloads for parent-only and metadata-initializing constructors
  • Adjust Q_PROPERTY definitions and remove parent setter from public API
  • Update WXWayland instantiation to use new constructor signature
waylib/src/server/kernel/wsocket.h
waylib/src/server/kernel/wsocket.cpp
waylib/src/server/protocols/wxwayland.cpp
Add security-context-v1 protocol generation and integration
  • Generate security-context-v1 protocol in CMakeLists
  • Include and attach WSecurityContextManager in Helper::init
  • Expose commit signal in qwlroots security context manager stub
waylib/src/server/CMakeLists.txt
src/seat/helper.cpp
qwlroots/src/types/qwsecuritycontextmanagerv1.h
Implement WSecurityContextManager and commit handler logic
  • Add new WSecurityContextManager class and Wayland backend in protocols
  • Copy wlroots security context implementation and emit commit events
  • In commit handler, create a new WSocket, bind listen fd, and add new wl_client to WClient
waylib/src/server/protocols/wsecuritycontextmanager.cpp
waylib/src/server/protocols/wsecuritycontextmanager.h

Possibly linked issues

  • #30629: The PR fixes a crash where clients created via security context were not properly managed, which can lead to the wl_event_loop_dispatch crash reported in the issue.
  • Init repository #1: The PR fixes a client crash by correctly associating WClient objects with new sockets created via security context, which resolves the Xwayland crash in the issue.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `waylib/src/server/kernel/wsocket.cpp:124` </location>
<code_context>
+        , parentSocket(nullptr)
+    {}
+
+    WSocketPrivate(WSocket *qq, WSocket *parent)
+        : WObjectPrivate(qq)
+        , freezeClientWhenDisable(WSocketPrivate::get(parent)->freezeClientWhenDisable)
         , parentSocket(parent)
</code_context>

<issue_to_address>
The constructor copies freezeClientWhenDisable from parent without null check.

A null check for parent should be added, or ensure parent is always valid to prevent potential crashes.
</issue_to_address>

### Comment 2
<location> `waylib/src/server/protocols/wsecuritycontextmanager.cpp:178` </location>
<code_context>
+    wl_list_remove(&security_context->link);
+    free(security_context);
+
+    security_context->socket->deleteLater();
+}
+
</code_context>

<issue_to_address>
deleteLater may not be safe if called from non-Qt thread context.

Call deleteLater only from the main Qt thread, or use a thread-safe alternative such as delete or a queued connection to prevent crashes.
</issue_to_address>

### Comment 3
<location> `waylib/src/server/kernel/wsocket.h:71` </location>
<code_context>
     Q_PROPERTY(QString fullServerName READ fullServerName NOTIFY fullServerNameChanged FINAL)
-    Q_PROPERTY(WSocket* parentSocket READ parentSocket WRITE setParentSocket NOTIFY parentSocketChanged FINAL)
+    Q_PROPERTY(WSocket* parentSocket READ parentSocket NOTIFY parentSocketChanged FINAL)
+    Q_PROPERTY(WSocket* rootSocket READ rootSocket CONSTANT FINAL)
+    Q_PROPERTY(QByteArray sandboxEngine READ sandboxEngine CONSTANT FINAL)
+    Q_PROPERTY(QByteArray appId READ appId CONSTANT FINAL)
</code_context>

<issue_to_address>
rootSocket property is declared but no implementation is shown.

Please add the rootSocket accessor to ensure the property works as intended and does not cause runtime issues.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@zccrs zccrs force-pushed the security_context branch 3 times, most recently from eb90805 to 18385aa Compare September 15, 2025 03:55
1. Fixed a crash issue where clients created through the security
context could not obtain the corresponding WClient object.
2. Clients created via security context use a new socket connection,
which is not managed by WSocket by default.
3. The wlroots protocol provides limited information for monitoring
such wl_clients.
4. This commit adapts the wlroots implementation: in
`security_context_handle_commit`, a new WSocket object is created for
the listening socket, and the new wl_client object created from this
socket is added to WClient for management.

Influence:
1. Test the creation of clients using the security context.
2. Verify that the created clients can correctly obtain the WClient
object.
3. Ensure that newly created sockets are correctly managed.

修复:修复通过安全上下文创建的客户端崩溃问题

1. 修复了通过安全上下文创建的客户端无法获取对应的 WClient 对象导致崩溃的
问题。
2. 通过安全上下文创建的客户端使用新的套接字连接,默认情况下不由 WSocket
管理。
3. wlroots 协议为监控此类 wl_clients 提供的相关信息有限。
4. 本次提交采纳了 wlroots 的实现:在 `security_context_handle_commit`
中,为监听的套接字创建一个新的 WSocket 对象,并将由此套接字创建的新
wl_client 对象添加到 WClient 中进行管理。

Influence:
1. 测试使用安全上下文创建客户端的过程。
2. 验证创建的客户端能否正确获取 WClient 对象。
3. 确保新创建的套接字能够得到正确的管理。
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ComixHe, zccrs, zzxyb

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zccrs zccrs merged commit 9556ef8 into linuxdeepin:master Sep 15, 2025
12 of 14 checks passed
@zccrs zccrs deleted the security_context branch September 15, 2025 06:48
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.

4 participants