Skip to content

fix: correct xdp document fuse mount option#1656

Merged
ComixHe merged 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:fix_xdp_fuse
May 12, 2026
Merged

fix: correct xdp document fuse mount option#1656
ComixHe merged 1 commit into
OpenAtom-Linyaps:masterfrom
reddevillg:fix_xdp_fuse

Conversation

@reddevillg

Copy link
Copy Markdown
Collaborator

No description provided.

@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 updates the mount options for the XDG runtime documentation directory in container_cfg_builder.cpp to include nosuid, nodev, and relatime. A critical typo was identified in the code where "realtime" was used instead of the standard "relatime" option, which could cause the mount operation to fail and prevent the container from starting.

Comment thread libs/oci-cfg-generators/src/linglong/oci-cfg-generators/container_cfg_builder.cpp Outdated
Signed-off-by: reddevillg <reddevillg@gmail.com>
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

deepin pr auto review

你好!我是CodeGeeX。我已仔细审查了你提供的Git Diff内容。本次修改主要是为容器挂载配置中的文档目录(doc)增加了 nosuidnodevrelatime 三个挂载选项。

整体来看,这次修改方向是非常好的,显著提升了容器的安全性和挂载性能。以下是我从语法逻辑、代码质量、代码性能和代码安全四个维度的详细审查意见:

1. 语法逻辑

  • 评价:良好。C++的指派初始化器(Designated initializers,如 .destination = ...)使用正确,格式化调整后代码对齐良好,可读性佳。
  • 潜在逻辑问题xdpOption->docMountPoint / "by-app" / appId 这里的 appId 如果来源于外部输入且未经过校验,可能存在路径遍历风险(详见安全部分)。另外,需确认 appId 不为空,否则源路径会以 / 结尾,可能导致非预期的挂载行为。

2. 代码质量

  • 评价:良好。代码格式规范,增加换行使得长参数列表更易阅读。
  • 改进建议(魔法字符串/硬编码):挂载选项 "bind", "nosuid", "nodev", "relatime" 以及挂载类型 "bind" 都是硬编码的字符串。建议在类作用域或命名空间内定义常量,避免拼写错误,并提高后续维护性。
    // 建议定义常量
    constexpr auto MOUNT_TYPE_BIND = "bind";
    const string_list BIND_OPTS_WITH_SAFE_DEFAULTS = {"bind", "nosuid", "nodev", "relatime"};

3. 代码性能

  • 评价:优秀。引入 relatime 是一个很好的性能优化点。
  • 解释:传统的 atime 每次读取文件都会更新访问时间,导致频繁的磁盘I/O写操作。relatime 只在文件修改时间(mtime)早于访问时间(atime)时才更新 atime,这大幅减少了不必要的磁盘写操作,对容器运行性能有积极影响。

4. 代码安全

  • 评价:显著提升,但需注意边界情况
  • 正面评价:添加 nosuidnodev 是容器安全基线的标准操作,有效防止了在挂载目录中执行 SUID 提权程序或创建设备文件进行逃逸。
  • 改进建议(防御性编程)
    1. 路径遍历防范appId 作为路径拼接的一部分,如果它是从外部传入的(如应用包名),必须严格校验其不包含 ../ 等路径遍历字符。否则,恶意构造的 appId 可能会将宿主机的其他目录挂载进容器。
    2. 可选的 noexec 选项:考虑到这是一个文档目录(/doc),通常不需要执行二进制文件。为了进一步收紧安全策略,可以考虑添加 noexec 选项,防止攻击者通过该目录执行恶意二进制文件。
      .options = string_list{ "bind", "nosuid", "nodev", "noexec", "relatime" }
      (注意:请确认该目录下确实没有需要执行的程序,如某些 AppImage 或二进制辅助工具,如果有则不能加 noexec)

综合改进后的代码建议

如果你认同上述部分建议,代码可以优化为如下形式(假设确认可以添加 noexec,并提取了常量):

// 在头文件或 cpp 文件顶部定义常量,避免魔法字符串
namespace {
    constexpr auto MOUNT_TYPE_BIND = "bind";
    const string_list SAFE_BIND_OPTIONS = {"bind", "nosuid", "nodev", "noexec", "relatime"};
}

// ... 中间代码 ...

utils::error::Result<void> ContainerCfgBuilder::buildXDGRuntime() noexcept
{
    environment["XDG_RUNTIME_DIR"] = *containerXDGRuntimeDir;

    if (xdpOption) {
        // TODO: 确保 appId 在此之前已经过严格校验,防止路径遍历攻击
        runMount->emplace_back(
          Mount{ .destination = *containerXDGRuntimeDir / "doc",
                 .options = SAFE_BIND_OPTIONS,
                 .source = xdpOption->docMountPoint / "by-app" / appId,
                 .type = MOUNT_TYPE_BIND });
    }

    return LINGLONG_OK;
}

总结:你的原始修改已经是正确且安全的,主要改进点在于防范 appId 的路径遍历风险,以及考虑是否适合加入 noexec 进一步提升安全性。常量提取则是为了长期的代码可维护性。希望这些意见对你有所帮助!

@reddevillg

Copy link
Copy Markdown
Collaborator Author

/gemini review

@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 updates the mount options for the XDP document portal in the container configuration builder, adding 'nosuid', 'nodev', and 'relatime' to the bind mount. A review comment suggests further hardening the security of this mount point by also including the 'noexec' option to prevent the execution of binaries within the document portal.

.type = "bind" });
runMount->emplace_back(
Mount{ .destination = *containerXDGRuntimeDir / "doc",
.options = string_list{ "bind", "nosuid", "nodev", "relatime" },

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.

security-medium medium

For security hardening, it is recommended to include the noexec mount option for the XDP document portal. Since this mount point is intended for accessing documents and data files, preventing the execution of binaries within it reduces the attack surface in the event of a sandbox escape or application compromise. This aligns with security practices in similar containerized environments like Flatpak.

                 .options = string_list{ "bind", "nosuid", "nodev", "noexec", "relatime" },

@codecov

codecov Bot commented May 12, 2026

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
...glong/oci-cfg-generators/container_cfg_builder.cpp 0.00% 4 Missing ⚠️
Files with missing lines Coverage Δ
...glong/oci-cfg-generators/container_cfg_builder.cpp 11.83% <0.00%> (-0.02%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@reddevillg reddevillg requested a review from ComixHe May 12, 2026 08:55
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ComixHe, 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

@ComixHe ComixHe merged commit 9f8c901 into OpenAtom-Linyaps:master May 12, 2026
16 of 17 checks passed
@reddevillg reddevillg deleted the fix_xdp_fuse branch May 12, 2026 12:42
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