WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
223707
[GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBufferCache()
https://bugs.webkit.org/show_bug.cgi?id=223707
Summary
[GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBuffe...
Peng Liu
Reported
2021-03-24 12:18:56 PDT
[GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBufferCache()
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Peng Liu
Comment 1
2021-03-24 13:36:42 PDT
Created
attachment 424176
[details]
Patch
Eric Carlson
Comment 2
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.
Peng Liu
Comment 3
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.
Peng Liu
Comment 4
2021-03-24 14:46:21 PDT
Created
attachment 424189
[details]
Revise the patch based on Eric's comments
Peng Liu
Comment 5
2021-03-24 22:31:02 PDT
Created
attachment 424218
[details]
Revise the patch to fix an assertion failure
youenn fablet
Comment 6
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()?
Peng Liu
Comment 7
2021-03-29 12:03:56 PDT
Created
attachment 424560
[details]
Patch
Peng Liu
Comment 8
2021-03-29 12:20:13 PDT
Created
attachment 424564
[details]
Patch
Peng Liu
Comment 9
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.
Peng Liu
Comment 10
2021-03-29 13:21:42 PDT
***
Bug 221735
has been marked as a duplicate of this bug. ***
Peng Liu
Comment 11
2021-03-29 13:53:39 PDT
Created
attachment 424575
[details]
Patch
youenn fablet
Comment 12
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)
Peng Liu
Comment 13
2021-03-30 11:40:55 PDT
Created
attachment 424665
[details]
Patch for landing
Peng Liu
Comment 14
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.
EWS
Comment 15
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]
.
Radar WebKit Bug Importer
Comment 16
2021-03-30 14:24:14 PDT
<
rdar://problem/76021876
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug