Skip to content

fix: auto-disable XDP for non-conforming apps#1662

Closed
reddevillg wants to merge 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:invalid_xdp_id
Closed

fix: auto-disable XDP for non-conforming apps#1662
reddevillg wants to merge 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:invalid_xdp_id

Conversation

@reddevillg

Copy link
Copy Markdown
Collaborator

Disable XDP integration when the appid does not conform xdp id

@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: reddevillg

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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a validation mechanism for XDG Desktop Portal IDs (app IDs) within the CLI. It adds a new utility function isValidXdgDesktopPortalId in libs/utils/src/linglong/utils/xdp.h and integrates it into the runResolvedContext flow to automatically disable XDP integration if the ID is invalid. Comprehensive unit tests for the validation logic were also added. The review feedback suggests replacing std::isalnum with explicit ASCII range checks to avoid locale-dependent behavior and potential undefined behavior when handling signed characters.

Comment thread libs/utils/src/linglong/utils/xdp.h
Comment thread libs/utils/src/linglong/utils/xdp.h
@codecov

codecov Bot commented May 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.61538% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libs/linglong/src/linglong/cli/cli.cpp 0.00% 4 Missing ⚠️
Files with missing lines Coverage Δ
libs/utils/src/linglong/utils/xdp.h 100.00% <100.00%> (ø)
libs/linglong/src/linglong/cli/cli.cpp 1.68% <0.00%> (-0.01%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Disable XDP integration when the appid does not conform xdp id

Signed-off-by: reddevillg <reddevillg@gmail.com>
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

deepin pr auto review

这是一份针对您提供的 Git Diff 的代码审查报告。本次代码变更的主要目的是在运行容器前校验应用 ID(appid)是否符合 XDG Desktop Portal (XDP) 规范,如果不符合则发出警告并自动禁用 XDP 集成,同时提供了相应的工具函数和单元测试。

整体来看,代码逻辑清晰,测试覆盖较为全面,但存在一些性能隐患、潜在的逻辑漏洞以及代码细节可以优化。以下是详细的审查意见:

1. 代码性能

问题:不必要的堆内存分配
xdp.hisValidXdgDesktopPortalId 函数中,使用了 std::vector<std::string_view> segments; 来存储按 . 分割的子串。由于 string_view 本身不拥有内存,拷贝开销极小,但 std::vector 本身的动态内存分配在频繁调用时会产生不必要的性能开销。

改进意见:
由于逻辑只需要区分“最后一个分段”和“非最后一个分段”,完全可以通过单次遍历在原字符串上直接校验,避免使用 vector 和分割操作,从而实现零堆内存分配。

inline bool isValidXdgDesktopPortalId(std::string_view appid) noexcept
{
    if (appid.empty()) {
        return false;
    }

    size_t segment_count = 1;
    for (char c : appid) {
        if (c == '.') {
            ++segment_count;
        }
    }

    if (segment_count < 2) {
        return false;
    }

    size_t start = 0;
    size_t segment_index = 0;

    for (size_t i = 0; i <= appid.size(); ++i) {
        // 遇到分隔符或到达末尾时处理分段
        if (i == appid.size() || appid[i] == '.') {
            size_t len = i - start;
            if (len == 0) {
                return false; // 空分段 (如 "org..app" 或末尾的 ".")
            }

            std::string_view segment = appid.substr(start, len);
            bool is_last = (segment_index == segment_count - 1);

            bool valid = std::all_of(segment.begin(), segment.end(), [is_last](char c) {
                if (std::isalnum(static_cast<unsigned char>(c)) || c == '_') {
                    return true;
                }
                // 只有最后一个分段允许连字符 '-'
                return is_last && c == '-';
            });

            if (!valid) {
                return false;
            }

            start = i + 1;
            ++segment_index;
        }
    }

    return true;
}

2. 语法与逻辑

问题:XDP 规范对分段首字符的限制被忽略
根据 Flatpak/XDG Desktop Portal 的 App ID 规范,不仅字符类型有限制,分段的首字符必须是字母(即 [A-Za-z]),数字和下划线不能作为分段的开头。
当前代码的测试用例 org.2example.appa.b.c.d.e 中的 2examplea, b, c, d 如果作为非末尾分段,首字符如果是数字,在严格的 D-Bus 命名规范中是无效的。虽然 XDP 的某些实现可能比较宽松,但为了严谨性,建议遵循 D-Bus 的命名规范。

改进意见:
在遍历分段时,增加对首字符是否为字母的校验。

// 在校验 segment 的 lambda 或循环中添加:
if (!std::isalpha(static_cast<unsigned char>(segment[0]))) {
    return false; // 分段必须以字母开头
}

(注意:如果修改此逻辑,请同步更新单元测试,将 org.2example.app 标记为 EXPECT_FALSE,并确保单字母分段如 a.b 仍然通过)

3. 代码质量

问题:std::isalnum 的未定义行为风险
当前代码已经很好地使用了 static_cast<unsigned char>(c) 来调用 std::isalnum,这是非常规范的写法,可以避免窄化转换导致的未定义行为。但是,由于 xdp.h 是一个被多处引用的头文件,且 isValidXdgDesktopPortalId 被声明为 inline 函数,标准库的 <cctype><algorithm> 增加了头文件的编译依赖负担。

改进意见:
如果对性能有极致要求,或者希望减少头文件依赖,可以将 std::isalnum 替换为简单的手写字符判断逻辑,因为 ASCII 范围的判断只需要几行代码:

inline bool isAlnum(char c) {
    unsigned char uc = static_cast<unsigned char>(c);
    return (uc >= '0' && uc <= '9') || (uc >= 'A' && uc <= 'Z') || (uc >= 'a' && uc <= 'z');
}

这样可以移除对 <cctype><algorithm> 的依赖。

4. 代码安全与业务逻辑

问题:--enable-xdp 参数的交互逻辑可能存在误解
cli.cpp 中的逻辑:

if (!options.disableXdp.has_value() && !utils::isValidXdgDesktopPortalId(appid)) {
    LogW("appid '{}' doesn't conform to XDP ID specification, disabling XDP integration. "
         "Use --enable-xdp to override.",
         appid);
    runOptions.disableXdp = true;
}

这里判断 !options.disableXdp.has_value(),意味着如果用户没有显式设置该参数,且 ID 不合法,则自动禁用,并提示用户用 --enable-xdp 覆盖。
但是,如果用户真的传入了 --enable-xdp,代码是如何将 options.disableXdp 置为 has_value() == true 的?
通常 CLI 参数解析时:

  • 如果 --enable-xdp 被传入,应该意味着 disableXdp = falsehas_value() == true
  • 如果 --disable-xdp 被传入,应该意味着 disableXdp = truehas_value() == true

改进意见:
请确保 CLI 参数解析层(如 CLI11 或 argparse 逻辑)在用户传入 --enable-xdp 时,正确地将 options.disableXdp 设置为 false(使其具有值)。如果解析层只是因为没有传参而保持 std::optional 为空,那么用户即使按照提示使用了 --enable-xdp,也无法进入 if 分支,导致无法覆盖成功。

建议在代码审查时确认 CLI 参数绑定代码,确保以下逻辑映射:

  • 未传参:disableXdp = std::nullopt
  • --enable-xdpdisableXdp = false
  • --disable-xdpdisableXdp = true

5. 测试用例

问题:版权年份与边界测试

  1. xdp_test.cppxdp.h 中的版权年份写的是 2026,这很可能是笔误,建议修改为当前年份(如 20232024)。
  2. 测试用例中缺少对超长字符串极端边界的测试。虽然 XDP ID 通常不会特别长,但作为工具函数,测试一个极长的 ID(如 4096 个字符)可以验证函数是否存在性能退化。

总结

代码的核心逻辑和意图是好的,通过前置校验避免了运行时的隐式错误。建议优先处理性能上的 vector 堆分配问题(改为原地遍历),并确认XDP ID 分段首字符的规范限制以及**--enable-xdp 参数解析的映射逻辑**是否正确。修改后,这将是一份非常健壮的代码。

@reddevillg reddevillg closed this May 15, 2026
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