Bug 223707 – [GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBufferCache()

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

[GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBufferCache()

[Peng Liu]

Created attachment 424176 [details]
Patch

[Eric Carlson]

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.

[Peng Liu]

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.

[Peng Liu]

Created attachment 424189 [details]
Revise the patch based on Eric's comments

[Peng Liu]

Created attachment 424218 [details]
Revise the patch to fix an assertion failure

[youenn fablet]

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()?

[Peng Liu]

Created attachment 424560 [details]
Patch

[Peng Liu]

Created attachment 424564 [details]
Patch

[Peng Liu]

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.

[Peng Liu]

*** Bug 221735 has been marked as a duplicate of this bug. ***

[Peng Liu]

Created attachment 424575 [details]
Patch

[youenn fablet]

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)

[Peng Liu]

Created attachment 424665 [details]
Patch for landing

[Peng Liu]

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.

[EWS]

Committed r275235: <https://commits.webkit.org/r275235>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424665 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76021876>