fix(auth): release daemon descriptors in session child - #104
Merged
Conversation
Reviewer's GuideEnsures the forked session leader closes all inherited daemon file descriptors except the IPC pipe to the parent, preventing stale D-Bus connections from retaining the display manager’s service name and causing registration failures. Sequence diagram for closing inherited daemon file descriptors in session leadersequenceDiagram
participant DaemonProcess
participant SessionLeader
DaemonProcess->>DaemonProcess: fork
DaemonProcess-->>SessionLeader: create session child
SessionLeader->>SessionLeader: delete signalHandler
SessionLeader->>SessionLeader: closeInheritedFileDescriptors(pipefd1)
opt [daemon fds closed except IPC pipe]
SessionLeader->>SessionLeader: proceed to DDM D-Bus registration
end
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:
- In
closeInheritedFileDescriptors, consider handlingsysconf(_SC_OPEN_MAX)returning -1 explicitly before using it as an upper bound, to avoid iterating with an invalid/overflowedmaxFdin edge environments. - The
closeInheritedFileDescriptorshelper currently preserves only a single fd; if future changes introduce multiple pipes or other child-owned descriptors, this function will need to be extended or its contract documented more clearly to avoid inadvertently closing required fds.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `closeInheritedFileDescriptors`, consider handling `sysconf(_SC_OPEN_MAX)` returning -1 explicitly before using it as an upper bound, to avoid iterating with an invalid/overflowed `maxFd` in edge environments.
- The `closeInheritedFileDescriptors` helper currently preserves only a single fd; if future changes introduce multiple pipes or other child-owned descriptors, this function will need to be extended or its contract documented more clearly to avoid inadvertently closing required fds.
## Individual Comments
### Comment 1
<location path="src/daemon/Auth.cpp" line_range="26" />
<code_context>
#include <utmpx.h>
namespace DDM {
+ void closeInheritedFileDescriptors(int preservedFd)
+ {
+#ifdef SYS_close_range
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the closeInheritedFileDescriptors helper to use a dedicated tryCloseInheritedWithCloseRange fast-path function and a simple loop fallback for clearer control flow and range handling.
The helper is doing the right thing functionally, but the control flow and range handling can be made easier to follow without changing behavior.
You can treat `close_range` as an optimization and isolate the range logic in a small helper, so the main function reads as “try fast path, else fall back to simple loop”.
For example:
```c++
namespace {
#ifdef SYS_close_range
bool tryCloseInheritedWithCloseRange(int preservedFd)
{
const unsigned int first = STDERR_FILENO + 1;
const unsigned int last = UINT_MAX;
// No preserved FD in range: just close everything
if (preservedFd <= static_cast<int>(first) || preservedFd > static_cast<int>(last))
return syscall(SYS_close_range, first, last, 0) == 0;
// Close [first, preservedFd - 1]
if (syscall(SYS_close_range,
first,
static_cast<unsigned int>(preservedFd - 1),
0) == -1)
return false;
// Close [preservedFd + 1, last]
if (syscall(SYS_close_range,
static_cast<unsigned int>(preservedFd + 1),
last,
0) == -1)
return false;
return true;
}
#endif
} // namespace
```
Then `closeInheritedFileDescriptors` becomes structurally simpler:
```c++
void closeInheritedFileDescriptors(int preservedFd)
{
#ifdef SYS_close_range
if (tryCloseInheritedWithCloseRange(preservedFd))
return;
#endif
const long maxFd = sysconf(_SC_OPEN_MAX);
for (int fd = STDERR_FILENO + 1; fd < maxFd; ++fd) {
if (fd != preservedFd)
close(fd);
}
}
```
This keeps all current behavior (two ranges when needed, `sysconf`-based fallback) but removes the `closed` flag and early-return logic from the main function, and makes the intent of the `close_range` optimization explicit and easier to reason about.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Close inherited daemon file descriptors in the forked session leader so old D-Bus connections cannot retain the display manager service names. Fail fast when D-Bus registration is unavailable and restore the original service start limit. Log: DDM 通过 fork 创建 session leader 时，子进程若继承 system D-Bus fd，旧 DDM 主进程退出后仍可能保留 org.deepin.DisplayManager 服务名，阻 止新 DDM 注册。session child 仅保留标准流和与父进程通信的 pipe，并关闭 其余 daemon fd；DDM 注册 D-Bus 服务或对象失败时立即退出，避免 systemd 显示 active 而 D-Bus 实际不可用。该修复不处理 systemd start-limit-hit； DDM 被限流停止时必须 reset-failed 后重新启动。 PMS: Influence: Affects session startup and DDM D-Bus ownership across daemon restarts.
deepin pr auto review★ 总体评分：100分■ 【总体评价】
■ 【详细分析】
■ 【改进建议代码示例】 // 当前代码已非常完善，无需修改。以下为保持格式的占位代码示例：
// No changes needed. |
zccrs
approved these changes
Jul 17, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zccrs, zorowk 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 |
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
3 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.
Close inherited daemon file descriptors in the forked session leader so old D-Bus connections cannot retain the display manager service names. Fail fast when D-Bus registration is unavailable and restore the original service start limit.
Log: DDM 通过 fork 创建 session leader 时，子进程若继承 system D-Bus fd，旧 DDM 主进程退出后仍可能保留 org.deepin.DisplayManager 服务名，阻 止新 DDM 注册。session child 仅保留标准流和与父进程通信的 pipe，并关闭
其余 daemon fd；DDM 注册 D-Bus 服务或对象失败时立即退出，避免 systemd
显示 active 而 D-Bus 实际不可用。该修复不处理 systemd start-limit-hit； DDM 被限流停止时必须 reset-failed 后重新启动。
PMS:
Influence: Affects session startup and DDM D-Bus ownership across daemon restarts.
fork之后 父子进程都会拥有 system dbus 的socket fd， 父进程会自己关闭，但是子进程也引用了同样的socket fd 。导致 下次重启的ddm服务无法注册新的 d-bus
Summary by Sourcery
Prevent session leader processes from inheriting daemon-owned file descriptors that keep D-Bus connections alive after the daemon exits.
Bug Fixes:
Enhancements: