[webkit-changes] [WebKit/WebKit] 1c283c: OOB crash under WebKit::dataProviderGetBytesAtPosi...

[Abrar Rahman Protyasha]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: 1c283c67a9c09fd482520eb7daffac5a0916da61
      https://github.com/WebKit/WebKit/commit/1c283c67a9c09fd482520eb7daffac5a0916da61
  Author: Abrar Rahman Protyasha <a_protyasha at apple.com>
  Date:   2025-01-31 (Fri, 31 Jan 2025)

  Changed paths:
    M Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h
    M Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm
    M Source/WebKit/WebProcess/Plugins/PDF/PDFPluginBase.h
    M Source/WebKit/WebProcess/Plugins/PDF/PDFPluginBase.mm

  Log Message:
  -----------
  OOB crash under WebKit::dataProviderGetBytesAtPositionCallback during off-main-thread incremental PDF loading
https://bugs.webkit.org/show_bug.cgi?id=284408
rdar://131110151

Reviewed by Simon Fraser.

We occasionally crash trying to memcpy a buffer for incremental loading
data provision. Here's a representative trace:

```
Thread 4 Crashed::   Dispatch queue: LinearizedPagePreload
0 _platform_memmove + 96
1 void WTF::memcpySpan<unsigned char, 18446744073709551615ul, unsigned char const, 18446744073709551615ul>(std::__1::span<unsigned char, 18446744073709551615ul>, std::__1::span<unsigned char const, 18446744073709551615ul>) + 16
2 WebKit::PDFIncrementalLoader::dataProviderGetBytesAtPosition(std::__1::span<unsigned char, 18446744073709551615ul>, long long) + 52
3 WebKit::dataProviderGetBytesAtPositionCallback(void*, void*, long long, unsigned long) + 308
4 provider_get_bytes_at_position + 84
5 CGDataProviderDirectGetBytesAtPositionInternal + 308
```

While we don't have a reproducible case yet, some analysis of the
incremental loading code suggests there is a small flaw in the threading
model for PDFPluginBase::dataSpanForRange() callers. That method secures
a lock to produce the data span, but if a load stream fails after a
caller gets the data span and before accessing said span, callers may
end up referencing null data.

This patch is a speculative fix for this issue. We teach
dataSpanForRange to accept a completion handler, which callers will
adopt as a substitute for the work they would have done with the data
span they expect to receive. The completion handler can then be called
while the data lock is still held.

This fix exposed an issue with the threading model for debug logging,
since our logging unconditionally jumps to the main thread and requests
to hold the data lock, thus deadlocking the web process. We address this
by securing a copy of m_streamedBytes before jumping across thread
boundaries -- see PDFPluginBase::streamedBytesForDebugLogging(). This
method skirts around thread safety analysis but ensures that the data
lock _is secured_ by the calling thread,  else asserting.

* Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.h:
* Source/WebKit/WebProcess/Plugins/PDF/PDFIncrementalLoader.mm:
(WebKit::ByteRangeRequest::completeUnconditionally):
(WebKit::PDFIncrementalLoader::dataSpanForRange const):
(WebKit::PDFIncrementalLoader::requestCompleteIfPossible):
(WebKit::PDFIncrementalLoader::dataProviderGetBytesAtPosition):
* Source/WebKit/WebProcess/Plugins/PDF/PDFPluginBase.h:
* Source/WebKit/WebProcess/Plugins/PDF/PDFPluginBase.mm:
(WebKit::PDFPluginBase::dataSpanForRange const):
(WebKit::PDFPluginBase::incrementalLoaderLog):
(WebKit::PDFPluginBase::incrementalLoaderLogWithBytes):

Originally-landed-as: 283286.578 at safari-7620-branch (de6e83ab1f4d). rdar://143592990
Canonical link: https://commits.webkit.org/289647@main



To unsubscribe from these emails, change your notification settings at https://github.com/WebKit/WebKit/settings/notifications