Skip to content

fix: [unrar] can not extract rar files with longFilenames#366

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:release/eaglefrom
LiHua000:release/eagle
Feb 26, 2026
Merged

fix: [unrar] can not extract rar files with longFilenames#366
deepin-bot[bot] merged 1 commit intolinuxdeepin:release/eaglefrom
LiHua000:release/eagle

Conversation

@LiHua000
Copy link
Contributor

@LiHua000 LiHua000 commented Feb 26, 2026

prevent extracting RAR files with long filenames when unrar tool is missing

Log: as title

Bug: https://pms.uniontech.com/bug-view-343139.html

Summary by Sourcery

Bug Fixes:

  • Prevent attempting long-filename handling for RAR archives when the configured move program is not available, avoiding extraction failures for files exceeding system name limits.

@sourcery-ai
Copy link

sourcery-ai bot commented Feb 26, 2026

Reviewer's Guide

Adds capability checks for an external moveProgram before enabling long-filename handling during extraction, and disables that handling with warnings when the required tool (e.g., unrar’s helper) is unavailable to avoid failed extractions for RAR files with long filenames.

Flow diagram for long filename handling with moveProgram capability check

flowchart TD
    A[bHandleLongName is true?] -->|no| Z[Proceed without long filename handling]
    A -->|yes| B[Read moveProgram from m_cliProps]

    B --> C{Is moveProgram empty?}
    C -->|yes| D[Set hasMoveCapability to false]
    C -->|no| E[Find executable path with QStandardPaths::findExecutable]

    E --> F{Executable path found?}
    F -->|yes| G[Set hasMoveCapability to true]
    F -->|no| D

    D --> H{hasMoveCapability is true?}
    G --> H

    H -->|yes| I[Keep bHandleLongName true and call handleLongNameExtract]
    H -->|no| J[Log warnings about missing moveProgram and archive format]
    J --> K[Set bHandleLongName to false]
    K --> Z
    I --> L[If handleLongNameExtract fails set m_eErrorType to ET_FileWriteError]
    L --> Z
Loading

File-Level Changes

Change Details Files
Guard long-filename handling logic with a runtime check for an available moveProgram and fall back to normal extraction (with warnings) when it is missing.
  • After determining whether long-filename handling is needed, resolve the moveProgram from cli properties, search it via QStandardPaths::findExecutable, and set a hasMoveCapability flag.
  • If long-filename handling is requested but moveProgram is not found, log detailed qWarning messages about the missing tool, archive format, and potential extraction errors, then disable long-filename handling by setting the flag to false.
  • Duplicate the moveProgram capability check in the later phase before invoking handleLongNameExtract to ensure long-filename handling is disabled if moveProgram is unavailable at that point as well.
3rdparty/interface/archiveinterface/cliinterface.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 found 1 issue, and left some high level feedback:

  • The hasMoveCapability/moveProgram lookup and associated qWarning block is duplicated in two places; consider factoring this into a small helper or a single early check to avoid repetition and keep the long-name handling logic in one place.
  • Since QStandardPaths::findExecutable(moveProgram) is relatively expensive and does not change during a single extraction, you could compute the move capability once and reuse it instead of calling it multiple times.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The `hasMoveCapability`/`moveProgram` lookup and associated `qWarning` block is duplicated in two places; consider factoring this into a small helper or a single early check to avoid repetition and keep the long-name handling logic in one place.
- Since `QStandardPaths::findExecutable(moveProgram)` is relatively expensive and does not change during a single extraction, you could compute the move capability once and reuse it instead of calling it multiple times.

## Individual Comments

### Comment 1
<location path="3rdparty/interface/archiveinterface/cliinterface.cpp" line_range="199-212" />
<code_context>
             }
         }
+
+        // 检查是否有可用的 moveProgram 来处理长文件名 (unrar无法修改rar压缩包内文件名)
+        bool hasMoveCapability = false;
+        QString moveProgram = m_cliProps->property("moveProgram").toString();
+        if (!moveProgram.isEmpty()) {
+            QString moveProgramPath = QStandardPaths::findExecutable(moveProgram);
+            hasMoveCapability = !moveProgramPath.isEmpty();
+        }
+
+        if (bHandleLongName && !hasMoveCapability) {
+            qWarning() << "Long filename detected, but moveProgram (" << moveProgram << ") is not available.";
+            qWarning() << "Archive format:" << m_mimetype.name() << "Skipping long name handling.";
+            qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
+            bHandleLongName = false;
+        }
+
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicated moveProgram capability check and warning logic.

The `moveProgram` lookup and `hasMoveCapability` handling (warnings and `bHandleLongName` reset) now appear in two places in this function, duplicating work and log messages and obscuring control flow. It would be clearer to perform this capability check only once—ideally after `bHandleLongName` is finalized (post `bLnfs`/NAME_MAX scan)—or encapsulate it in a small helper such as `bool ensureMoveCapabilityOrDisableLongName()` to avoid repetition and make the conditions for disabling `bHandleLongName` explicit.

```suggestion
```
</issue_to_address>

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.

prevent extracting RAR files with long filenames when unrar tool is missing

Log: as title

Bug: https://pms.uniontech.com/bug-view-343139.html
@deepin-ci-robot
Copy link

deepin pr auto review

以下是对提供的 git diff 的代码审查意见,涵盖语法逻辑、代码质量、代码性能和代码安全四个方面。

1. 语法与逻辑

  • 代码重复

    • cliinterface.cpp 中,检查长文件名处理能力的代码块完全重复了两次(一次在 bLnfs 检查之前,一次在之后):
      if (bHandleLongName && !checkMoveCapability()) {
          qWarning() << "Long filename detected, but moveProgram (" << m_cliProps->property("moveProgram").toString() << ") is not available.";
          // ... (重复代码)
          bHandleLongName = false;
      }
    • 改进意见:这两段检查逻辑是完全一样的。由于 bHandleLongName 是一个局部变量,且后续逻辑依赖于它的状态,建议将这段检查合并到 bHandleLongName 被设置之后、且在 bLnfs 分支之前,或者提取为一个私有函数(尽管对于这种简单逻辑,合并位置更合适)。例如,在确定 bHandleLongName 的初始值后,立即进行一次检查。
  • 逻辑位置

    • 第一次检查位于 destPath 判断逻辑之后,password 赋值之前。第二次检查位于 password 赋值之后,bLnfs 判断之后。
    • 改进意见:这段检查的核心目的是确认系统是否有能力处理长文件名。它应该在决定是否调用 handleLongNameExtract 之前完成。目前的逻辑流看起来有些分散,建议将其整理到函数的前部,统一处理 bHandleLongName 的有效性。

2. 代码质量

  • 日志输出

    • qWarning() 的使用是恰当的,能够帮助调试。
    • 改进意见:为了便于日志分析和过滤,建议在日志中加入更具体的上下文信息,例如当前正在处理的压缩包名称或函数名(虽然 qWarning 默认包含类别,但自定义前缀有时更有用)。
  • 函数封装

    • 新增的 checkMoveCapability() 函数封装良好,职责单一,符合单一职责原则。
  • 变量命名

    • bHandleLongNamebLnfs 等变量名符合匈牙利命名法习惯(在 Qt/C++ 旧代码中常见),虽然可读性尚可,但现代 C++ 风格更推荐使用 handleLongNameisLnfs 等布尔语义更清晰的名字。不过考虑到这是在既有代码库中修改,保持现有风格是可以接受的。

3. 代码性能

  • 系统调用开销
    • checkMoveCapability() 中调用了 QStandardPaths::findExecutable(moveProgram)。这是一个文件系统操作,用于查找可执行文件。
    • 分析:在 extractFiles 函数中,这段检查代码可能被执行两次(如果之前的重复代码未被合并)。虽然 findExecutable 通常有缓存机制,且文件系统查找相对较快,但在高频调用的场景下,重复执行是不必要的开销。
    • 改进意见:再次强调消除代码重复,确保在一个解压操作周期内只检查一次能力。此外,如果 m_cliProps 的属性在对象生命周期内不变,可以考虑在构造函数或初始化阶段缓存 checkMoveCapability 的结果,避免每次解压都去查找路径。

4. 代码安全

  • 路径处理
    • checkMoveCapability 依赖 m_cliProps->property("moveProgram")。如果配置文件或属性被恶意篡改,传入了一个危险的程序名或路径,虽然 findExecutable 只会在系统路径中查找,降低了直接执行任意路径的风险,但仍需确保 moveProgram 的来源是可信的。
  • 长文件名截断风险
    • 代码逻辑中,如果 checkMoveCapability() 返回 false,则 bHandleLongName 被设为 false,随后注释提到 "The extraction tool will report errors for files with names exceeding system limit"。
    • 改进意见:这是一个降级策略,虽然安全(不会导致未定义行为),但用户体验较差。如果可能,建议在解压前遍历文件列表,检测是否存在超长文件名。如果存在且无法处理,应尽早向用户报错并终止解压,而不是让解压工具报错,这样更符合"快速失败"(Fail Fast)原则。

5. 具体修改建议

针对上述分析,建议对 extractFiles 函数中的逻辑进行如下重构(伪代码示意):

PluginFinishType CliInterface::extractFiles(const QList<FileEntry> &files, const Options &options)
{
    // ... 前面的代码 ...

    // 1. 确定初始的 bHandleLongName 状态
    bool bHandleLongName = /* ... 原有逻辑 ... */;

    // 2. 统一检查处理长文件名的能力 (消除重复)
    if (bHandleLongName) {
        if (!checkMoveCapability()) {
            qWarning() << "Long filename detected, but moveProgram (" 
                       << m_cliProps->property("moveProgram").toString() 
                       << ") is not available.";
            qWarning() << "Archive format:" << m_mimetype.name() << "Skipping long name handling.";
            // 可以在这里进一步检查是否真的有超长文件名,如果有则直接返回错误
            bHandleLongName = false;
        }
    }

    // 3. 后续逻辑使用确定后的 bHandleLongName
    if (destPath.startsWith("/tmp") && destPath.contains("/deepin-compressor-")) {
        // ...
    }
    
    // ...
    
    if (!bLnfs) {
        // ...
    }
    
    // 这里不再需要重复 checkMoveCapability 的代码块
    if (bHandleLongName) {
        if (!handleLongNameExtract(arcData.mapFileEntry.values())) {
            // ...
        }
    }

    // ...
}

总结

这段代码的主要问题在于逻辑重复,这不仅影响代码的整洁性,也带来了微小的性能损耗。通过合并检查逻辑,可以显著提升代码质量。此外,新增的 checkMoveCapability 函数实现合理,但在性能敏感场景下可考虑缓存其结果。安全性方面,目前的降级处理是安全的,但用户体验有优化空间。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LiHua000, max-lvs

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

@LiHua000
Copy link
Contributor Author

/merge

@deepin-bot deepin-bot bot merged commit 2aba266 into linuxdeepin:release/eagle Feb 26, 2026
15 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