Skip to content

fix(pdfium): resolve buffer overflow in PDF link handling#269

Merged
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
add-uos:fix-348865-fix-pdf-link-buffer-overflow
May 6, 2026
Merged

fix(pdfium): resolve buffer overflow in PDF link handling#269
deepin-bot[bot] merged 1 commit intolinuxdeepin:masterfrom
add-uos:fix-348865-fix-pdf-link-buffer-overflow

Conversation

@add-uos
Copy link
Copy Markdown
Contributor

@add-uos add-uos commented Apr 30, 2026

Fix potential buffer overflow when extracting PDF URI and file paths by using dynamic buffer allocation instead of fixed-size arrays.

修复 PDF 链接处理中的缓冲区溢出问题,使用动态缓冲区分配替代固定大小数组。

Log: 修复 PDF 链接缓冲区溢出
PMS: BUG-348865
Influence: 修复后 PDF 文档中的 URI 和文件路径链接能够正确处理,避免缓冲区溢出导致的安全隐患和程序崩溃,提升 PDF 阅读器的稳定性。

Fix potential buffer overflow when extracting PDF URI and file paths
by using dynamic buffer allocation instead of fixed-size arrays.

修复 PDF 链接处理中的缓冲区溢出问题,使用动态缓冲区分配替代固定大小数组。

Log: 修复 PDF 链接缓冲区溢出
PMS: BUG-348865
Influence: 修复后 PDF 文档中的 URI 和文件路径链接能够正确处理,避免缓冲区溢出导致的安全隐患和程序崩溃,提升 PDF 阅读器的稳定性。
@deepin-ci-robot
Copy link
Copy Markdown

deepin pr auto review

这段代码修改主要针对 PDFium 库中获取 URI 和文件路径的逻辑进行了改进。以下是对这段 diff 的详细审查意见:

1. 语法逻辑

  • 审查结果:通过
  • 分析
    • 代码逻辑正确。原代码使用了固定大小的栈缓冲区(char uri[256]),这存在截断风险。新代码采用了标准的两步获取策略:第一次调用传入 nullptr0 获取所需的缓冲区大小,第二次调用传入分配好的缓冲区获取实际数据。这是处理 C 风格可变长度字符串接口的正确做法。
    • QByteArray 的初始化和 data() 的使用符合 Qt 规范。

2. 代码质量

  • 审查结果:良好,但有改进空间
  • 分析
    • 优点:消除了固定缓冲区大小限制,避免了长 URL 或路径被截断的问题,增强了程序的健壮性。
    • 拼写修正:原代码中的变量名 lenth(拼写错误)被修正为 length,提高了代码可读性。
    • 编码处理:显式使用 QString::fromUtf8() 将获取到的字节数组转换为字符串,比隐式转换更安全、更明确。
    • 建议:虽然代码逻辑正确,但 QByteArray 初始化时填充了 0 (QByteArray(length, 0)),而 FPDFAction_GetURIPath 等函数通常会写入字符串并以 \0 结尾。虽然这不会导致错误,但略显多余。不过考虑到安全性,保持现状是可以接受的。

3. 代码性能

  • 审查结果:通过
  • 分析
    • 新代码将原本的一次函数调用变成了两次(一次获取长度,一次获取数据)。这确实增加了一次跨库调用的开销。
    • 然而,对于 PDF 注解加载这种非高频、非实时渲染路径的操作,这点性能损耗完全可以忽略不计。
    • 相比之下,固定栈分配(旧代码)虽然分配极快,但存在缓冲区溢出的严重隐患。堆分配(新代码)虽然稍慢,但换取了安全性,是值得的。

4. 代码安全

  • 审查结果:显著提升
  • 分析
    • 缓冲区溢出修复:这是本次修改最大的安全改进。旧代码使用 char uri[256],如果 PDF 文件中的 URI 长度超过 255 字节,就会导致栈溢出,可能被恶意利用。新代码动态分配所需大小,彻底解决了这个问题。
    • 字符串截断:旧代码可能导致 URL 或路径被截断,导致链接无法打开。新代码保证了数据的完整性。
    • 编码安全:使用 QString::fromUtf8 明确了编码格式,避免了不同系统默认编码不一致可能导致的乱码或转换错误。

总结与改进建议

这段代码修改是非常高质量的,主要解决了缓冲区溢出的安全隐患,值得肯定。

进一步改进建议(可选):

  1. 检查返回值FPDFAction_GetURIPath 的第二次调用返回值(实际复制的长度)目前被忽略了。虽然理论上第一次调用已经分配了足够空间,但为了防御性编程,可以检查第二次调用的返回值是否等于 length,或者简单地检查 uriBuffer 是否为空。
    • 示例代码片段:
      // ...
      unsigned long actualLength = FPDFAction_GetURIPath(m_doc, action, uriBuffer.data(), length);
      if (actualLength > 0) {
          // 确保字符串以 \0 结尾(PDFium 通常会处理,但为了安全可以手动截断)
          uriBuffer.truncate(actualLength); 
          dAnnot->setUrl(QString::fromUtf8(uriBuffer.constData()));
      }
      // ...
  2. 边界检查:虽然 PDFium 返回 length 时通常包含结束符 \0,但 QByteArray(length, 0) 分配的大小正好是 length。如果 FPDFAction_GetURIPath 返回的长度不包含 \0,可能会导致越界写入。查阅 PDFium 文档确认其行为(通常是包含 \0 的字节数)。如果为了绝对安全,可以分配 length + 1 的大小,或者使用 QByteArrayresize 方法。

总体评价:代码修改正确且必要,显著提升了安全性和健壮性,建议合并。

@deepin-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: add-uos, wyu71

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

@add-uos
Copy link
Copy Markdown
Contributor Author

add-uos commented May 6, 2026

/merge

@deepin-bot deepin-bot Bot merged commit 8c9ff3e into linuxdeepin:master May 6, 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