Skip to content

fix: start package manager before repo loading in peer mode#1687

Merged
dengbo11 merged 1 commit into
OpenAtom-Linyaps:masterfrom
dengbo11:fix-no-dbus-repo
Jun 24, 2026
Merged

fix: start package manager before repo loading in peer mode#1687
dengbo11 merged 1 commit into
OpenAtom-Linyaps:masterfrom
dengbo11:fix-no-dbus-repo

Conversation

@dengbo11

Copy link
Copy Markdown
Collaborator
  1. Move the ll-package-manager --no-dbus startup logic from Cli::getPkgMan() to main.cpp
  2. Start the package manager immediately after detecting peer mode (--no-dbus flag)
  3. Ensure the package manager is initialized before OSTreeRepo::loadFromPath() is called
  4. The previous location in getPkgMan() was too late in the execution flow, as the repo was already loaded before the package manager had a chance to initialize

Influence:

  1. Test ll-cli with --no-dbus flag to verify package manager starts correctly
  2. Verify that repository loading succeeds in peer mode
  3. Test that subcommands work properly after the package manager initializes
  4. Verify the 1-second sleep is sufficient for package manager startup
  5. Test running ll-cli commands in --no-dbus mode multiple times to ensure no duplicate processes
  6. Verify sudo permissions and environment variable preservation work correctly

@dengbo11 dengbo11 requested a review from reddevillg June 24, 2026 11:40

@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 moves the startup logic of the ll-package-manager process in --no-dbus mode from Cli::getPkgMan() to main.cpp before the repository is loaded. The review feedback highlights that because of this relocation, the root user check (getuid() != 0) is executed too late. It is recommended to perform this check in main.cpp before spawning the process, and to verify the return value of QProcess::startDetached to ensure the process starts successfully.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread apps/ll-cli/src/main.cpp
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

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

Files with missing lines Patch % Lines
apps/ll-cli/src/main.cpp 0.00% 6 Missing ⚠️
Files with missing lines Coverage Δ
libs/linglong/src/linglong/cli/cli.cpp 1.77% <ø> (+<0.01%) ⬆️
apps/ll-cli/src/main.cpp 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

1. Move the ll-package-manager --no-dbus startup logic from
Cli::getPkgMan() to main.cpp
2. Start the package manager immediately after detecting peer mode
(--no-dbus flag)
3. Ensure the package manager is initialized before
OSTreeRepo::loadFromPath() is called
4. The previous location in getPkgMan() was too late in the execution
flow, as the repo was already loaded before the package manager had a
chance to initialize

Influence:
1. Test ll-cli with --no-dbus flag to verify package manager starts
correctly
2. Verify that repository loading succeeds in peer mode
3. Test that subcommands work properly after the package manager
initializes
4. Verify the 1-second sleep is sufficient for package manager startup
5. Test running ll-cli commands in --no-dbus mode multiple times to
ensure no duplicate processes
6. Verify sudo permissions and environment variable preservation work
correctly
@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

deepin pr auto review

★ 总体评分:35分

■ 【总体评价】

代码将进程启动逻辑下沉以解决重复拉起问题,但引入了严重的不安全临时文件使用漏洞和未解决的竞态条件。
逻辑正确但因存在高危符号链接劫持风险及中危状态校验缺失扣65分

■ 【详细分析】

  • 1.语法逻辑存在错误✕

代码将服务启动从 getPkgMan() 移至 main() 函数,解决了重复触发的问题,但注释中写“避免硬编码 sleep 导致的竞态问题”,实际代码中仍然保留了 std::this_thread::sleep_for(1s);,注释与实现存在严重矛盾,且竞态问题并未真正解决,服务可能未就绪即进行后续操作。
潜在问题:竞态条件依然存在;注释与代码实现不一致导致维护者误解。
建议:移除误导性注释,实现基于 socket 文件状态或连接重试的真实就绪检测机制。

  • 2.代码质量一般✕

职责分离做得较好,getPkgMan() 不再承担启动进程的副作用。但使用固定时间休眠作为同步手段是极差的实践,属于典型的反模式代码。
潜在问题:硬编码的 1 秒休眠在不同负载的机器上表现不一致,可能导致启动过慢或失败。
建议:引入带有超时的轮询机制,检查 socket 文件是否可连接,替代盲等。

  • 3.代码性能存在性能问题✕

将休眠从可能被多次调用的 getPkgMan() 移至仅执行一次的 main() 入口,显著减少了无谓的阻塞时间,这是性能上的改进。
潜在问题:固定 1 秒的阻塞在每次启动时依然存在,且在服务快速启动的场景下白白浪费时间。
建议:改为短间隔(如 100ms)轮询,一旦 socket 可用立即返回,降低平均等待时间。

  • 4.代码安全存在 2 个安全漏洞(高危1个,中危1个)✕

漏洞对比统计:新增漏洞 2 个,减少漏洞 0 个,持平 0 个
代码在处理无 DBus 模式的本地通信时,使用了不安全的固定路径 socket,且缺乏对后台进程启动状态的校验,存在本地劫持和提权风险。

  • 安全漏洞1(【高危】):不安全的临时文件/Socket路径 在 libs/linglong/src/linglong/cli/cli.cppgetPkgMan() 函数中,使用了硬编码的 unix:path=/tmp/linglong-package-manager.socket/tmp 目录默认全局可写,攻击者可预先创建指向敏感文件的符号链接。当 ll-package-manager 尝试绑定该 socket 时,可能引发拒绝服务,或攻击者伪造 socket 监听恶意响应,导致连接该 socket 的 root 权限 CLI 工具被欺骗执行恶意指令。——非常重要

  • 安全漏洞2(【中危】):进程启动缺乏状态校验 在 apps/ll-cli/src/main.cppmain 函数中,通过 QProcess::startDetached 拉起服务后仅盲等 1 秒,未校验进程 PID 是否存活或 socket 是否由预期进程创建。在竞态窗口期,恶意用户可能抢占 socket,导致后续 CLI 操作连接至恶意后端,造成权限提升。——非常重要

  • 建议:避免在 /tmp 下使用固定路径,应使用 abstract: 抽象命名空间 socket(Linux 特有,无文件系统路径),或放在受保护的目录(如 /run/linglong/)并设置严格权限;启动服务后应轮询检查 socket 文件的属主和权限,或通过 PID 文件验证进程身份。

■ 【改进建议代码示例】

// apps/ll-cli/src/main.cpp
    const bool peerMode = noDBusFlag->count() > 0;
    if (peerMode) {
        if (getuid() != 0) {
            LogE("--no-dbus should only be used by root user.");
            return -1;
        }

        // Start ll-package-manager --no-dbus first to initialize the repository
        QProcess::startDetached("sudo",
                                { "--user",
                                  LINGLONG_USERNAME,
                                  "--preserve-env=QT_FORCE_STDERR_LOGGING",
                                  "--preserve-env=QDBUS_DEBUG",
                                  LINGLONG_LIBEXEC_DIR "/ll-package-manager",
                                  "--no-dbus" });
        
        // Secure polling mechanism to wait for service readiness
        const QString secureSocketPath = "/run/linglong/linglong-package-manager.socket";
        constexpr int maxRetries = 20;
        constexpr int retryIntervalMs = 100;
        
        for (int i = 0; i < maxRetries; ++i) {
            QFileInfo fileInfo(secureSocketPath);
            // Verify socket exists and is owned by root to prevent TOCTOU attacks
            if (fileInfo.exists() && fileInfo.ownerId() == 0) {
                break;
            }
            QThread::msleep(retryIntervalMs);
        }
    }
    auto repo = linglong::repo::OSTreeRepo::loadFromPath(LINGLONG_ROOT);
// libs/linglong/src/linglong/cli/cli.cpp
    if (this->peerMode) {
        LogW("some subcommands will failed in --no-dbus mode.");
        // Use Linux abstract namespace to prevent symlink hijacking in /tmp
        const auto pkgManAddress = QString("unix:abstract=/linglong-package-manager");

        const auto &pkgManConn =
          QDBusConnection::connectToPeer(pkgManAddress, "ll-package-manager");

@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

@dengbo11: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
github-pr-review-ci e0c65aa link true /test github-pr-review-ci

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@deepin-ci-robot

Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

@dengbo11 dengbo11 merged commit 7ad802c into OpenAtom-Linyaps:master Jun 24, 2026
15 of 18 checks passed
@dengbo11 dengbo11 deleted the fix-no-dbus-repo branch July 1, 2026 05:49
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