Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pengfeixx 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 guide (collapsed on small PRs)Reviewer's GuideThe PR hardens DjVu saving and document conversion against invalid TMPDIR configurations by detecting QTemporaryDir creation failures, logging warnings with the underlying error, and returning failure values instead of continuing with empty paths; successful-path behavior is unchanged. Sequence diagram for temporary directory validation during document operationssequenceDiagram
participant Caller
participant DjVuDocument
participant DocSheet
participant QTemporaryDir
participant FileOperation
alt DjVuDocument::save()
Caller->>DjVuDocument: save()
DjVuDocument->>QTemporaryDir: isValid()
alt temporary directory invalid
DjVuDocument->>QTemporaryDir: errorString()
DjVuDocument-->>Caller: false
else temporary directory valid
DjVuDocument->>QTemporaryDir: path()
DjVuDocument->>FileOperation: saveAs(tempFilePath)
FileOperation-->>Caller: save result
end
else DocSheet::convertedFileDir()
Caller->>DocSheet: convertedFileDir()
DocSheet->>QTemporaryDir: isValid()
alt temporary directory invalid
DocSheet->>QTemporaryDir: errorString()
DocSheet-->>Caller: QString()
else temporary directory valid
DocSheet->>QTemporaryDir: path()
DocSheet-->>Caller: temporary directory path
end
end
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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="reader/uiframe/DocSheet.cpp" line_range="989" />
<code_context>
+ qCWarning(appLog) << "Failed to create temporary directory:" << m_tempDir->errorString();
+ delete m_tempDir;
+ m_tempDir = nullptr;
+ return QString();
+ }
</code_context>
<issue_to_address>
**issue (broader_impact):** The new empty-string failure result is not propagated by its callers: `openFileAsync()` and `openFileExec()` pass it to the renderer, while `saveAsData()` and `openedFilePath()` append filenames to it. This produces paths such as `/saveAsTemp.pdf` or `/temp.pdf`, so conversion and save operations still attempt invalid root-level paths instead of failing gracefully.
**Triggers:** When `QTemporaryDir` creation fails, such as with an invalid `TMPDIR`.
**Suggested fix:** Check the result of `convertedFileDir()` at each caller and abort before invoking the renderer or constructing a derived path; make `openedFilePath()` return an empty result when the conversion directory is unavailable.
</issue_to_address>| qCWarning(appLog) << "Failed to create temporary directory:" << m_tempDir->errorString(); | ||
| delete m_tempDir; | ||
| m_tempDir = nullptr; | ||
| return QString(); |
There was a problem hiding this comment.
issue (broader_impact): The new empty-string failure result is not propagated by its callers: openFileAsync() and openFileExec() pass it to the renderer, while saveAsData() and openedFilePath() append filenames to it. This produces paths such as /saveAsTemp.pdf or /temp.pdf, so conversion and save operations still attempt invalid root-level paths instead of failing gracefully.
Triggers: When QTemporaryDir creation fails, such as with an invalid TMPDIR.
Suggested fix: Check the result of convertedFileDir() at each caller and abort before invoking the renderer or constructing a derived path; make openedFilePath() return an empty result when the conversion directory is unavailable.
When TMPDIR environment variable points to a non-existent directory,
QTemporaryDir construction silently fails and path() returns an empty
string. Two locations were affected:
1. DjVuDocument::save() in reader/document/DjVuModel.cpp:795
- save() used the empty path to construct a temp file path,
causing saveAs() to operate on an invalid path.
2. DocSheet::convertedFileDir() in reader/uiframe/DocSheet.cpp:983
- convertedFileDir() returned the empty path to callers,
causing subsequent file operations to silently fail.
Fix: add isValid() check after QTemporaryDir creation in both
locations. On failure, log a warning with errorString() and return
an appropriate error value (false / empty QString).
0c08cf6 to
3b516b0
Compare
deepin pr auto reviewAI 代码审查报告
总体评价
本次提交修复了 QTemporaryDir 在 TMPDIR 环境变量指向不存在目录时静默失败的问题。在两处位置（DjVuDocument::save() 和 DocSheet::convertedFileDir()）添加了 isValid() 检查，在临时目录创建失败时记录警告日志并返回适当的错误值。代码实现与 commit message 描述的修复目的完全一致，修改小而聚焦，逻辑清晰，无安全风险。 漏洞统计
漏洞对比统计：新增漏洞 0 个，减少漏洞 0 个，持平 0 个 四维度评分维度1: 语法逻辑 ✓ (25/25)
分析:
问题: 无 维度2: 代码质量 ✓ (25/25)
分析:
问题: 无 维度3: 代码性能 ✓ (20/20)
分析:
问题: 无 维度4: 代码安全 ✓ (30/30)
分析:
问题: 无 修改文件清单
代码变更详情reader/document/DjVuModel.cpp (DjVuDocument::save(), 第796-799行)QTemporaryDir tempDir;
if (!tempDir.isValid()) {
qCWarning(appLog) << "Failed to create temporary directory:" << tempDir.errorString();
return false;
}评价: 在栈上创建 QTemporaryDir 后立即检查有效性，失败时记录警告并返回 false，防止后续在空路径上执行 saveAs() 操作。处理方式简洁正确。 reader/uiframe/DocSheet.cpp (DocSheet::convertedFileDir(), 第985-990行)if (!m_tempDir->isValid()) {
qCWarning(appLog) << "Failed to create temporary directory:" << m_tempDir->errorString();
delete m_tempDir;
m_tempDir = nullptr;
return QString();
}评价: 在堆上创建 QTemporaryDir 后检查有效性，失败时记录警告、清理内存（delete + nullptr）并返回空 QString。内存清理确保下次调用时可重新创建临时目录，处理方式正确。 审查结论本次提交是一个高质量的 bug 修复，代码实现与 commit message 描述的目的完全一致。修改在两处关键位置添加了 QTemporaryDir::isValid() 检查，防止了临时目录创建失败时的静默错误行为。代码逻辑正确、结构清晰、无安全风险，建议合并。
|
2 participants
Root Cause Analysis
Two locations in deepin-reader use
QTemporaryDirwithout checkingisValid()after construction. When theTMPDIRenvironment variable points to a non-existent directory,QTemporaryDirsilently fails —path()returns an empty string, and subsequent file operations use invalid paths without any error message or warning.reader/document/DjVuModel.cpp:795— uses the empty path to construct a temp file path, causingsaveAs()to operate on an invalid path.reader/uiframe/DocSheet.cpp:983— returns the empty path to callers, causing subsequent file operations to silently fail.Fix
Add
isValid()check afterQTemporaryDircreation in both locations. On failure, log a warning witherrorString()and return an appropriate error value (falseforsave(), emptyQStringforconvertedFileDir()).Change Safety Assessment
Code Safety
QTemporaryDirconstructs successfully) is completely unchanged.convertedFileDir()properly deletes and nulls the pointer).Business Impact Scope
Affects DjVu document saving and document format conversion (e.g., PDF export) features when TMPDIR is misconfigured. Users with a valid TMPDIR are unaffected.
Verification Suggestion
Test DjVu document save and document conversion/export with
TMPDIRset to a non-existent directory — verify a warning is logged and the operation fails gracefully instead of silently producing invalid files.根因分析
deepin-reader 中两处使用
QTemporaryDir的代码在构造后未检查isValid()。当TMPDIR环境变量指向不存在的目录时，QTemporaryDir静默失败，path()返回空字符串，后续文件操作使用无效路径，且无任何错误日志或警告。reader/document/DjVuModel.cpp:795）— 使用空路径拼接临时文件路径，导致saveAs()在无效路径上执行。reader/uiframe/DocSheet.cpp:983）— 返回空路径给调用方，导致后续文件操作静默失败。修复方案
在两处
QTemporaryDir创建后增加isValid()校验，失败时输出qCWarning日志并返回相应错误值（save()返回false，convertedFileDir()返回空QString）。改动安全评估
代码安全评估
QTemporaryDir构造成功时）完全不变。convertedFileDir()失败路径正确释放并置空指针）。业务影响范围
影响 TMPDIR 配置错误时的 DjVu 文档保存和文档格式转换（如 PDF 导出）功能。TMPDIR 正常的用户不受影响。
验证建议
将
TMPDIR设为不存在的目录后测试 DjVu 文档保存和文档转换/导出功能——验证会输出警告日志并优雅失败，而非静默生成无效文件。Summary by Sourcery
Fail gracefully and report errors when temporary directories cannot be created during document saving or conversion.
Bug Fixes: