Skip to content

refactor(wacom): replace shell command with direct exec call#189

Open
mhduiy wants to merge 1 commit intolinuxdeepin:masterfrom
mhduiy:sec
Open

refactor(wacom): replace shell command with direct exec call#189
mhduiy wants to merge 1 commit intolinuxdeepin:masterfrom
mhduiy:sec

Conversation

@mhduiy
Copy link
Copy Markdown
Contributor

@mhduiy mhduiy commented May 8, 2026

  1. Replace doAction(string) with doAction(args ...string) to avoid shell injection risk
  2. Refactor all Wacom set methods to pass arguments individually instead of using fmt.Sprintf to build shell commands
  3. Remove exec.Command("/bin/sh", "-c", cmd) in favor of exec.Command(cmdXSetWacom, args...) for safer command execution
  4. Sort imports and add strconv/errors usage for integer/string conversions

Log: Replace unsafe shell command construction with direct exec.Command calls in wacom module

refactor(wacom): 用直接 exec 调用替换 shell 命令拼接

  1. doAction(string) 重构为 doAction(args ...string) 以消除 shell 注入风险
  2. 重构所有 Wacom set 方法,改为逐个传参而非使用 fmt.Sprintf 拼接 shell 命令
  3. 移除 exec.Command("/bin/sh", "-c", cmd) 改用 exec.Command(cmdXSetWacom, args...) 执行命令
  4. 调整 import 排序,添加 strconv/errors 用于整数/字符串转换

Log: 将 wacom 模块中不安全的 shell 命令拼接替换为直接 exec.Command 调用
Change-Id: Ia4978b3f2920dd8f005dc6f1cff45470d956bb65

Copy link
Copy Markdown

@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.

Sorry @mhduiy, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhduiy

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

1. Replace `doAction(string)` with `doAction(args ...string)` to avoid shell injection risk
2. Refactor all Wacom set methods to pass arguments individually instead of using fmt.Sprintf to build shell commands
3. Remove `exec.Command("/bin/sh", "-c", cmd)` in favor of `exec.Command(cmdXSetWacom, args...)` for safer command execution
4. Sort imports and add strconv/errors usage for integer/string conversions

Log: Replace unsafe shell command construction with direct exec.Command calls in wacom module

refactor(wacom): 用直接 exec 调用替换 shell 命令拼接

1. 将 `doAction(string)` 重构为 `doAction(args ...string)` 以消除 shell 注入风险
2. 重构所有 Wacom set 方法,改为逐个传参而非使用 fmt.Sprintf 拼接 shell 命令
3. 移除 `exec.Command("/bin/sh", "-c", cmd)` 改用 `exec.Command(cmdXSetWacom, args...)` 执行命令
4. 调整 import 排序,添加 strconv/errors 用于整数/字符串转换

Log: 将 wacom 模块中不安全的 shell 命令拼接替换为直接 exec.Command 调用
pms: TASK-389293
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这份代码变更主要是对 Wacom 设备控制命令执行方式的重大重构,从使用 fmt.Sprintf 构造 Shell 命令字符串转变为使用 exec.Command 的变长参数形式。这是一个非常好的改进,以下是详细的审查意见:

1. 语法逻辑

  • 变更分析
    • 代码将原本的字符串拼接(fmt.Sprintf)改为直接传递参数。
    • doAction 函数签名从 func doAction(cmd string) 变更为 func doAction(args ...string)
    • 调用处将整数类型显式转换为字符串(strconv.Itoastrconv.FormatUint)。
    • 移除了 // #nosec G204 注释,这意味着之前的代码存在静态安全扫描警告,而新代码解决了这个问题。
  • 逻辑正确性:逻辑转换是正确的。exec.Command 的第一个参数是命令名,后续是参数列表。代码中 cmdXSetWacom(推测是 "xsetwacom")作为命令,"set" 和后续参数作为参数传入,完全符合 CLI 程序的调用规范。

2. 代码质量

  • 改进点
    • 消除字符串拼接:旧代码使用 fmt.Sprintf 构造命令行,这种方式难以阅读且容易出错(例如参数中包含空格时)。新代码参数列表清晰,可读性大大提高。
    • 移除 Magic String:虽然 cmdXSetWacom 在旧代码中也存在,但旧代码将其埋在字符串拼接中。新代码将其明确作为 exec.Command 的首参数,结构更清晰。
    • 代码一致性:所有的 Set 函数(如 SetArea, SetRotate 等)现在都统一调用 doAction 并传递参数,代码风格更加统一。
  • 潜在问题
    • 类型转换冗余:在 SetArea, SetPressureCurve 等函数中,使用了 strconv.Itoa。虽然这是必须的(因为 exec.Command 接受 []string),但代码行数变多了。这是为了安全性付出的代价,可以接受。

3. 代码性能

  • 性能提升
    • 减少内存分配fmt.Sprintf 涉及格式化解析和内存分配来生成新的字符串对象。新代码直接传递已有的字符串或转换后的字符串,避免了复杂的格式化开销和中间字符串的创建。
    • 免去 Shell 启动开销:旧代码调用 /bin/sh -c "cmd",这需要额外启动一个 Shell 进程,再由 Shell 去解析并执行 xsetwacom。新代码直接执行 xsetwacom 可执行文件,减少了一个进程的创建和销毁,以及 Shell 的解析开销,性能会有显著提升。

4. 代码安全

  • 重大安全增强(最重要的改进)
    • 防止命令注入:这是本次修改最大的亮点。
      • 旧代码:使用 exec.Command("/bin/sh", "-c", cmd)。如果 w.Idoutput 等参数包含恶意构造的字符(例如 ; rm -rf /),Shell 会将其解析为命令分隔符,从而执行任意命令。
      • 新代码:使用 exec.Command(cmdXSetWacom, args...)。这种调用方式不会通过 Shell 解析参数。操作系统直接将参数传递给目标程序。即使参数中包含 ;$()、反引号等特殊字符,它们也会被当作普通的字符串参数传递给 xsetwacom,而不会被当作命令执行。这彻底消除了命令注入的风险。
    • 移除 #nosec:这表明开发者已经意识到并修复了 G204(CWE-78: OS Command Injection)这一安全漏洞。

总结与建议

这是一个非常优秀的重构,主要提升了代码的安全性性能,同时也略微提高了可读性

建议

  1. 保持现状:目前的实现方式是 Go 语言执行系统命令的最佳实践。
  2. 常量检查:请确保 cmdXSetWacom 常量在文件其他地方已正确定义(例如 const cmdXSetWacom = "xsetwacom"),从 diff 来看应该是存在的。
  3. 错误处理doAction 中的 errors.New(string(out)) 是一个简单实用的错误处理方式。如果 out(标准输出/错误输出)可能包含大量文本,考虑截断或清理,但在配置工具场景下通常没问题。

结论:该变更完全符合安全编码规范,建议合并。

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.

2 participants