Skip to content

fix: set SyslogIdentifier for pre-launch hook scripts#334

Merged
ComixHe merged 1 commit intolinuxdeepin:masterfrom
mhduiy:log
Feb 26, 2026
Merged

fix: set SyslogIdentifier for pre-launch hook scripts#334
ComixHe merged 1 commit intolinuxdeepin:masterfrom
mhduiy:log

Conversation

@mhduiy
Copy link
Contributor

@mhduiy mhduiy commented Feb 25, 2026

此提交为了修复 DDE 的 journalctl 中经常出现debfix.sh标识符的问题

原因

经过分析,systemd会自动捕获stderr和stdout到日志系统,只要dtk程序声明了DLogManager::registerConsoleAppender();,就会把日志打印到标准输出,从而进入journalctl中。AM启动应用时套用了一个hook,这个hook会wrap启动程序,虽然debFix中使用了exec去启动主程序,但是递给systemd的cmdline已经包含了该脚本名称,根据其文档,会把这个脚本作为标识符传进日志系统中,就导致出现了debfix.sh 这个标识符,下面是systemd拿到的cmdline

_CMDLINE=/bin/bash /usr/libexec/deepin/application-manager/debFix.sh dde-control-center --show

识别到SYSLOG_IDENTIFIER=debFix.sh,所以最终就表现为debFix.sh输出,这是不合理的

修复方案

只要wrap脚本就可能会出现这样的现象,最好的方式就是手动指定标识符,手动解析cmdline 不太靠谱,在deepin上本身存在一个标识符 DSG_APP_ID,且通过AM启动时默认注入了这个环境变量,所以考虑使用 DSG_APP_ID 作为标识符,大部分情况下他就是应用启动器文件的文件名(经过一些转译)


另外,额外发现大部分通过AM启动的dtk应用会打印两次重复日志,其实一次是systemd捕获标准输出到journalctl的,一个是dtk程序可能会注册DLogManager::registerJournalAppender();,使用journal的api传递进journal日志系统中,使用的改进方案是当检测到应用由systemd启动、重定向输出并手动开启了journal日志打印,则不向控制台打印日志

影响

通过AM启动的应用,重定向标准输出到日志系统的标识符会变为 DSG_APP_ID ,大部分情况下就是其转译后desktop文件的文件名,其余仅通过systemd service 或者 通过journal api 写入的日志不受影响

The issue was that when AM (Application Manager) supports pre-launch
hooks, a wrapper script is executed before starting the application.
However, systemd uses the script name as the SyslogIdentifier in
its command line, causing ambiguous identifiers in journalctl logs.
To ensure consistent and clear logging, we now explicitly set the
SyslogIdentifier to DSG_APP_ID.

Changes made:
1. Modified cmdParse() in main.cpp to parse and store the
SyslogIdentifier from command line arguments
2. Added logic to append the SyslogIdentifier property to the systemd
unit if provided
3. Updated ApplicationService::Launch() to include --SyslogIdentifier
parameter with the application ID when constructing the launch command
4. Increased the estimated command size to accommodate the new parameter

This ensures that journalctl logs will consistently use the application
ID as the identifier, making logs easier to filter and understand
regardless of wrapper script names.

fix: 为启动前钩子脚本设置系统日志标识符

问题在于当AM支持启动前钩子时,会在启动应用程序前执行一个包装脚本。然
而systemd会使用脚本名称作为命令行中的SyslogIdentifier,导致journalctl
日志中出现歧义的标识符。为确保日志的一致性和清晰度,我们现在明确将
SyslogIdentifier设置为DSG_APP_ID。

具体修改:
1. 修改main.cpp中的cmdParse()函数以解析和存储命令行参数中的
SyslogIdentifier
2. 添加逻辑,在提供SyslogIdentifier时将其附加到systemd单元属性
3. 更新ApplicationService::Launch()方法,在构建启动命令时包含带有应用程
序ID的--SyslogIdentifier参数
4. 增加了预估命令大小以容纳新参数

这确保journalctl日志将始终使用应用程序ID作为标识符,无论包装脚本名称如
何,都能使日志更易于过滤和理解。
@sourcery-ai
Copy link

sourcery-ai bot commented Feb 25, 2026

Reviewer's Guide

Ensures systemd units launched via the app-launch-helper always set SyslogIdentifier explicitly (using the application ID) by propagating a new SyslogIdentifier argument from ApplicationService::Launch through the launch helper, and appending it as a unit property on the D-Bus call.

Sequence diagram for launching app with explicit SyslogIdentifier

sequenceDiagram
    actor User
    participant ApplicationService
    participant AppLaunchHelper as app-launch-helper
    participant systemd
    participant journald

    User->>ApplicationService: Launch(action, fields, args)
    ApplicationService->>ApplicationService: Build newCommands
    ApplicationService->>ApplicationService: Add --unitName=app-DDE-id@uuid.service
    ApplicationService->>ApplicationService: Add --SyslogIdentifier=applicationId
    ApplicationService->>AppLaunchHelper: Exec with newCommands

    AppLaunchHelper->>AppLaunchHelper: cmdParse(cmdLines)
    AppLaunchHelper->>AppLaunchHelper: Parse SyslogIdentifier from cmdLines
    AppLaunchHelper->>AppLaunchHelper: Build D-Bus msg
    AppLaunchHelper->>AppLaunchHelper: Append SyslogIdentifier property
    AppLaunchHelper->>systemd: sd_bus_call(StartTransientUnit)

    systemd->>systemd: Start app unit with SyslogIdentifier=applicationId
    systemd->>journald: Send logs tagged with applicationId
    journald-->>User: journalctl filter SyslogIdentifier=applicationId
Loading

File-Level Changes

Change Details Files
Parse SyslogIdentifier from helper CLI and append it as a systemd unit property on the D-Bus message when present.
  • Add local storage for the SyslogIdentifier argument in cmdParse
  • Detect SyslogIdentifier key/value pairs while scanning command-line arguments and store the value
  • After other validation, append a SyslogIdentifier property to the sd-bus message when a non-empty value was provided
apps/app-launch-helper/src/main.cpp
Include the application ID as --SyslogIdentifier in the launch-helper command line and adjust the estimated argument list size accordingly.
  • Increase the estimatedSize count to account for the new CLI argument
  • Append a --SyslogIdentifier= argument when building the launch-helper command list so systemd receives the explicit identifier
src/dbus/applicationservice.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@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.

Hey - I've left some high level feedback:

  • If application IDs can contain characters not ideal for systemd/journal identifiers (e.g. spaces, special chars), consider reusing escapeApplicationId(...) or a similar sanitization for the --SyslogIdentifier value instead of passing this->id() raw.
  • The estimatedSize constant in ApplicationService::Launch is a bit opaque; consider deriving it from named counts (e.g. baseArgs = 3 for --unitName, --SyslogIdentifier, --SourcePath) or adding a brief comment so future changes don’t accidentally desync the estimate and the actual arguments appended.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- If application IDs can contain characters not ideal for systemd/journal identifiers (e.g. spaces, special chars), consider reusing `escapeApplicationId(...)` or a similar sanitization for the `--SyslogIdentifier` value instead of passing `this->id()` raw.
- The `estimatedSize` constant in `ApplicationService::Launch` is a bit opaque; consider deriving it from named counts (e.g. `baseArgs = 3` for `--unitName`, `--SyslogIdentifier`, `--SourcePath`) or adding a brief comment so future changes don’t accidentally desync the estimate and the actual arguments appended.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

deepin pr auto review

这段代码的修改旨在为应用启动添加 SyslogIdentifier 支持,以便在 systemd 日志中更好地识别应用。整体逻辑是正确的,能够实现功能。以下是从语法逻辑、代码质量、代码性能和代码安全四个方面提出的详细审查意见和改进建议:

1. 语法逻辑审查

  • main.cpp 中的逻辑判断错误(严重)
    main.cpp 的第 372 行:
    if (ret = sd_bus_message_append(..., syslogIdentifier.c_str()); ret < 0)
    这里存在一个逻辑错误。sd_bus_message_append 函数在成功时返回 0 或正整数,失败时返回负数。
    目前的写法 if (ret = ...; ret < 0) 的逻辑是:如果赋值成功,且返回值小于 0(即失败),则进入 if 块。
    问题在于sd_bus_message_append 的返回值被赋给 ret,然后检查 ret < 0。如果 ret < 0(失败),代码打印错误并返回 nullopt。这部分逻辑本身处理了错误情况,但是,结合代码上下文,通常这类 API 调用失败是严重错误,应该阻止后续操作。
    更严重的问题if 语句中使用了逗号运算符 ,,结合 if 的判断条件,这里的写法非常晦涩且容易出错。虽然 C++ 允许在 if 初始化语句中使用逗号,但这里的意图应该是“如果调用失败(ret < 0)则报错”。
    建议:为了清晰起见,建议拆分语句,或者确保逻辑意图明确。如果意图是“失败则报错”,目前的逻辑是对的,但写法不推荐。
    修正建议
    ret = sd_bus_message_append(msg, "(sv)", "SyslogIdentifier", "s", syslogIdentifier.c_str());
    if (ret < 0) {
        sd_journal_perror("failed to append SyslogIdentifier property.");
        return std::nullopt;
    }

2. 代码质量审查

  • 错误处理一致性
    main.cpp 中,sd_bus_message_append 失败时使用了 sd_journal_perror 打印错误。而在 processKVPair 调用失败时(第 382 行),似乎没有看到明确的错误日志打印(虽然 processKVPair 内部可能处理了)。建议保持错误处理的一致性,确保所有关键路径失败都有日志记录,便于调试。

  • 字符串处理
    syslogIdentifier 是一个 std::string,在 sd_bus_message_append 中使用 c_str() 是正确的。但在 applicationservice.cpp 中,直接使用 this->id() 构造命令行参数。如果 this->id() 包含空格或特殊字符,可能会导致命令行解析错误或注入风险(见安全部分)。

3. 代码性能审查

  • 字符串拷贝
    cmdParse 函数中,syslogIdentifier 是通过 value(可能是 std::string_view 或类似类型)赋值的。如果 value 是视图,赋值给 std::string 会发生一次拷贝。考虑到这通常发生在进程启动阶段,频率不高,性能影响可忽略。

  • 内存预留
    applicationservice.cpp 中,estimatedSize 从 5 增加到 6,这是正确的,因为增加了一个参数。newCommands.reserve(estimatedSize) 是良好的实践,避免了多次重分配。

4. 代码安全审查

  • 命令注入风险(中等)
    applicationservice.cpp 中:
    newCommands << QStringLiteral("--SyslogIdentifier=%1").arg(this->id());
    如果 this->id() 包含 shell 元字符(如 ;, &, |, $, ` 等),且 app-launch-helper 或后续处理命令的组件没有正确引用参数,可能会导致命令注入。
    建议
    1. 确保 this->id() 的来源是可信的或经过严格校验的(例如,只允许字母、数字、连字符和下划线)。
    2. cmdParse 中,虽然 SyslogIdentifier 是作为值传递给 DBus 的,但 systemd 对 SyslogIdentifier 有格式限制(通常要求是合法的 unit 名称或简单字符串)。如果传入非法字符,systemd 可能会拒绝或截断,但这本身不是注入风险,只是功能失效。
    3. 如果 app-launch-helper 解析命令行参数的方式是简单的字符串分割,确保 --SyslogIdentifier=... 中的 ... 部分不包含会导致解析错误的字符(如 =)。

综合改进建议代码

apps/app-launch-helper/src/main.cpp:

// ... (前略)

    // 检查 SyslogIdentifier 是否非空
    if (!syslogIdentifier.empty()) {
        // 修正后的错误处理逻辑
        int ret = sd_bus_message_append(msg,
                                        "(sv)",
                                        "SyslogIdentifier",
                                        "s",
                                        syslogIdentifier.c_str());
        if (ret < 0) {
            sd_journal_perror("failed to append SyslogIdentifier property.");
            return std::nullopt;
        }
    }

    if (ret = processKVPair(msg, props); ret < 0) {  // process props
        // 建议在此处也添加日志,如果 processKVPair 内部没有的话
        // sd_journal_perror("failed to process KV pairs.");
        return std::nullopt;
    }
// ... (后略)

src/dbus/applicationservice.cpp:

// ... (前略)
            QStringList newCommands;
            // 预留空间计算正确
            const int estimatedSize = 6 + cmds.size() + task.command.size() + (value.isValid() ? 1 : 0);
            newCommands.reserve(estimatedSize);
            
            // 确保 id() 是安全的,或者进行转义/校验
            QString appId = this->id();
            // 可选:简单的校验示例,确保只包含安全字符
            // if (appId.contains(QRegularExpression("[^a-zA-Z0-9._-]"))) { ... handle error ... }

            newCommands
                << QStringLiteral("--unitName=app-DDE-%1@%2.service").arg(escapeApplicationId(appId), instanceRandomUUID);
            newCommands << QStringLiteral("--SyslogIdentifier=%1").arg(appId);
            newCommands << QStringLiteral("--SourcePath=%1").arg(m_desktopSource.sourcePath());
            newCommands << std::move(cmds);
// ... (后略)

总结

这段代码修改在功能上是合理的,主要改进点在于修正 main.cpp 中晦涩且可能存在误解的 if 语句写法,并增强对 SyslogIdentifier 内容的安全性校验。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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 da8e0d8 into linuxdeepin:master Feb 26, 2026
17 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