Refactor arguments escaping

[ComixHe]

No description provided.

[sourcery-ai]

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

Please try again later or upgrade to continue using Sourcery

[deepin-ci-robot]

deepin pr auto review

Git Diff 代码审查报告

总体评价

这次提交主要涉及应用启动器(app-launch-helper)和应用管理器(dde-application-manager)的代码重构，重点改进了参数解析、命令行转义、DBus通信和代码结构。整体上代码质量有所提升，使用了更现代的C++特性，但仍有部分细节可以进一步优化。

详细审查

1. 语法逻辑

1.1 encodeArgument 函数 (main.cpp)

void encodeArgument(std::string &arg)
{
    if (arg == ";") {
        arg = R"(\;)";
        return;
    }
    // ... 其余代码
}

问题：函数只处理了;和$的转义，但根据systemd规范，还需要处理其他特殊字符。

建议：

1. 添加对其他特殊字符的处理，如反引号()、双引号("`)等
2. 考虑添加单元测试覆盖各种转义情况

1.2 cmdParse 函数返回值变更

-std::string cmdParse(msg_ptr &msg, std::deque<std::string_view> cmdLines)
+std::optional<std::string> cmdParse(msg_ptr &msg, const std::vector<std::string_view> &cmdLines)

优点：使用std::optional更清晰地表示可能失败的操作。

建议：考虑定义一个Result类型或使用std::expected(C++23)来携带错误信息，而不仅仅是成功/失败状态。

1.3 参数解析逻辑

for (const auto *it = str.begin(); it != str.end(); ++it) {
    const auto c = *it;
    // ...
    if (c == u'\\') {
        if ((it + 1) == str.end()) {
            qWarning() << "Exec parsing error: Backslash at end of line";
            return std::nullopt;
        }
        // ...
    }
}

问题：参数解析逻辑复杂，容易出错。

建议：

1. 考虑使用状态机模式重写参数解析逻辑
2. 添加更详细的错误信息，指出具体位置
3. 考虑使用第三方库如shlex(Python)的C++实现

2. 代码质量

2.1 getPropType 函数改进

-    static std::unordered_map<std::string_view, DBusValueType> map{...};
+    // small data just use array and linear search
+    static constexpr std::array<Entry, 4> map{...};

优点：对于小数据集，使用数组+线性搜索比哈希表更高效，且代码更简洁。

建议：保持当前实现，但添加注释说明为什么选择数组而非哈希表。

2.2 processExecStart 函数参数变更

-int processExecStart(msg_ptr &msg, const std::deque<std::string_view> &execArgs)
+int processExecStart(msg_ptr msg,
+                     std::vector<std::string_view>::const_iterator begin,
+                     std::vector<std::string_view>::const_iterator end)

优点：使用迭代器范围更灵活，且避免了不必要的容器拷贝。

建议：考虑使用std::span(C++20)代替迭代器对，更清晰地表示连续内存范围。

2.3 日志记录

+        sd_journal_print(LOG_INFO, "after encode: %s", buffer.c_str());

问题：生产环境中频繁记录详细日志可能影响性能。

建议：

1. 使用条件编译或日志级别控制此日志输出
2. 考虑添加日志上下文信息，如函数名、行号等

3. 代码性能

3.1 字符串处理

std::string buffer{*begin};
encodeArgument(buffer);

问题：每次调用encodeArgument都会创建临时字符串，可能影响性能。

建议：

1. 考虑实现原地编码版本
2. 对于大量参数，考虑预分配足够大的缓冲区

3.2 路径处理

const std::filesystem::path path{value};
if (!path.is_absolute()) {
    sd_journal_print(LOG_WARNING, "ExecSearchPath ignoring relative path: %s", value.data());
    continue;
}
props[std::move(key)].emplace_back(path.lexically_normal());

优点：使用std::filesystem进行路径规范化。

建议：

1. 考虑缓存规范化结果，避免重复计算
2. 对于已知安全的路径，可以跳过规范化

3.3 容器使用

-    std::deque<std::string_view> args;
+    std::vector<std::string_view> args;
+    args.reserve(argc);

优点：使用vector并预分配空间更高效。

建议：对于小量参数，当前实现足够好；对于大量参数，考虑使用std::string_view避免字符串拷贝。

4. 代码安全

4.1 输入验证

if (str.size() < 3 || str.compare(0, 2, "--") != 0) {
    sd_journal_print(LOG_WARNING, "Unknown option: %s", str.data());
    return std::nullopt;
}

优点：增加了输入验证。

建议：

1. 添加对特殊字符的验证，如空字符、控制字符等
2. 考虑添加长度限制，防止超长输入导致缓冲区溢出

4.2 命令注入防护

void encodeArgument(std::string &arg)
{
    // ...
    auto extra = std::count(arg.cbegin(), arg.cend(), '$');
    // ...
}

优点：对特殊字符进行转义，防止命令注入。

建议：

1. 添加更多安全检查，如检测命令链(;, &&, ||)等
2. 考虑使用白名单机制，只允许特定字符集

4.3 错误处理

if (ret = sd_bus_message_append(msg, "s", buffer.c_str()); ret < 0) {
    sd_journal_perror("append binary of execStart failed.");
    return ret;
}

优点：检查所有系统调用的返回值。

建议：

1. 考虑添加错误上下文信息，如参数值、函数名等
2. 对于关键错误，考虑添加更详细的日志或告警

其他建议

1. 测试覆盖：新增的ut_escape.cpp测试很好，但建议增加更多边界条件和异常情况测试。

2. 文档：为复杂的函数(如cmdParse, processExec)添加详细注释，说明其行为、参数和返回值。

3. 代码复用：unescapeValue和splitExecArguments有相似功能，考虑合并或提取公共部分。

4. 性能分析：建议对关键路径进行性能分析，识别热点并优化。

5. 代码风格：保持一致的命名约定和代码风格，如使用snake_case或camelCase。

总结

这次提交在代码质量和安全性方面有明显改进，使用了更现代的C++特性，并增强了错误处理和输入验证。主要改进点包括：

1. 使用std::optional更清晰地表示可能失败的操作
2. 改进参数解析和转义逻辑
3. 优化容器使用和内存管理
4. 增强输入验证和安全检查

建议在后续版本中：

1. 添加更全面的测试覆盖
2. 考虑使用更现代的C++特性(如std::span, std::expected)
3. 改进错误处理和日志记录
4. 进行性能分析和优化

整体而言，这是一次高质量的代码提交，为项目的稳定性和可维护性打下了良好基础。

[deepin-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BLumia, ComixHe

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• debian/deepin/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment