Skip to content

fix: fix config reload abort on file update failure and uppercase path matching#148

Merged
18202781743 merged 1 commit into
linuxdeepin:masterfrom
18202781743:master
May 11, 2026
Merged

fix: fix config reload abort on file update failure and uppercase path matching#148
18202781743 merged 1 commit into
linuxdeepin:masterfrom
18202781743:master

Conversation

@18202781743
Copy link
Copy Markdown
Contributor

  1. Change reload() to continue processing remaining files when one
    update fails, instead of aborting early
  2. Add updateInternal() method that returns error string without sending
    DBus error, allowing reload to handle failures gracefully
  3. Fix regular expression patterns to support uppercase letters in
    configuration paths, as file systems may use mixed case
  4. Track failure count during reload to report accurate processing
    results

Log: Fixed reload aborting on file update failure and added uppercase
path support

Influence:

  1. Test reload with multiple files where one update fails, verify
    remaining files still process
  2. Test configuration paths with uppercase letters to ensure proper
    matching
  3. Verify DBus error response still works correctly for single file
    updates
  4. Check reload completes without hanging on consecutive failures

fix: 修复配置重载在文件更新失败时提前终止以及大写路径匹配错误的问题

  1. 修改 reload() 函数,当某个文件更新失败时继续处理剩余文件,而不是提前
    终止
  2. 添加 updateInternal() 方法,返回错误字符串而不发送 DBus 错误,使
    reload 能优雅处理失败
  3. 修复正则表达式模式,支持配置文件路径中的大写字母,因为文件系统可能使
    用混合大小写
  4. 在重载过程中跟踪失败计数,以报告准确的处理结果

Log: 修复重载在文件更新失败时提前终止的问题,并增加大写路径支持

Influence:

  1. 测试多文件重载,其中一个文件更新失败,验证其他文件仍能正常处理
  2. 测试包含大写字母的配置文件路径,确保正确匹配
  3. 验证单个文件更新时 DBus 错误响应仍正常工作
  4. 检查连续失败时重载不会卡住

PMS: BUG-360197

@18202781743 18202781743 requested review from BLumia and mhduiy May 11, 2026 06:09
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 @18202781743, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@18202781743 18202781743 force-pushed the master branch 2 times, most recently from 38ed8bd to 98dfcda Compare May 11, 2026 06:25
failure

1. Updated regex patterns in dconfig_global.h to accept uppercase
letters (A-Z) in config file paths, fixing parsing failures for files
with uppercase names
2. Refactored update() by extracting updateInternal() that returns error
messages instead of early-aborting via sendErrorReply on DBus
3. Modified reload() to continue processing remaining files when
individual updates fail, instead of aborting the entire reload
4. Enhanced reload() to collect and report total failed file count for
better debugging

Log: Fixed config file matching for uppercase names and improved reload
robustness

Influence:
1. Verify config files with uppercase names (e.g., MyConfig.json) are
correctly parsed
2. Test scenario: multiple files to update, one file update fails,
verify reload continues processing remaining files
3. Verify error count is correctly reported in logs when partial reload
failures occur
4. Test DBus update calls still report errors correctly via
sendErrorReply
5. Verify no regression for existing lowercase-path config files
6. Test with mixed case paths in config directories

fix: 支持大写文件名称并防止重新加载时因部分失败而终止

1. 修改了 dconfig_global.h 中的正则表达式,增加了对大写字母 (A-Z) 的支
持,修复了含有大写字母的配置文件路径解析失败的问题
2. 重构了 update() 方法,提取出 updateInternal() 返回错误信息,而不是通
过 sendErrorReply 提前终止 DBus 调用
3. 修改 reload() 方法,当个别文件更新失败时继续处理剩余文件,而不是整个
重载过程提前终止
4. 增强 reload() 日志,收集并报告失败的文件数量,方便调试

Log: 修复了配置文件名大写匹配问题,增强了重载的鲁棒性

Influence:
1. 验证包含大写的配置文件(如 MyConfig.json)能够正确解析
2. 测试多文件更新场景:其中一个文件更新失败,验证重载继续处理剩余文件
3. 验证部分重载失败时日志中正确报告错误数量
4. 测试 DBus 更新调用仍通过 sendErrorReply 正确报告错误
5. 验证现有小写路径配置文件无回归问题
6. 测试配置目录中混合大小写路径的情况

PMS: BUG-360197
Change-Id: I054dd047b2924d6475b357ec1a83643048e7e2ea
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff。本次修改主要涉及三个方面:正则表达式的优化与重构业务逻辑与DBus通信的解耦(提取updateInternal、以及**reload流程的日志与错误统计增强**。

整体来看,这次修改的方向非常好,提高了代码的内聚性,并修复了正则表达式的一些潜在问题。以下是针对语法逻辑、代码质量、代码性能和代码安全的详细审查意见及改进建议:

一、 语法与逻辑

1. 正则表达式:原子组与捕获组的混用问题

  • 问题:在新增的正则中,你使用了原子组 (?>...) 来防止回溯,但同时又在其中使用了命名捕获组 (?<name>...)。例如:(?<appid>(?>[a-zA-Z0-9\s_@^!#$%&.\-]+)\/)?
  • 分析:在PCRE和Qt的正则引擎中,(?> 会开启一个独立的回溯栈,当原子组匹配完毕后,内部发生的任何回溯都会被丢弃。虽然将命名捕获组包在原子组内语法上合法且能正常提取文本,但这种写法逻辑上略显混乱,且在某些边缘场景下可能引发非预期的匹配截断。
  • 建议:如果只是为了防止回溯提升性能,建议将原子组放在捕获组内部,或者直接使用占有量词(Possessive Quantifiers,如 ++, *+),这样既防回溯,又保持捕获组语义清晰:
    // 修改前:
    (?<appid>(?>[a-zA-Z0-9\s_@^!#$%&.\-]+)\/)?
    // 修改后(使用占有量词,更简洁且性能相同):
    (?<appid>[a-zA-Z0-9\s_@^!#$%&.\-]++\/)?

2. 正则表达式:转义字符的冗余与潜在风险

  • 问题:字符类 [] 中的 \- 写法。在 [] 中,如果 - 放在开头或结尾,不需要转义;如果放在中间,必须转义 \-。但你之前的代码中出现了 [a-z0-9\s\-_\@\-\^!#$%&.],这里存在连续的 \-_\@\-\^,其中 \- 是合法的转义,但显得有些凌乱,容易引起误解。
  • 建议:统一规范,将 - 放在字符类的最后,这样既不需要转义,又具有最好的可读性:
    // 修改前:
    [a-zA-Z0-9\s_@^!#$%&.\-]
    // 修改后:
    [a-zA-Z0-9\s_@^!#$%&.+-]  // 如果需要匹配加号也加上,把减号放最后

3. 正则表达式:大小写扩展的覆盖范围

  • 问题:你将 [a-z] 扩展为了 [a-zA-Z],这是正确的。但Qt的QRegularExpression支持内联修饰符,如果整个正则都需要忽略大小写,可以直接在开头加上 (?i),这样能显著缩短正则表达式的长度,降低维护难度。
  • 建议
    static QRegularExpression usrReg(
        R"((?i)/configs/(?!overrides/)(?<appid>[a-z0-9\s_@^!#$%&.]++\/)?(?<subpath>(?:[a-z0-9\s_@^!#$%&.]++\/)*)(?<resource>[a-z0-9\s_@^!#$%&.]++)\.json$)"
    );

二、 代码质量

1. 解耦与职责分离(优秀)

  • 提取 updateInternal 并返回 std::optional<QString> 是一个非常棒的设计!它完美地将核心业务逻辑DBus通信副作用分离开来,使得 reload() 这种非DBus调用的场景可以复用核心逻辑,而不必处理DBus的 sendErrorReply。这极大提升了代码的可测试性和可维护性。

2. reload 方法的日志优化(优秀)

  • 增加 failedCount 统计,以及对 changedFiles.isEmpty() 的提前返回,都是很好的防御性编程和日志体验优化。

三、 代码性能

1. 正则表达式回溯控制(优秀)

  • 引入原子组 (?>...) 或占有量词 ++ 来处理文件路径这种可能很长的字符串,是非常专业的做法。它可以有效防止恶意或错误构造的超长路径导致的ReDoS(正则表达式拒绝服务)攻击,提升了系统的健壮性。

2. std::optionalQString 的值传递

  • 问题updateInternal 返回 std::optional<QString>,而在 update 中通过 *errorMsg 取出并传递给 qWarning()sendErrorReply()
  • 分析QString 内部使用了隐式共享(Copy-on-Write),所以拷贝开销极低。但如果你希望追求极致性能,可以避免拷贝:
  • 建议:由于 updateInternal 返回的是新构造的临时对象,编译器通常会应用RVO(返回值优化)或移动语义。在调用侧,可以使用 const 引用来避免潜在的深拷贝:
    void DSGConfigServer::update(const QString &path)
    {
        const auto errorMsg = updateInternal(path);
        if (errorMsg) {
            // 使用 const& 避免解引用时的拷贝
            const QString& errMsg = *errorMsg; 
            qWarning() << errMsg;
            if (calledFromDBus()) {
                sendErrorReply(QDBusError::Failed, errMsg);
            }
        }
    }

四、 代码安全

1. 路径遍历攻击防范

  • 问题:正则表达式目前允许匹配包含空格 \s 和多种特殊符号的路径,但没有显式限制 .. 目录跳转。
  • 分析:如果传入的 path/usr/share/dsg/configs/appid/../secret.json,当前的正则表达式可能会将 appid/.. 中的 .. 匹配为合法的文件名字符,从而解析到预期之外的配置文件。
  • 建议:在 isConfigurePathupdateInternal 的入口处,建议使用 QFileInfo(path).canonicalFilePath() 获取规范化路径,确保路径中不包含 .. 跳转,再进行正则匹配。或者在正则中显式排除 . 连缀(但这会让正则更复杂,推荐代码层面过滤)。

2. ReDoS 防范(已解决)

  • 正如性能部分提到的,原正则 ([a-z0-9...]+\/)* 具有典型的嵌套量词特征,极易受到ReDoS攻击。本次修改通过原子组/占有量词彻底消除了这一安全隐患,值得肯定。

综合修改建议代码示例

结合以上审查意见,我为你优化了 dconfig_global.h 中的正则表达式代码(采用 (?i) 忽略大小写 + 占有量词 ++ + 字符类重排):

// dconfig_global.h

inline ConfigureId getMetaConfigureId(const QString &path)
{
    ConfigureId info;
    // /usr/share/dsg/configs/[$appid]/[$subpath]/$resource.json
    // 使用 (?i) 忽略大小写,使用占有量词 ++ 防止回溯,将 - 放在字符类末尾避免混乱转义
    static QRegularExpression usrReg(
        R"((?i)/configs/(?!overrides/)(?<appid>[a-z0-9\s_@^!#$%&.+-]++\/)?(?<subpath>(?:[a-z0-9\s_@^!#$%&.+-]++\/)*)(?<resource>[a-z0-9\s_@^!#$%&.+-]++)\.json$)"
    );

    QRegularExpressionMatch match;
    match = usrReg.match(path);
    // ... 后续逻辑保持不变
}

inline ConfigureId getOverrideConfigureId(const QString &path)
{
    ConfigureId info;
    // /usr/share/dsg/configs/overrides/[$appid]/$resource/[$subpath]/$override_id.json
    static QRegularExpression usrReg(
        R"((?i)/configs/overrides/(?<appid>[a-z0-9\s_@^!#$%&.+-]++\/)?(?<resource>[a-z0-9\s_@^!#$%&.+-]++)/(?<subpath>(?:[a-z0-9\s_@^!#$%&.+-]++\/)*)(?<configurationid>[a-z0-9\s_@^!#$%&.+-]++)\.json$)"
    );

    // /etc/dsg/configs/overrides/[$appid]/$resource/[$subpath]/$override_id.json
    static QRegularExpression etcReg(
        R"((?i)^/etc/dsg/configs/overrides/(?<appid>[a-z0-9\s_@^!#$%&.+-]++\/)?(?<resource>[a-z0-9\s_@^!#$%&.+-]++)/(?<subpath>(?:[a-z0-9\s_@^!#$%&.+-]++\/)*)(?<configurationid>[a-z0-9\s_@^!#$%&.+-]++)\.json$)"
    );

    QRegularExpressionMatch match;
    match = usrReg.match(path);
    // ... 后续逻辑保持不变
}
// dconfigserver.cpp

void DSGConfigServer::update(const QString &path)
{
    const auto errorMsg = updateInternal(path);
    if (errorMsg) {
        const QString& errMsg = *errorMsg; // 使用常量引用避免多余的QString深拷贝
        qWarning() << errMsg;
        if (calledFromDBus()) {
            sendErrorReply(QDBusError::Failed, errMsg);
        }
    }
}

总结:本次Diff整体质量很高,逻辑解耦十分合理,性能与安全性均有提升。只需稍微打磨一下正则表达式的书写规范,并注意防范路径跳转漏洞,代码就会非常健壮。希望这些意见对你有所帮助!

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@18202781743 18202781743 merged commit bef0d22 into linuxdeepin:master May 11, 2026
21 of 22 checks passed
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