Skip to content

fix(pdfium): allow zero-length WriteBlock() no-op#256

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
LiHua000:master
Mar 18, 2026
Merged

fix(pdfium): allow zero-length WriteBlock() no-op#256
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
LiHua000:master

Conversation

@LiHua000
Copy link
Contributor

Treat WriteBlock(size==0) as success and only require non-null data when size>0. This prevents aborts when serializing empty names/streams and aligns implementations/call sites with the interface contract.

log: fix bug

upstream:chromium/pdfium@0e723c5#diff-89818b20cf16ed5dfbf8b50d13c34fc3596cacf04bd3c4da3c2d433a6bbe6526 Bug: https://pms.uniontech.com/bug-view-353429.html

Treat WriteBlock(size==0) as success and only require non-null data when
size>0. This prevents aborts when serializing empty names/streams and aligns
implementations/call sites with the interface contract.

log: fix bug

upstream:chromium/pdfium@0e723c5#diff-89818b20cf16ed5dfbf8b50d13c34fc3596cacf04bd3c4da3c2d433a6bbe6526
Bug: https://pms.uniontech.com/bug-view-353429.html
@deepin-ci-robot
Copy link

deepin pr auto review

这段代码修改主要涉及 PDF 处理库(PDFium)中的流写入接口实现,目的是统一处理零长度写入的情况,并改进对空指针的检查逻辑。以下是对代码的详细审查和改进意见:

1. 代码逻辑与安全性分析

修改点 1: CFX_FileBufferArchive::WriteBlock

修改内容:

+  if (size == 0)
+    return true;
+
   ASSERT(pBuf);
-  ASSERT(size > 0);
+  if (!pBuf)
+    return false;

审查意见:

  • 逻辑改进: 原代码使用 ASSERT(size > 0),这意味着在 Debug 模式下,如果传入 size=0 会触发断言失败导致程序崩溃,而在 Release 模式下行为未定义。新代码显式处理 size == 0 的情况,将其定义为合法的空操作,这符合 POSIX write 等系统调用的标准行为,增强了代码的健壮性。
  • 安全性:ASSERT(pBuf) 改为运行时检查 if (!pBuf) return false; 是非常正确的。ASSERT 仅在 Debug 版本生效,Release 版本会忽略它。如果传入 size > 0pBuf == nullptr,Release 版本会继续执行导致空指针解引用崩溃。现在的逻辑是:size=0 时允许 pBuf 为空(不做操作);size>0pBuf 必须非空。
  • 建议: 逻辑已非常完善,无需额外修改。

修改点 2: CPDF_Stream::WriteTo

修改内容:

-  if (size && !archive->WriteBlock(data.data(), size))
+  if (!archive->WriteBlock(data.data(), size))
     return false;

审查意见:

  • 一致性: 由于 WriteBlock 的实现(修改点 1)已经能够正确处理 size=0 的情况,调用方不再需要显式检查 size。这里的修改消除了冗余检查,使代码更简洁,并依赖于底层实现的正确性。
  • 建议: 逻辑正确,符合修改意图。

修改点 3: CFX_MemoryStream::WriteBlockAtOffset

修改内容:

-  if (!buffer || offset < 0 || !size)
+  if (offset < 0)
+    return false;
+
+  if (size == 0)
+    return true;
+
+  DCHECK(buffer);
+  if (!buffer)
     return false;

审查意见:

  • 逻辑改进: 与修改点 1 类似,将 size == 0 的处理前置,并允许 buffer 为空(当 size 为 0 时)。将 offset < 0 的检查独立出来。
  • 宏的使用: 这里使用了 DCHECK(buffer) 而不是 ASSERT。在 Chromium/PDFium 代码库中,DCHECK 通常用于 Debug 检查,而 Release 版本会忽略。紧接着的 if (!buffer) return false; 确保了 Release 版本的安全性。这种写法(Debug 断言 + Release 运行时检查)是 C++ 项目中处理不可变契约的良好实践。
  • 建议: 代码质量很高,逻辑清晰,兼顾了 Debug 时的错误发现和 Release 时的安全性。

修改点 4: IFX_WriteStream 接口注释

修改内容:

+  // When `size` is 0, treat it as a no-op and return true. That is also the
+  // only time when `pData` can be null.
   virtual bool WriteBlock(const void* pData, size_t size) = 0;

审查意见:

  • 文档规范: 这是一个关键的修改。通过在接口基类中添加注释,明确了所有派生类(如 CFX_FileBufferArchiveCFX_MemoryStream)必须遵守的契约:size=0 时是空操作且 pData 可为空。这有助于防止未来的开发者误用接口。
  • 建议: 注释非常准确,有助于代码维护。

2. 综合改进意见

  1. 代码质量

    • 优点:修改消除了未定义行为(Release 模式下的空指针访问),统一了不同流实现(文件流、内存流)对零长度写入的处理逻辑,符合“宽进严出”或防御性编程的原则。
    • 潜在问题:无。代码风格符合 Chromium/PDFium 的规范。
  2. 性能

    • 影响:微乎其微。增加了对 size == 0 的分支判断,但这通常是非常快的操作,且避免了在调用方(如 CPDF_Stream::WriteTo)进行重复检查。整体性能影响可以忽略不计,甚至可能因为减少了冗余检查而略微提升效率。
  3. 安全性

    • 提升:显著提升。主要修复了 Release 版本中可能存在的空指针解引用漏洞。将 ASSERT 转换为运行时检查,防止了恶意或意外输入导致的服务崩溃(DoS 风险)。

总结

这是一组高质量的代码修改。它不仅修复了潜在的 Release 版本崩溃问题,还通过接口文档明确了函数的行为契约,提高了代码的健壮性和可维护性。逻辑严密,符合 C++ 最佳实践,建议采纳。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: LiHua000, lzwind

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 e6bcd0e into linuxdeepin:master Mar 18, 2026
10 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