fix: harden udev rule file permission and durability on write - #1223
Open
fly602 wants to merge 1 commit into
Open
fix: harden udev rule file permission and durability on write#1223fly602 wants to merge 1 commit into
fly602 wants to merge 1 commit into
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 GuideThe touchpad udev rule write path now explicitly enforces 0644 permissions and syncs both file data and, on first creation, the parent directory. This prevents permissive umasks or widened existing modes from weakening the rule while improving survival across forced power loss. Sequence diagram for durable touchpad udev rule creationsequenceDiagram
participant Touchpad
participant FS as FileSystem
participant RuleFile as UdevRuleFile
participant ParentDir as ParentDirectory
Touchpad->>FS: os.Stat(udevRuleFile)
FS-->>Touchpad: isNew
Touchpad->>FS: os.OpenFile(udevRuleFile, O_WRONLY|O_CREATE|O_TRUNC, 0644)
FS-->>Touchpad: f
Touchpad->>RuleFile: f.Chmod(0644)
Touchpad->>RuleFile: f.Write(udevRuleContent)
Touchpad->>RuleFile: f.Sync()
Touchpad->>RuleFile: f.Close()
alt isNew
Touchpad->>FS: os.Open(filepath.Dir(udevRuleFile))
FS-->>Touchpad: dir
Touchpad->>ParentDir: dir.Sync()
Touchpad->>ParentDir: dir.Close()
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 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="system/inputdevices1/touchpad.go" line_range="152-154" />
<code_context>
logger.Debug("udev rule file already exists with correct content, skip writing")
return nil
}
+ // 标记是否为首次创建，用于后续同步父目录元数据
+ _, statErr := os.Stat(udevRuleFile)
+ isNew := os.IsNotExist(statErr)
- // 创建或覆盖 udev 规则文件，使用 fsync 确保落盘，
- // 防止强制关机（断电）时 page cache 丢失导致规则文件丢失
- f, err := os.Create(udevRuleFile)
+ // 创建或覆盖 udev 规则文件，显式指定 0644 权限，
+ // 防止 os.Create 默认 0666 在宽松 umask 下导致规则文件可被组或全局写入
+ f, err := os.OpenFile(udevRuleFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
if err != nil {
return err
}
+ // 兜底已存在文件权限被改宽的情况
+ if err := f.Chmod(0644); err != nil {
+ f.Close()
+ return err
</code_context>
<issue_to_address>
**🚨 issue (security):** The correct-content early return skips `f.Chmod(0644)`, so an existing rule file with the expected contents but widened permissions remains group- or world-writable.
**Triggers:** When `/etc/udev/rules.d/90-dde-touchpad.rules` has correct contents but mode 0666 or otherwise broader than 0644.
**Suggested fix:** Check and repair the file mode before returning from the correct-content fast path, or perform the permission normalization independently of the content comparison.
```suggestion
existingContent, err := os.ReadFile(udevRuleFile)
if err == nil && string(existingContent) == udevRuleContent {
if err := os.Chmod(udevRuleFile, 0644); err != nil {
return err
}
logger.Debug("udev rule file already exists with correct content, skip writing")
return nil
}
```
</issue_to_address>
### Comment 2
<location path="system/inputdevices1/touchpad.go" line_range="156-164" />
<code_context>
return nil
}
+ // 标记是否为首次创建，用于后续同步父目录元数据
+ _, statErr := os.Stat(udevRuleFile)
+ isNew := os.IsNotExist(statErr)
- // 创建或覆盖 udev 规则文件，使用 fsync 确保落盘，
</code_context>
<issue_to_address>
**issue (bug_risk):** The `os.Stat`/`os.OpenFile` sequence is racy: if the file exists during `os.Stat` but is removed before `os.OpenFile`, the file is newly created with `isNew == false` and the parent directory is never synced.
**Triggers:** When another process or concurrent enable operation removes the rule file between `os.Stat` and `os.OpenFile`.
**Suggested fix:** Determine whether creation occurred atomically, for example by using an exclusive-create attempt or otherwise synchronizing the check and creation before deciding whether to sync the directory.
```suggestion
// 创建或覆盖 udev 规则文件，显式指定 0644 权限，
// 防止 os.Create 默认 0666 在宽松 umask 下导致规则文件可被组或全局写入
f, err := os.OpenFile(udevRuleFile, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0644)
isNew := err == nil
if err != nil && !os.IsExist(err) {
return err
}
if os.IsExist(err) {
f, err = os.OpenFile(udevRuleFile, os.O_WRONLY|os.O_TRUNC, 0644)
}
if err != nil {
return err
}
```
</issue_to_address>
### Comment 3
<location path="system/inputdevices1/touchpad.go" line_range="187-196" />
<code_context>
return err
}
+ // 首次创建文件时，还需 fsync 父目录以确保目录项落盘，
+ // 否则断电后文件数据虽已持久但目录项丢失，规则文件仍会缺失
+ if isNew {
+ dir, err := os.Open(filepath.Dir(udevRuleFile))
+ if err != nil {
+ return err
+ }
+ err = dir.Sync()
</code_context>
<issue_to_address>
**issue (bug_risk):** If opening or syncing the parent directory fails after the file has been written and closed, the function returns an error, but a retry sees the same content and returns at the earlier fast path without retrying the directory sync, so the creation durability guarantee is permanently skipped.
**Triggers:** When the first directory open or `dir.Sync()` fails and the operation is retried with unchanged rule content.
**Suggested fix:** Record or otherwise preserve the need for a directory sync across retries, or avoid treating an unchanged file as fully handled until its directory-entry durability has been established.
</issue_to_address>
### Comment 4
<location path="system/inputdevices1/touchpad.go" line_range="159-166" />
<code_context>
- // 创建或覆盖 udev 规则文件，使用 fsync 确保落盘，
- // 防止强制关机（断电）时 page cache 丢失导致规则文件丢失
- f, err := os.Create(udevRuleFile)
+ // 创建或覆盖 udev 规则文件，显式指定 0644 权限，
+ // 防止 os.Create 默认 0666 在宽松 umask 下导致规则文件可被组或全局写入
+ f, err := os.OpenFile(udevRuleFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644)
if err != nil {
return err
}
+ // 兜底已存在文件权限被改宽的情况
+ if err := f.Chmod(0644); err != nil {
+ f.Close()
+ return err
</code_context>
<issue_to_address>
**issue (bug_risk):** `os.OpenFile` truncates an existing rule before `f.Chmod(0644)` runs; if `Chmod` fails, the function returns an error with the previous valid rule content already erased.
**Triggers:** When the filesystem or file metadata prevents changing the mode after the writable file has been opened.
**Suggested fix:** Avoid truncating until permission normalization succeeds, or write the new content to a temporary file and atomically replace the rule only after all preparatory operations succeed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1. Replace os.Create with os.OpenFile(..., 0644) to avoid 0666 permission regression under a permissive umask 2. Add explicit f.Chmod(0644) to narrow permissions on existing files that may have been widened externally 3. fsync parent directory after first creation so the directory entry survives a forced power loss, not just file data 4. Repair file mode on the content-match fast path so existing rule files with widened permissions are still narrowed to 0644 5. Use O_EXCL to atomically detect first creation, avoiding the Stat/OpenFile TOCTOU race that could skip the parent directory sync when the file is removed concurrently 6. Truncate only after Chmod succeeds, so a Chmod failure cannot erase the previous valid rule content via O_TRUNC 7. Best-effort fsync the parent directory on the fast path to recover when a prior dir.Sync failed and the call is retried 8. Split setTouchpadEnableViaUdev into writeUdevRuleFile and syncDirBestEffort for readability without changing behavior Log: Fix touchpad udev rule file losing durability and permission guarantees introduced by the fsync refactor Influence: 1. Disable touchpad, power off forcibly, reboot and verify the udev rule file still exists and the touchpad stays disabled 2. Check the permission of /etc/udev/rules.d/90-dde-touchpad.rules is 0644 with a permissive umask (e.g. 0000) set on the service 3. Toggle touchpad enable/disable repeatedly and confirm no errors in dde-system-daemon logs fix: 加固触控板 udev 规则文件写入的权限与落盘耐久性 1. 用 os.OpenFile(..., 0644) 替换 os.Create，避免在宽松 umask 下 0666 导致规则文件可被组或全局写入 2. 新增 f.Chmod(0644) 兜底，收窄已被外部改宽的已存在文件权限 3. 首次创建文件后 fsync 父目录，确保目录项在断电时也能落盘 4. 内容匹配的 fast path 同样执行 Chmod(0644)，修复已存在但权限 被改宽的规则文件跳过权限收窄的问题 5. 用 O_EXCL 原子探测首次创建，消除 Stat/OpenFile 之间文件被 并发删除导致 isNew 误判、父目录漏 sync 的 TOCTOU 竞态 6. 先 Chmod 成功再 Truncate，避免 Chmod 失败时 O_TRUNC 已清空 原有效规则内容 7. fast path 补 best-effort 父目录 fsync，覆盖上次创建时 dir.Sync 失败后重试永久跳过目录项持久化的场景 8. 将 setTouchpadEnableViaUdev 拆分为 writeUdevRuleFile 与 syncDirBestEffort，提升可读性，行为不变 Log: 修复 fsync 重构引入的触控板 udev 规则文件权限退化与 断电后目录项丢失问题 Influence: 1. 禁用触控板后强制断电重启，验证 udev 规则文件仍存在且触控板保持禁用 2. 在服务设置宽松 umask（如 0000）下检查 /etc/udev/rules.d/90-dde-touchpad.rules 权限为 0644 3. 反复启用/禁用触控板，确认 dde-system-daemon 日志无报错 PMS: BUG-374789
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.
Log: Fix touchpad udev rule file losing durability and permission guarantees introduced by the fsync refactor
Influence:
fix: 加固触控板 udev 规则文件写入的权限与落盘耐久性
Log: 修复 fsync 重构引入的触控板 udev 规则文件权限退化与
断电后目录项丢失问题
Influence:
PMS: BUG-374789
Summary by Sourcery
Harden touchpad udev rule file writes to preserve permission and durability guarantees.
Bug Fixes:
Enhancements: