cherry-pick 2 release/eagle commits to develop/snipe#367
Merged
deepin-bot[bot] merged 2 commits intolinuxdeepin:develop/snipefrom Mar 30, 2026
Merged
cherry-pick 2 release/eagle commits to develop/snipe#367deepin-bot[bot] merged 2 commits intolinuxdeepin:develop/snipefrom
deepin-bot[bot] merged 2 commits intolinuxdeepin:develop/snipefrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiHua000 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideAdds safety checks and cleanup around CLI-based archive extraction: long-filename handling is disabled when the configured move program is unavailable, extraction failure now triggers cleanup of zero-sized files and empty directories, and progress is forced to 100% on early termination so UIs complete correctly. Updated class diagram for CliInterface extraction-related methodsclassDiagram
class ReadWriteArchiveInterface
class CliInterface {
+PluginFinishType extractFiles(files, options)
+bool moveExtractTempFilesToDest(files, options)
+void removeExtractedFilesOnFailure(strTargetPath, entries)
+bool handleLongNameExtract(files)
+bool checkMoveCapability()
+void readStdout(handleAll)
+void extractProcessFinished(exitCode, exitStatus)
}
CliInterface --|> ReadWriteArchiveInterface
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
bHandleLongName && !checkMoveCapability()block (including repeated warning strings) appears twice inextractFiles; consider refactoring into a small helper to avoid duplication and keep the behavior consistent in one place. - Emitting
signalprogress(100)immediately before killing the process onhandleLinefailure may be misleading for callers that interpret 100% as successful completion; consider distinguishing failure completion from success in the progress reporting. - In
removeExtractedFilesOnFailure, looping repeatedly over the samepathslist to find empty directories can be inefficient for large archives; you might consider tracking directories in a separate structure and walking them bottom-up to minimize repeated checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `bHandleLongName && !checkMoveCapability()` block (including repeated warning strings) appears twice in `extractFiles`; consider refactoring into a small helper to avoid duplication and keep the behavior consistent in one place.
- Emitting `signalprogress(100)` immediately before killing the process on `handleLine` failure may be misleading for callers that interpret 100% as successful completion; consider distinguishing failure completion from success in the progress reporting.
- In `removeExtractedFilesOnFailure`, looping repeatedly over the same `paths` list to find empty directories can be inefficient for large archives; you might consider tracking directories in a separate structure and walking them bottom-up to minimize repeated checks.
## Individual Comments
### Comment 1
<location path="3rdparty/interface/archiveinterface/cliinterface.cpp" line_range="207" />
<code_context>
+
+ if (bHandleLongName && !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.";
+ qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
+ bHandleLongName = false;
</code_context>
<issue_to_address>
**suggestion:** Consider fixing the missing space and reducing duplication of this warning block.
The log currently joins `m_mimetype.name()` and `"Skipping long name handling."` without a space, yielding output like `"zipSkipping long name handling."`. Also, the `bHandleLongName && !checkMoveCapability()` block (including the warning and `bHandleLongName = false`) appears multiple times in `extractFiles`; consider a small helper or shared formatter so the behavior and message stay consistent across call sites.
Suggested implementation:
```cpp
if (bHandleLongName && !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.";
qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
bHandleLongName = false;
}
```
To fully address the duplication concern across `extractFiles`, you can introduce a small helper (either a `static` function in an anonymous namespace in `cliinterface.cpp` or a private member on the relevant class), e.g.:
```cpp
static void logLongNameWithoutMoveProgram(const QMimeType &mimeType, const QVariant &moveProgramProperty)
{
const QString moveProgram = moveProgramProperty.toString();
qWarning() << "Long filename detected, but moveProgram (" << moveProgram << ") is not available.";
qWarning() << "Archive format:" << mimeType.name() << " Skipping long name handling.";
qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
}
```
Then replace each duplicated block of:
```cpp
if (bHandleLongName && !checkMoveCapability()) {
qWarning() << ...;
qWarning() << ...;
qWarning() << ...;
bHandleLongName = false;
}
```
with:
```cpp
if (bHandleLongName && !checkMoveCapability()) {
logLongNameWithoutMoveProgram(m_mimetype, m_cliProps->property("moveProgram"));
bHandleLongName = false;
}
```
so all call sites share the same, correctly formatted message.
</issue_to_address>
### Comment 2
<location path="3rdparty/interface/archiveinterface/cliinterface.cpp" line_range="1165" />
<code_context>
+ if (relPath.isEmpty()) {
+ continue;
+ }
+ paths.append(qMakePair(targetDir.absoluteFilePath(relPath), entry.isDirectory));
+ }
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Using `absoluteFilePath(relPath)` directly may allow `..` segments to escape `strTargetPath`.
If `relPath` contains `..` components (e.g. from a crafted archive), `targetDir.absoluteFilePath(relPath)` may resolve outside `strTargetPath`, and `removeExtractedFilesOnFailure` could delete files or directories beyond the intended root. Consider normalizing and validating `relPath` (e.g. rejecting `..` or verifying the resolved path is under `targetDir.canonicalPath()`) before adding it to `paths`.
</issue_to_address>
### Comment 3
<location path="3rdparty/interface/archiveinterface/cliinterface.cpp" line_range="205" />
<code_context>
}
}
+
+ if (bHandleLongName && !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.";
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the duplicated long-filename/moveProgram check into a helper and simplifying the cleanup routine into separate file/dir phases to make the logic clearer and easier to maintain.
You can reduce duplication and simplify control flow in two spots without changing behavior.
### 1) Consolidate `bHandleLongName && !checkMoveCapability()` logic
You now have the same warning + flag-reset logic twice. Extracting a helper keeps behavior identical and makes future changes safer:
```cpp
// CliInterface.h
class CliInterface : public QObject {
...
private:
void ensureMoveCapabilityForLongNames(bool &bHandleLongName);
...
};
```
```cpp
// CliInterface.cpp
void CliInterface::ensureMoveCapabilityForLongNames(bool &bHandleLongName)
{
if (!bHandleLongName) {
return;
}
if (checkMoveCapability()) {
return;
}
qWarning() << "Long filename detected, but moveProgram (" << m_cliProps->property("moveProgram").toString() << ") 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;
}
```
Then replace both duplicated blocks with:
```cpp
// first location
if (!bLnfs) {
...
}
ensureMoveCapabilityForLongNames(bHandleLongName);
// second location
if (!bLnfs) {
...
}
ensureMoveCapabilityForLongNames(bHandleLongName);
```
This keeps the timing of the check the same while centralizing the messages and policy.
---
### 2) Simplify `removeExtractedFilesOnFailure`
You can avoid the `QList<QPair<QString,bool>>` and the do/while by separating file and directory paths and processing them in two clearly named phases:
```cpp
void CliInterface::removeExtractedFilesOnFailure(const QString &strTargetPath,
const QList<FileEntry> &entries)
{
QList<FileEntry> listToRemove = entries;
if (listToRemove.isEmpty()) {
listToRemove = DataManager::get_instance().archiveData().mapFileEntry.values();
}
if (listToRemove.isEmpty()) {
return;
}
QDir targetDir(strTargetPath);
if (!targetDir.exists()) {
return;
}
QStringList filePaths;
QStringList dirPaths;
for (const FileEntry &entry : listToRemove) {
QString relPath = entry.strFullPath;
if (relPath.endsWith(QLatin1Char('/'))) {
relPath.chop(1);
}
if (relPath.isEmpty()) {
continue;
}
const QString absPath = targetDir.absoluteFilePath(relPath);
if (entry.isDirectory) {
dirPaths << absPath;
} else {
filePaths << absPath;
}
}
// remove zero-size files
for (const QString &path : filePaths) {
QFileInfo fi(path);
if (fi.exists() && fi.isFile() && fi.size() == 0) {
QFile::remove(path);
}
}
// remove empty directories bottom-up
// (one pass is usually enough; if you want to be extra safe, you can loop,
// but in a helper with a clear name)
std::sort(dirPaths.begin(), dirPaths.end(),
[](const QString &a, const QString &b) { return a.length() > b.length(); });
for (const QString &dirPath : dirPaths) {
QDir d(dirPath);
if (d.exists() && d.isEmpty()) {
d.removeRecursively();
}
}
}
```
This keeps the functionality (cleaning up zero-size files and empty directories under the target) but reduces cognitive load by:
- Removing the `QPair<QString,bool>` indirection.
- Making the “file pass” and “directory pass” explicit.
- Using a length-based sort for directories to achieve a bottom‑up removal instead of a `do/while removed` loop.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| if (bHandleLongName && !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."; |
There was a problem hiding this comment.
suggestion: Consider fixing the missing space and reducing duplication of this warning block.
The log currently joins m_mimetype.name() and "Skipping long name handling." without a space, yielding output like "zipSkipping long name handling.". Also, the bHandleLongName && !checkMoveCapability() block (including the warning and bHandleLongName = false) appears multiple times in extractFiles; consider a small helper or shared formatter so the behavior and message stay consistent across call sites.
Suggested implementation:
if (bHandleLongName && !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.";
qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
bHandleLongName = false;
}
To fully address the duplication concern across extractFiles, you can introduce a small helper (either a static function in an anonymous namespace in cliinterface.cpp or a private member on the relevant class), e.g.:
static void logLongNameWithoutMoveProgram(const QMimeType &mimeType, const QVariant &moveProgramProperty)
{
const QString moveProgram = moveProgramProperty.toString();
qWarning() << "Long filename detected, but moveProgram (" << moveProgram << ") is not available.";
qWarning() << "Archive format:" << mimeType.name() << " Skipping long name handling.";
qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
}Then replace each duplicated block of:
if (bHandleLongName && !checkMoveCapability()) {
qWarning() << ...;
qWarning() << ...;
qWarning() << ...;
bHandleLongName = false;
}with:
if (bHandleLongName && !checkMoveCapability()) {
logLongNameWithoutMoveProgram(m_mimetype, m_cliProps->property("moveProgram"));
bHandleLongName = false;
}so all call sites share the same, correctly formatted message.
| if (relPath.isEmpty()) { | ||
| continue; | ||
| } | ||
| paths.append(qMakePair(targetDir.absoluteFilePath(relPath), entry.isDirectory)); |
There was a problem hiding this comment.
🚨 issue (security): Using absoluteFilePath(relPath) directly may allow .. segments to escape strTargetPath.
If relPath contains .. components (e.g. from a crafted archive), targetDir.absoluteFilePath(relPath) may resolve outside strTargetPath, and removeExtractedFilesOnFailure could delete files or directories beyond the intended root. Consider normalizing and validating relPath (e.g. rejecting .. or verifying the resolved path is under targetDir.canonicalPath()) before adding it to paths.
| } | ||
| } | ||
|
|
||
| if (bHandleLongName && !checkMoveCapability()) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the duplicated long-filename/moveProgram check into a helper and simplifying the cleanup routine into separate file/dir phases to make the logic clearer and easier to maintain.
You can reduce duplication and simplify control flow in two spots without changing behavior.
1) Consolidate bHandleLongName && !checkMoveCapability() logic
You now have the same warning + flag-reset logic twice. Extracting a helper keeps behavior identical and makes future changes safer:
// CliInterface.h
class CliInterface : public QObject {
...
private:
void ensureMoveCapabilityForLongNames(bool &bHandleLongName);
...
};// CliInterface.cpp
void CliInterface::ensureMoveCapabilityForLongNames(bool &bHandleLongName)
{
if (!bHandleLongName) {
return;
}
if (checkMoveCapability()) {
return;
}
qWarning() << "Long filename detected, but moveProgram (" << m_cliProps->property("moveProgram").toString() << ") 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;
}Then replace both duplicated blocks with:
// first location
if (!bLnfs) {
...
}
ensureMoveCapabilityForLongNames(bHandleLongName);
// second location
if (!bLnfs) {
...
}
ensureMoveCapabilityForLongNames(bHandleLongName);This keeps the timing of the check the same while centralizing the messages and policy.
2) Simplify removeExtractedFilesOnFailure
You can avoid the QList<QPair<QString,bool>> and the do/while by separating file and directory paths and processing them in two clearly named phases:
void CliInterface::removeExtractedFilesOnFailure(const QString &strTargetPath,
const QList<FileEntry> &entries)
{
QList<FileEntry> listToRemove = entries;
if (listToRemove.isEmpty()) {
listToRemove = DataManager::get_instance().archiveData().mapFileEntry.values();
}
if (listToRemove.isEmpty()) {
return;
}
QDir targetDir(strTargetPath);
if (!targetDir.exists()) {
return;
}
QStringList filePaths;
QStringList dirPaths;
for (const FileEntry &entry : listToRemove) {
QString relPath = entry.strFullPath;
if (relPath.endsWith(QLatin1Char('/'))) {
relPath.chop(1);
}
if (relPath.isEmpty()) {
continue;
}
const QString absPath = targetDir.absoluteFilePath(relPath);
if (entry.isDirectory) {
dirPaths << absPath;
} else {
filePaths << absPath;
}
}
// remove zero-size files
for (const QString &path : filePaths) {
QFileInfo fi(path);
if (fi.exists() && fi.isFile() && fi.size() == 0) {
QFile::remove(path);
}
}
// remove empty directories bottom-up
// (one pass is usually enough; if you want to be extra safe, you can loop,
// but in a helper with a clear name)
std::sort(dirPaths.begin(), dirPaths.end(),
[](const QString &a, const QString &b) { return a.length() > b.length(); });
for (const QString &dirPath : dirPaths) {
QDir d(dirPath);
if (d.exists() && d.isEmpty()) {
d.removeRecursively();
}
}
}This keeps the functionality (cleaning up zero-size files and empty directories under the target) but reduces cognitive load by:
- Removing the
QPair<QString,bool>indirection. - Making the “file pass” and “directory pass” explicit.
- Using a length-based sort for directories to achieve a bottom‑up removal instead of a
do/while removedloop.
Contributor
|
TAG Bot New tag: 6.5.23 |
…ails (e.g. wrong password) Log: as title Bug: https://pms.uniontech.com/bug-view-343249.html
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 pr auto review这段代码主要增加了对解压失败时的清理功能、长文件名处理能力的检查，以及一些错误处理逻辑。以下是对代码的详细审查和改进建议： 1. 代码逻辑与结构1.1 重复代码块在 if (bHandleLongName && !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.";
qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
bHandleLongName = false;
}改进建议：将这段代码提取为一个单独的函数或内联函数，避免重复。例如： void CliInterface::handleLongNameCapability(bool &bHandleLongName) {
if (bHandleLongName && !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.";
qWarning() << "The extraction tool will report errors for files with names exceeding system limit (255 bytes).";
bHandleLongName = false;
}
}1.2
|
Contributor
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1 fix: [unrar] can not extract rar files with longFilenames
2 fix: remove empty files left when split-volume encrypted extraction fails (e.g. wrong password)
Summary by Sourcery
Improve archive extraction robustness for long file names and failed encrypted extractions.
Bug Fixes:
Enhancements: