fix: handle X server disconnection gracefully#1649
Closed
fly602 wants to merge 1 commit into
Closed
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602 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 GuideInstalls custom X11 I/O and SIGPIPE handlers (only on xcb) so X server disconnection leads to a clean Qt shutdown instead of an immediate exit, and adds X11 as a build dependency. Sequence diagram for graceful shutdown on X server disconnectionsequenceDiagram
participant XServer
participant libX11
participant dde_shell_main as dde_shell_main
participant QCoreApplication
participant PluginManager
dde_shell_main->>libX11: XSetIOErrorHandler(xio_error_handler)
XServer-->>libX11: [connection broken]
libX11->>dde_shell_main: xio_error_handler(Display)
dde_shell_main->>QCoreApplication: QCoreApplication::quit()
QCoreApplication-->>PluginManager: aboutToQuit
PluginManager->>PluginManager: cleanup_plugins()
QCoreApplication-->>dde_shell_main: exec() returns
dde_shell_main->>dde_shell_main: exit main()
Note over dde_shell_main,QCoreApplication: Custom handler avoids libX11 calling exit() directly
Flow diagram for installing X11 handlers only on xcb platformflowchart TD
A[main] --> B[QGuiApplication::platformName]
B --> C{platformName starts with xcb}
C -- yes --> D["XSetIOErrorHandler(xio_error_handler)"]
D --> E[setup sigaction for SIGPIPE with sigpipe_handler]
C -- no --> F[Skip X11 handlers]
E --> G[a.exec]
F --> G[a.exec]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 found 1 issue, and left some high level feedback:
- The SIGPIPE handler currently calls
qCCriticalandQCoreApplication::quit(), both of which are not async-signal-safe; consider replacing this with setting astd::atomic<bool>/sig_atomic_tflag and reacting in the main event loop instead of doing complex work in the signal handler. - Including
<X11/Xlib.h>unconditionally and adding a REQUIRED X11 pkg-config dependency can break builds on platforms without X11 even when only Wayland is used; consider wrapping the X11-specific code in preprocessor guards and making the X11 dependency optional or conditional on an X11 build option.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The SIGPIPE handler currently calls `qCCritical` and `QCoreApplication::quit()`, both of which are not async-signal-safe; consider replacing this with setting a `std::atomic<bool>`/`sig_atomic_t` flag and reacting in the main event loop instead of doing complex work in the signal handler.
- Including `<X11/Xlib.h>` unconditionally and adding a REQUIRED X11 pkg-config dependency can break builds on platforms without X11 even when only Wayland is used; consider wrapping the X11-specific code in preprocessor guards and making the X11 dependency optional or conditional on an X11 build option.
## Individual Comments
### Comment 1
<location path="shell/main.cpp" line_range="66-68" />
<code_context>
+ return 0;
+}
+
+static void sigpipe_handler(int /*sig*/)
+{
+ qCCritical(dsLog) << "Received SIGPIPE, shutting down gracefully.";
+ QCoreApplication::quit();
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid non-async-signal-safe work in the SIGPIPE handler.
`qCCritical` and `QCoreApplication::quit()` are not async-signal-safe: they depend on Qt internals, likely heap allocations and locks, which must not be used in a signal handler. Instead, have the handler set an `std::atomic<bool>`/`sig_atomic_t` flag or write to a self-pipe, and then perform logging and `quit()` from the main event loop (e.g. via `QSocketNotifier` or a timer). The current approach risks undefined behavior in rare, hard-to-debug scenarios.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+66
to
+68
| static void sigpipe_handler(int /*sig*/) | ||
| { | ||
| qCCritical(dsLog) << "Received SIGPIPE, shutting down gracefully."; |
There was a problem hiding this comment.
issue (bug_risk): Avoid non-async-signal-safe work in the SIGPIPE handler.
qCCritical and QCoreApplication::quit() are not async-signal-safe: they depend on Qt internals, likely heap allocations and locks, which must not be used in a signal handler. Instead, have the handler set an std::atomic<bool>/sig_atomic_t flag or write to a self-pipe, and then perform logging and quit() from the main event loop (e.g. via QSocketNotifier or a timer). The current approach risks undefined behavior in rare, hard-to-debug scenarios.
When X server is killed (e.g., during init 3 to init 5 switch), libX11's default I/O error handler calls exit(), bypassing Qt's normal shutdown. This causes plugins to be destroyed during global destructors when objects are already partially destroyed, leading to SIGSEGV. Changes made: 1. Added X11 package dependency to CMakeLists.txt for Xlib headers and linking 2. Implemented custom X I/O error handler that calls QCoreApplication::quit() instead of exit() 3. Added SIGPIPE signal handler to prevent crashes when X pipe is broken 4. Handlers are installed only on xcb platform, not needed on Wayland This ensures plugins receive aboutToQuit signal and can clean up properly. Log: Fixed crash when X server disconnects by installing custom error handlers Influence: 1. Test graceful shutdown when X server is terminated (init 3 to init 5 switch) 2. Verify no SIGSEGV occurs during X server disconnection 3. Confirm application exits cleanly with proper plugin cleanup 4. Verify fix does not affect Wayland sessions 5. Test that aboutToQuit signal fires correctly for plugin cleanup fix: 优雅处理X服务器断开连接 当X服务器被终止时（例如从init 3切换到init 5），libX11的默认I/O错误处理器 会调用exit()，绕过Qt的正常关闭流程。这导致插件在全局析构函数中被销毁，此 时对象已部分销毁，从而引发SIGSEGV崩溃。 修改内容： 1. 在CMakeLists.txt中添加X11包依赖，引入Xlib头文件和链接库 2. 实现自定义X I/O错误处理器，调用QCoreApplication::quit()替代exit() 3. 添加SIGPIPE信号处理器，防止X管道断开时崩溃 4. 仅在xcb平台安装处理器，Wayland不需要 这确保插件能收到aboutToQuit信号并正确清理资源。 Log: 通过安装自定义错误处理器修复X服务器断开时的崩溃问题 Influence: 1. 测试X服务器终止时的优雅关闭（init 3到init 5切换） 2. 验证X服务器断开时不会出现SIGSEGV崩溃 3. 确认应用程序能正常退出并正确清理插件 4. 验证修复不影响Wayland会话 5. 测试aboutToQuit信号是否正确触发以清理插件 PMS: BUG-363347
deepin pr auto review★ 总体评分：95分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 当前代码已非常优秀，若需进一步提升跨平台编译的隔离性，
// 可将X11头文件引用及函数调用通过宏进行隔离（非必须）：
#ifdef QT_NO_XCB
// Wayland-only build path (if X11 headers are completely absent)
#else
#include <X11/Xlib.h>
static int xio_error_handler(Display * /*dpy*/)
{
qCCritical(dsLog) << "X I/O error detected, shutting down gracefully.";
QCoreApplication::quit();
return 0;
}
#endif
// 在 main 函数中：
#ifndef QT_NO_XCB
if (QGuiApplication::platformName().startsWith(QStringLiteral("xcb"))) {
XSetIOErrorHandler(xio_error_handler);
struct sigaction sa;
memset(&sa, 0, sizeof(sa));
sa.sa_handler = sigpipe_handler;
sigemptyset(&sa.sa_mask);
sa.sa_flags = 0;
sigaction(SIGPIPE, &sa, nullptr);
}
#endif |
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
2 participants
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.
When X server is killed (e.g., during init 3 to init 5 switch), libX11's default I/O error handler calls exit(), bypassing Qt's normal shutdown. This causes plugins to be destroyed during global destructors when objects are already partially destroyed, leading to SIGSEGV.
Changes made:
This ensures plugins receive aboutToQuit signal and can clean up properly.
Log: Fixed crash when X server disconnects by installing custom error handlers
Influence:
fix: 优雅处理X服务器断开连接
当X服务器被终止时（例如从init 3切换到init 5），libX11的默认I/O错误处理器
会调用exit()，绕过Qt的正常关闭流程。这导致插件在全局析构函数中被销毁，此
时对象已部分销毁，从而引发SIGSEGV崩溃。
修改内容：
这确保插件能收到aboutToQuit信号并正确清理资源。
Log: 通过安装自定义错误处理器修复X服务器断开时的崩溃问题
Influence:
PMS: BUG-363347
Summary by Sourcery
Handle X server disconnections gracefully to avoid crashes and ensure clean Qt shutdown.
Bug Fixes:
Build: