Skip to content

fix: Improve command execution for file operations#117

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
dengzhongyuan365-dev:master
Feb 5, 2026
Merged

fix: Improve command execution for file operations#117
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
dengzhongyuan365-dev:master

Conversation

@dengzhongyuan365-dev
Copy link
Contributor

  • Updated sendUrlToDownloader to use QStringList for better argument handling in QProcess.
  • Replaced system calls with QFile methods for copying and removing files in MainFrame, enhancing safety and portability.

Log: Refactor command execution to utilize Qt's file handling capabilities.

- Updated `sendUrlToDownloader` to use `QStringList` for better argument handling in `QProcess`.
- Replaced system calls with `QFile` methods for copying and removing files in `MainFrame`, enhancing safety and portability.

Log: Refactor command execution to utilize Qt's file handling capabilities.
Copy link

@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 @dengzhongyuan365-dev, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

lzwind
lzwind previously approved these changes Feb 2, 2026
@github-actions
Copy link

github-actions bot commented Feb 5, 2026

TAG Bot

TAG: 6.0.18
EXISTED: no
DISTRIBUTION: unstable

@dengzhongyuan365-dev
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Feb 5, 2026

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit 4d09b95 into linuxdeepin:master Feb 5, 2026
29 of 31 checks passed
@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev

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 similar comment
@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev

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

@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

这段代码的 diff 主要展示了从使用 C 风格的 system() 调用和字符串拼接命令,转向使用 Qt 框架提供的跨平台类(如 QProcessQFileQDir)来处理文件操作和进程启动。这是一个非常好的重构方向。

以下是对这两处修改的详细审查意见,包括语法逻辑、代码质量、代码性能和代码安全方面的分析:

1. extensionservice.cpp 修改审查

修改内容:
proc.startDetached("downloader " + url); 修改为 proc.startDetached("downloader", QStringList() << url);

  • 语法逻辑:正确。QProcess::startDetached 接受程序名和参数列表作为分开的参数,这是推荐用法。
  • 代码质量显著提升
    • 安全性:原代码通过字符串拼接构造命令,如果 url 中包含空格或特殊字符(如 ;, |, &),可能会导致命令注入攻击或命令解析错误。修改后的代码将 URL 作为参数传递,由系统处理转义,避免了命令注入风险。
    • 可读性:参数列表的形式更清晰地表达了"程序名"和"参数"的区别。
  • 代码性能:影响微乎其微,但避免了不必要的字符串拼接操作。

2. mainframe.cpp 修改审查

修改内容:
onPowerOnChangedsetAutoStart 函数中,移除了 system("cp ...")system("rm ...") 的调用,改为使用 QDir, QFile 等 Qt 类进行文件操作。

  • 语法逻辑:正确。
    • QDir().mkpath(userDefaultDesktopPath) 确保目标目录存在,这是原代码隐含的逻辑(cp 命令通常要求目标目录存在),显式写出更健壮。
    • QFile::remove(dstPath)QFile::copy 之前调用,这是因为 QFile::copy 在目标文件存在时会失败,这是必要的逻辑补充。
  • 代码质量显著提升
    • 安全性:原代码使用 system() 执行 shell 命令,这是极其危险的。如果文件路径变量(如 defaultDesktopuserDefaultDesktopPath)包含特殊字符(例如文件名为 foo; rm -rf /),将导致严重的安全漏洞。使用 QFile 直接操作文件系统 API,完全规避了 Shell 解释器的介入,消除了命令注入风险。
    • 跨平台性system("cp ...") 是 Linux/Unix 专用的。改为 QFile::copy 后,代码具备了跨 Windows 平台的能力(尽管路径分隔符可能仍需注意,但逻辑是通用的)。
    • 错误处理:虽然 QFile 的静态函数默认不报错(只返回 bool),但相比 system() 那种"盲执行"的方式,Qt API 提供了更好的错误检查基础(例如可以检查返回值)。
  • 代码性能
    • system() 调用会启动一个新的 Shell 进程(/bin/sh),然后再启动 cp/rm 进程,涉及两次进程创建和上下文切换,开销较大。
    • QFile 直接调用系统内核 API,不需要启动子进程,性能开销更小,响应更快。

综合改进建议

虽然代码修改方向非常正确,但在 mainframe.cpponPowerOnChanged 函数中,还可以进一步优化:

  1. 增加错误检查
    文件操作(如复制、删除)可能会失败(权限不足、磁盘已满、源文件不存在等)。建议检查 QFile::copyQFile::remove 的返回值,并在失败时输出 qWarning()

    // 示例:在 onPowerOnChanged 中增加错误检查
    if (!QFile::exists(srcPath)) {
        qWarning() << "Source file does not exist:" << srcPath;
        return;
    }
    if (QFile::exists(dstPath)) {
        if (!QFile::remove(dstPath)) {
            qWarning() << "Failed to remove existing file:" << dstPath;
        }
    }
    if (!QFile::copy(srcPath, dstPath)) {
        qWarning() << "Failed to copy file from" << srcPath << "to" << dstPath;
    }
  2. 资源管理(针对 extensionservice.cpp)
    extensionservice.cpp 中,QTimer 使用了 new 但没有看到对应的 delete。虽然 connect 使用了 lambda 捕获了 timer,但 lambda 内部似乎没有 timer->deleteLater()timer->stop()

    • 问题:如果这个定时器是一次性的,应该设置 timer->setSingleShot(true)
    • 内存泄漏:如果定时器不是单发的,或者没有在槽函数中销毁它,会导致内存泄漏。
    • 建议:确保定时器在触发后能够自我销毁,或者使用成员变量管理定时器的生命周期。
    // 建议修改 extensionservice.cpp 中的定时器逻辑
    QTimer *timer = new QTimer(this); // 指定 parent,便于对象树自动管理
    timer->setSingleShot(true);      // 设置为单次触发
    connect(timer, &QTimer::timeout, this, [=](){
        // ... 逻辑代码 ...
        timer->deleteLater();        // 或者依赖 this 的销毁
    });
    timer->start(50);

总结

这次修改是一次高质量的重构,主要解决了代码的安全隐患(命令注入)和平台兼容性问题,同时略微提升了性能。建议在后续代码中继续保持这种风格,并补充文件操作的错误处理逻辑。

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.

3 participants