fix: add error handling when sourcing profile scripts#91
Closed
wineee wants to merge 1 commit into
Closed
Conversation
Suppress stderr and emit a warning to stderr when sourcing /etc/profile, /etc/xprofile, /usr/local/etc/xprofile, $HOME/.profile, $HOME/.xprofile, $HOME/.xsessionrc, or $HOME/.xsession fails, preventing broken user configs from crashing the session startup.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wineee 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 GuideAdds defensive error handling around sourcing profile and session scripts in Xsession, suppressing stderr and printing explicit warnings instead of letting failing user configs abort session startup. Sequence diagram for updated error handling when sourcing profile scriptssequenceDiagram
actor User
participant Xsession
participant Profile_script
User->>Xsession: start_session
Xsession->>Profile_script: source_/etc/profile
alt source succeeds
Profile_script-->>Xsession: environment_loaded
else source fails
Xsession->>Xsession: echo_warning
Xsession->>Xsession: [continue startup]
end
Xsession->>Profile_script: source_user_xsession
alt source succeeds
Profile_script-->>Xsession: session_config_loaded
else source fails
Xsession->>Xsession: echo_warning
Xsession->>Xsession: [continue startup]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review你好！我是CodeGeeX。我已仔细审查了你提供的 以下是我从语法逻辑、代码质量、代码性能和代码安全四个维度提出的详细审查意见和改进建议： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
综合改进后的代码示例结合以上分析，我为你重构了这部分代码，使其更健壮、更安全、更易维护： #!/bin/sh
# 定义安全的 source 函数，避免代码冗余，同时保留原始错误输出
source_safe() {
if ! . "$1"; then
echo "Warning: Failed to source $1" >&2
fi
}
case $SHELL in
*/bash)
# bash 不需要 source profile，它作为 login shell 会自动处理
;;
*/zsh)
# zsh 也不需要，它有自己的 ZDOTDIR 机制
;;
*/csh|*/tcsh)
# ... (保持原样)
;;
*/fish)
[ -f /etc/profile ] && source_safe /etc/profile
[ -f "$HOME/.profile" ] && source_safe "$HOME/.profile"
# 改进临时文件处理：使用 trap 确保清理，变量加引号
xsess_tmp=$(mktemp) || { echo "Error: Failed to create temp file" >&2; exit 1; }
trap 'rm -f "$xsess_tmp"' EXIT INT TERM
# 注意：此处 fish 输出重定向到 $xsess_tmp 是安全的，因为文件由当前用户创建
if ! $SHELL --login -c "/bin/sh -c 'export -p'" > "$xsess_tmp" 2>/dev/null; then
echo "Warning: Failed to export fish environment" >&2
else
source_safe "$xsess_tmp"
fi
rm -f "$xsess_tmp"
trap - EXIT INT TERM
;;
*) # Plain sh, ksh, and anything we do not know.
[ -f /etc/profile ] && source_safe /etc/profile
[ -f "$HOME/.profile" ] && source_safe "$HOME/.profile"
;;
esac
[ -f /etc/xprofile ] && source_safe /etc/xprofile
[ -f /usr/local/etc/xprofile ] && source_safe /usr/local/etc/xprofile
[ -f "$HOME/.xprofile" ] && source_safe "$HOME/.xprofile"
# run all system xinitrc shell scripts.
if [ -d /etc/X11/xinit/xinitrc.d ]; then
# ... (保持原样)
fi
[ -f "$HOME/.Xresources" ] && xrdb -merge "$HOME/.Xresources"
if [ -f "$USERXSESSIONRC" ]; then
source_safe "$USERXSESSIONRC"
fi
if [ -f "$USERXSESSION" ]; then
source_safe "$USERXSESSION"
fi
# ... (后续代码)总结： |
![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 2 issues, and left some high level feedback:
- File paths like
$HOME/.profile,$HOME/.xprofile, and/usr/local/etc/xprofileshould be consistently quoted in the-ftests (e.g.,[ -f "$HOME/.profile" ]) to avoid issues if any part of the path contains spaces or special characters. - The repeated
source + suppress stderr + warnpattern could be wrapped in a small helper function (e.g.,safe_source <path>) to centralize the behavior and avoid duplicating the same logic in multiple places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- File paths like `$HOME/.profile`, `$HOME/.xprofile`, and `/usr/local/etc/xprofile` should be consistently quoted in the `-f` tests (e.g., `[ -f "$HOME/.profile" ]`) to avoid issues if any part of the path contains spaces or special characters.
- The repeated `source + suppress stderr + warn` pattern could be wrapped in a small helper function (e.g., `safe_source <path>`) to centralize the behavior and avoid duplicating the same logic in multiple places.
## Individual Comments
### Comment 1
<location path="data/scripts/Xsession" line_range="29" />
<code_context>
*/fish)
- [ -f /etc/profile ] && . /etc/profile
- [ -f $HOME/.profile ] && . $HOME/.profile
+ [ -f /etc/profile ] && { . /etc/profile 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; }
+ [ -f $HOME/.profile ] && { . $HOME/.profile 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; }
xsess_tmp=`mktemp /tmp/xsess-env-XXXXXX`
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Quoting file paths would make these conditionals more robust
Since these lines are already being changed, please quote the paths in the `-f` tests and when sourcing, to avoid issues if any path ever contains whitespace or glob characters. For example:
```sh
[ -f "/etc/profile" ] && { . "/etc/profile" 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; }
[ -f "$HOME/.profile" ] && { . "$HOME/.profile" 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; }
```
The same adjustment applies to the other similar conditionals in this file.
Suggested implementation:
```
*/fish)
[ -f "/etc/profile" ] && { . "/etc/profile" 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; }
[ -f "$HOME/.profile" ] && { . "$HOME/.profile" 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; }
xsess_tmp=`mktemp /tmp/xsess-env-XXXXXX`
$SHELL --login -c "/bin/sh -c 'export -p' > $xsess_tmp"
. $xsess_tmp
rm -f $xsess_tmp
;;
*) # Plain sh, ksh, and anything we do not know.
[ -f "/etc/profile" ] && { . "/etc/profile" 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; }
[ -f "$HOME/.profile" ] && { . "$HOME/.profile" 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; }
;;
```
Search the rest of `data/scripts/Xsession` for other `[ -f ... ] && . ...` patterns (e.g. for other shells or configuration files) and apply the same quoting convention: quote the path in the `-f` test and in the `.` (source) command, e.g. `[ -f "/path" ] && . "/path"`.
</issue_to_address>
### Comment 2
<location path="data/scripts/Xsession" line_range="84-87" />
<code_context>
if [ -f "$USERXSESSIONRC" ]; then
- . "$USERXSESSIONRC"
+ { . "$USERXSESSIONRC" 2>/dev/null || echo "Warning: Failed to source $USERXSESSIONRC" >&2; }
fi
if [ -f "$USERXSESSION" ]; then
- . "$USERXSESSION"
+ { . "$USERXSESSION" 2>/dev/null || echo "Warning: Failed to source $USERXSESSION" >&2; }
fi
</code_context>
<issue_to_address>
**question (bug_risk):** Treating any non-zero exit from user session scripts as a failure may be too strict
Previously, these scripts’ exit codes were simply propagated. With the new `|| echo` wrapper, any non-zero exit (including intentional early `return/exit` used by users) now emits a warning, which may introduce noisy logs or surprise users who rely on non-zero exits for control flow. Consider limiting warnings to specific failure modes (e.g., file-not-found, permission denied) or clarifying in documentation that user session scripts should exit 0 for expected early termination.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| */fish) | ||
| [ -f /etc/profile ] && . /etc/profile | ||
| [ -f $HOME/.profile ] && . $HOME/.profile | ||
| [ -f /etc/profile ] && { . /etc/profile 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; } |
There was a problem hiding this comment.
suggestion (bug_risk): Quoting file paths would make these conditionals more robust
Since these lines are already being changed, please quote the paths in the -f tests and when sourcing, to avoid issues if any path ever contains whitespace or glob characters. For example:
[ -f "/etc/profile" ] && { . "/etc/profile" 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; }
[ -f "$HOME/.profile" ] && { . "$HOME/.profile" 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; }The same adjustment applies to the other similar conditionals in this file.
Suggested implementation:
*/fish)
[ -f "/etc/profile" ] && { . "/etc/profile" 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; }
[ -f "$HOME/.profile" ] && { . "$HOME/.profile" 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; }
xsess_tmp=`mktemp /tmp/xsess-env-XXXXXX`
$SHELL --login -c "/bin/sh -c 'export -p' > $xsess_tmp"
. $xsess_tmp
rm -f $xsess_tmp
;;
*) # Plain sh, ksh, and anything we do not know.
[ -f "/etc/profile" ] && { . "/etc/profile" 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; }
[ -f "$HOME/.profile" ] && { . "$HOME/.profile" 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; }
;;
Search the rest of data/scripts/Xsession for other [ -f ... ] && . ... patterns (e.g. for other shells or configuration files) and apply the same quoting convention: quote the path in the -f test and in the . (source) command, e.g. [ -f "/path" ] && . "/path".
Comment on lines
+84
to
+87
| { . "$USERXSESSIONRC" 2>/dev/null || echo "Warning: Failed to source $USERXSESSIONRC" >&2; } | ||
| fi | ||
| if [ -f "$USERXSESSION" ]; then | ||
| . "$USERXSESSION" | ||
| { . "$USERXSESSION" 2>/dev/null || echo "Warning: Failed to source $USERXSESSION" >&2; } |
There was a problem hiding this comment.
question (bug_risk): Treating any non-zero exit from user session scripts as a failure may be too strict
Previously, these scripts’ exit codes were simply propagated. With the new || echo wrapper, any non-zero exit (including intentional early return/exit used by users) now emits a warning, which may introduce noisy logs or surprise users who rely on non-zero exits for control flow. Consider limiting warnings to specific failure modes (e.g., file-not-found, permission denied) or clarifying in documentation that user session scripts should exit 0 for expected early termination.
There was a problem hiding this comment.
Pull request overview
This PR updates the session startup scripts to make sourcing of system/user profile files non-fatal by suppressing stderr and emitting a warning when sourcing “fails”, so that broken user configs don’t disrupt session startup.
Changes:
- Wrap sourcing of
/etc/profileand$HOME/.profilein bothdata/scripts/Xsessionanddata/scripts/wayland-sessionto silence stderr and print a warning on failure. - Wrap sourcing of X-specific profile/session scripts in
data/scripts/Xsession(e.g.,/etc/xprofile,$HOME/.xprofile,$HOME/.xsessionrc,$HOME/.xsession) with the same warning behavior.
中文概述
此 PR 修改会话启动脚本，使 source 系统/用户 profile 文件在失败时不再影响会话启动：通过抑制 stderr，并在检测到“失败”时向 stderr 输出告警，从而避免用户配置问题干扰会话启动流程。
变更：
- 在
data/scripts/Xsession与data/scripts/wayland-session中为/etc/profile与$HOME/.profile的 source 增加 stderr 抑制与失败告警。 - 在
data/scripts/Xsession中为 X 相关 profile/会话脚本（如/etc/xprofile、$HOME/.xprofile、$HOME/.xsessionrc、$HOME/.xsession）增加同样的容错告警逻辑。
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
data/scripts/Xsession |
Add guarded sourcing + stderr suppression + warning messages for multiple profile/session scripts / 为多个 profile/会话脚本增加容错 source 与告警 |
data/scripts/wayland-session |
Add guarded sourcing + stderr suppression + warning messages for /etc/profile and $HOME/.profile / 为 /etc/profile 与 $HOME/.profile 增加容错 source 与告警 |
Comment on lines
+29
to
+30
| [ -f /etc/profile ] && { . /etc/profile 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; } | ||
| [ -f $HOME/.profile ] && { . $HOME/.profile 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; } |
Comment on lines
+37
to
+38
| [ -f /etc/profile ] && { . /etc/profile 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; } | ||
| [ -f $HOME/.profile ] && { . $HOME/.profile 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; } |
| [ -f $HOME/.xprofile ] && . $HOME/.xprofile | ||
| [ -f /etc/xprofile ] && { . /etc/xprofile 2>/dev/null || echo "Warning: Failed to source /etc/xprofile" >&2; } | ||
| [ -f /usr/local/etc/xprofile ] && { . /usr/local/etc/xprofile 2>/dev/null || echo "Warning: Failed to source /usr/local/etc/xprofile" >&2; } | ||
| [ -f $HOME/.xprofile ] && { . $HOME/.xprofile 2>/dev/null || echo "Warning: Failed to source $HOME/.xprofile" >&2; } |
|
|
||
| if [ -f "$USERXSESSIONRC" ]; then | ||
| . "$USERXSESSIONRC" | ||
| { . "$USERXSESSIONRC" 2>/dev/null || echo "Warning: Failed to source $USERXSESSIONRC" >&2; } |
| fi | ||
| if [ -f "$USERXSESSION" ]; then | ||
| . "$USERXSESSION" | ||
| { . "$USERXSESSION" 2>/dev/null || echo "Warning: Failed to source $USERXSESSION" >&2; } |
Comment on lines
+33
to
+34
| [ -f /etc/profile ] && { . /etc/profile 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; } | ||
| [ -f $HOME/.profile ] && { . $HOME/.profile 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; } |
Comment on lines
+41
to
+42
| [ -f /etc/profile ] && { . /etc/profile 2>/dev/null || echo "Warning: Failed to source /etc/profile" >&2; } | ||
| [ -f $HOME/.profile ] && { . $HOME/.profile 2>/dev/null || echo "Warning: Failed to source $HOME/.profile" >&2; } |
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.
Suppress stderr and emit a warning to stderr when sourcing /etc/profile, /etc/xprofile, /usr/local/etc/xprofile, $HOME/.profile, $HOME/.xprofile, $HOME/.xsessionrc, or $HOME/.xsession fails, preventing broken user configs from crashing the session startup.
Summary by Sourcery
Handle failures when sourcing user and system profile scripts during X session startup to avoid crashing the session.
Bug Fixes: