[GPUP] Add an IPC message to implement RemoteImageDecoderAVF::clearFrameBufferCache()
Created attachment 424176 [details] Patch
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 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.
Created attachment 424189 [details] Revise the patch based on Eric's comments
Created attachment 424218 [details] Revise the patch to fix an assertion failure
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()?
Created attachment 424560 [details] Patch
Created attachment 424564 [details] Patch
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.
*** Bug 221735 has been marked as a duplicate of this bug. ***
Created attachment 424575 [details] Patch
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)
Created attachment 424665 [details] Patch for landing
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.
Committed r275235: <https://commits.webkit.org/r275235> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424665 [details].
<rdar://problem/76021876>