Bug 223707 - [GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBufferCache()
Summary: [GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBuffe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
: 221735 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-03-24 12:18 PDT by Peng Liu
Modified: 2021-03-30 14:24 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.49 KB, patch)
2021-03-24 13:36 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Revise the patch based on Eric's comments (10.89 KB, patch)
2021-03-24 14:46 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Revise the patch to fix an assertion failure (10.94 KB, patch)
2021-03-24 22:31 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (12.35 KB, patch)
2021-03-29 12:03 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (12.45 KB, patch)
2021-03-29 12:20 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2021-03-29 13:53 PDT, Peng Liu
youennf: review+
Details | Formatted Diff | Diff
Patch for landing (12.42 KB, patch)
2021-03-30 11:40 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2021-03-24 12:18:56 PDT
[GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBufferCache()
Comment 1 Peng Liu 2021-03-24 13:36:42 PDT
Created attachment 424176 [details]
Patch
Comment 2 Eric Carlson 2021-03-24 13:53:10 PDT
Comment on attachment 424176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424176&action=review

> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:163
> +    if (m_frameImages[index])

Won't this assert if index is out of bounds?

> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:183
> +            m_frameImages[index] = image;

Ditto

> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:188
> +    return m_frameImages[index];

Ditto.

> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:229
> +    for (size_t i = 0; i <= index; i++)
> +        m_frameImages[i] = nullptr;

Ditto.
Comment 3 Peng Liu 2021-03-24 14:02:58 PDT
Comment on attachment 424176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424176&action=review

>> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:163
>> +    if (m_frameImages[index])
> 
> Won't this assert if index is out of bounds?

Good catch!
I think I should add an assert(for debug) and check(for release) before this line. The same for other places.
Comment 4 Peng Liu 2021-03-24 14:46:21 PDT
Created attachment 424189 [details]
Revise the patch based on Eric's comments
Comment 5 Peng Liu 2021-03-24 22:31:02 PDT
Created attachment 424218 [details]
Revise the patch to fix an assertion failure
Comment 6 youenn fablet 2021-03-25 07:15:32 PDT
Comment on attachment 424218 [details]
Revise the patch to fix an assertion failure

View in context: https://bugs.webkit.org/attachment.cgi?id=424218&action=review

> Source/WebKit/GPUProcess/media/RemoteImageDecoderAVFProxy.cpp:137
> +void RemoteImageDecoderAVFProxy::clearFrameBufferCache(const ImageDecoderIdentifier& identifier, size_t index)

s/const ImageDecoderIdentifier&/ImageDecoderIdentifier/

> Source/WebKit/GPUProcess/media/RemoteImageDecoderAVFProxy.cpp:144
> +    imageDecoder->clearFrameBufferCache(index);

This does two queries to the map.

We could write it like this:
ASSERT(m_imageDecoders.contains(identifier));
if (auto* imageDecoder = m_imageDecoders.contains(identifier))
    imageDecoder->clearFrameBufferCache(index);

> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:246
> +    m_gpuProcessConnection->connection().send(Messages::RemoteImageDecoderAVFProxy::ClearFrameBufferCache(m_identifier, index), 0);

We are doing a check on index before sending the IPC message. A compromised WebProcess could still send the same IPC message with weird index values.
Is it safe to send a message to GPUProcess with index higher than m_frameImages.size()?
Comment 7 Peng Liu 2021-03-29 12:03:56 PDT
Created attachment 424560 [details]
Patch
Comment 8 Peng Liu 2021-03-29 12:20:13 PDT
Created attachment 424564 [details]
Patch
Comment 9 Peng Liu 2021-03-29 12:21:51 PDT
Comment on attachment 424218 [details]
Revise the patch to fix an assertion failure

View in context: https://bugs.webkit.org/attachment.cgi?id=424218&action=review

>> Source/WebKit/GPUProcess/media/RemoteImageDecoderAVFProxy.cpp:137
>> +void RemoteImageDecoderAVFProxy::clearFrameBufferCache(const ImageDecoderIdentifier& identifier, size_t index)
> 
> s/const ImageDecoderIdentifier&/ImageDecoderIdentifier/

Fixed this one as well as others in this class.

>> Source/WebKit/GPUProcess/media/RemoteImageDecoderAVFProxy.cpp:144
>> +    imageDecoder->clearFrameBufferCache(index);
> 
> This does two queries to the map.
> 
> We could write it like this:
> ASSERT(m_imageDecoders.contains(identifier));
> if (auto* imageDecoder = m_imageDecoders.contains(identifier))
>     imageDecoder->clearFrameBufferCache(index);

Good point! Fixed.

>> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:246
>> +    m_gpuProcessConnection->connection().send(Messages::RemoteImageDecoderAVFProxy::ClearFrameBufferCache(m_identifier, index), 0);
> 
> We are doing a check on index before sending the IPC message. A compromised WebProcess could still send the same IPC message with weird index values.
> Is it safe to send a message to GPUProcess with index higher than m_frameImages.size()?

It would be safe because of the implementation of `ImageDecoderAVFObjC::clearFrameBufferCache()`. But I added a protection in `RemoteImageDecoderAVFProxy::clearFrameBufferCache()` anyway.
Comment 10 Peng Liu 2021-03-29 13:21:42 PDT
*** Bug 221735 has been marked as a duplicate of this bug. ***
Comment 11 Peng Liu 2021-03-29 13:53:39 PDT
Created attachment 424575 [details]
Patch
Comment 12 youenn fablet 2021-03-30 00:14:01 PDT
Comment on attachment 424575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424575&action=review

> Source/WebKit/GPUProcess/media/RemoteImageDecoderAVFProxy.cpp:142
> +        imageDecoder->clearFrameBufferCache(index);

Could be a one-liner. ImageDecoderAVFObjC::clearFrameBufferCache implementation seems safe even if index is above frameCount.

> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:231
> +    for (size_t i = 0; i <= index; i++)

Could be a one liner, we also tend to prefer < instead of <=, and ++I as well.
for (size_t i = 0; i < std::min(index + 1, m_frameCount); ++i)
Comment 13 Peng Liu 2021-03-30 11:40:55 PDT
Created attachment 424665 [details]
Patch for landing
Comment 14 Peng Liu 2021-03-30 11:43:45 PDT
Comment on attachment 424575 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424575&action=review

>> Source/WebKit/GPUProcess/media/RemoteImageDecoderAVFProxy.cpp:142
>> +        imageDecoder->clearFrameBufferCache(index);
> 
> Could be a one-liner. ImageDecoderAVFObjC::clearFrameBufferCache implementation seems safe even if index is above frameCount.

Right. But it seems better to keep the protection, I think.

>> Source/WebKit/WebProcess/GPU/media/RemoteImageDecoderAVF.cpp:231
>> +    for (size_t i = 0; i <= index; i++)
> 
> Could be a one liner, we also tend to prefer < instead of <=, and ++I as well.
> for (size_t i = 0; i < std::min(index + 1, m_frameCount); ++i)

Fixed.
Comment 15 EWS 2021-03-30 14:23:27 PDT
Committed r275235: <https://commits.webkit.org/r275235>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424665 [details].
Comment 16 Radar WebKit Bug Importer 2021-03-30 14:24:14 PDT
<rdar://problem/76021876>