Bug 171614 – When the image decoding thread makes a callOnMainThread(), ensure all the objects it needs are protected

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

This is the scenario of the crash:

1. The BitmapImage::draw() in the main thread requests decoding an image frame through ImageFrameCache::requestFrameAsyncDecodingAtIndex()
2. The decoding thread in ImageFrameCache::startAsyncDecodingQueue() starts decoding the image frame.
3. The main thread deletes the BitmapImage. This means the ImageFrameCache will be dereferenced but will not deleted since the decoding thread has a protecting copy.
4. The decoding thread still can proceed since all the data members are captured including the ImageFrameCache.
5. The decoding thread pushes a request on the main thread to cache the decoded frame.
6. The decoding thread terminates since ImageFrameCache::stopAsyncDecodingQueue() was called when the BitmapImage was deleted. ImageFrameCache is dereferenced the second time. And in this case, it is deleted.
7. The call on the main thread to ImageFrameCache::cacheFrameNativeImageAtIndex() is now acting on a deleted objet.

The fix is to protect this and protectedDecoder as well by making callOnMainThread() in the decoding thread capture them. We do the same thing for protectedQueue.

[Said Abou-Hallawa]

<rdar://problem/31538943>

[Said Abou-Hallawa]

Created attachment 308944 [details]
Patch

[Build Bot]

Attachment 308944 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:3:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use after free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 308944 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:295
> +            callOnMainThread([this, protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage, frameRequest] () mutable {

Why not just pass a  RefPtr<ImageFrameCache> protectedThis(this) to the callOnMainThread?

[Said Abou-Hallawa]

Comment on attachment 308944 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:295
>> +            callOnMainThread([this, protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage, frameRequest] () mutable {
> 
> Why not just pass a  RefPtr<ImageFrameCache> protectedThis(this) to the callOnMainThread?

Because m_decodingQueue can be set to nullptr in ImageFrameCache::stopAsyncDecodingQueue() and m_decoder can be changed to another ImageDecoder in ImageFrameCache::setDecoder(). So protecting ImageFrameCache does not protect the members which the decoding thread starts with.

[Said Abou-Hallawa]

Created attachment 309959 [details]
Patch

[Said Abou-Hallawa]

The difference between the two patches is the second one makes all the pointers, which are protected by the decoding thread, be ThreadSafeRefCounted.

[David Kilzer (:ddkilzer)]

Comment on attachment 309959 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        The asynchronous image decoding was design to not block the main thread if

Typo:  "design" => "designed"

> Source/WebCore/ChangeLog:18
> +        This might lead to two kinds of crashes:
> +        1. A SIG fault inside the ImageDecoder trying to access one of its member
> +        2. A SIG fault inside the ImageFrameCache trying to access one of its frames

Instead of "SIG fault" I'd use "segfault" (or SIGSEGV).

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:298
>              // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
> -            callOnMainThread([this, protectedQueue = protectedQueue.copyRef(), nativeImage, frameRequest] () mutable {
> +            callOnMainThread([this, protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage, frameRequest] () mutable {
>                  // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
>                  if (protectedQueue.ptr() == m_decodingQueue) {
>                      ASSERT(m_frameCommitQueue.first() == frameRequest);

I don't think adding the protectedThings to the lambda is enough.  You need to actually use them in the code inside the lambda, maybe something like this (UNTESTED):

                // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
                if (protectedQueue.ptr() == protectedThis->m_decodingQueue) {
                    ASSERT(protectedThis->m_frameCommitQueue.first() == frameRequest);
                    protectedThis->m_frameCommitQueue.removeFirst();
                    protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
                } else
                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, protectedThis, sourceURL().string().utf8().data(), frameRequest.index);

Where is protectedDecoder used here?  If it's not called by this block of code, it probably isn't needed.  If it is called from this block of code, it may need to be passed through (via function arguments) to where it's used.

[Said Abou-Hallawa]

Created attachment 310311 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 309959 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        The asynchronous image decoding was design to not block the main thread if
> 
> Typo:  "design" => "designed"

Fixed.

>> Source/WebCore/ChangeLog:18
>> +        2. A SIG fault inside the ImageFrameCache trying to access one of its frames
> 
> Instead of "SIG fault" I'd use "segfault" (or SIGSEGV).

segfault was used.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:298
>>                      ASSERT(m_frameCommitQueue.first() == frameRequest);
> 
> I don't think adding the protectedThings to the lambda is enough.  You need to actually use them in the code inside the lambda, maybe something like this (UNTESTED):
> 
>                 // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
>                 if (protectedQueue.ptr() == protectedThis->m_decodingQueue) {
>                     ASSERT(protectedThis->m_frameCommitQueue.first() == frameRequest);
>                     protectedThis->m_frameCommitQueue.removeFirst();
>                     protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
>                 } else
>                     LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, protectedThis, sourceURL().string().utf8().data(), frameRequest.index);
> 
> Where is protectedDecoder used here?  If it's not called by this block of code, it probably isn't needed.  If it is called from this block of code, it may need to be passed through (via function arguments) to where it's used.

You are right. I changed the code above to be like what you suggested. The decoder is used in cacheMetadataAtIndex() which is called from cacheNativeImageAtIndex() which is called from cacheNativeImageAtIndexAsync().

Instead of passing protectedDecoder all the way from this lambda function to cacheMetadataAtIndex(), I changed the above if-statment to be

if (protectedQueue.ptr() == protectedThis->m_decodingQueue && protectedDecoder.ptr() == protectedThis->m_decoder)

Adding protectedDecoder.ptr() == protectedThis->m_decoder ensures that, the decoder we start the thread with is the one we are going to use in cacheMetadataAtIndex(). This check is safe since this code runs on the main thread and m_decoder can be changed only from the main thread. When the decoder changes in ImageFrameCache::setDecoder() the decoding thread stops which means that decoding the current frame on the decoding thread and caching its metadata on the main thread is not needed and can be thrown away.

[David Kilzer (:ddkilzer)]

Comment on attachment 310311 [details]
Patch

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

r=me with two fixes for removing 'this' from the lambda.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:298
> +            callOnMainThread([this, protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage, frameRequest] () mutable {

Don't need to capture 'this' since you have 'protectedThis'.  (See below.)

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:299
>                  // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called

Nit:  Period at end of comment.

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:305
> +                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, this, protectedThis->sourceURL().string().utf8().data(), frameRequest.index);

Replace 'this' with 'protectedThis->get()' here.

[Said Abou-Hallawa]

Created attachment 310408 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 310311 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:298
>> +            callOnMainThread([this, protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage, frameRequest] () mutable {
> 
> Don't need to capture 'this' since you have 'protectedThis'.  (See below.)

Yes this is correct. I removed capturing 'this' from the decoding thread and from the lambda of the callOnMainThread(). This will ensure we only use 'protectedThis' in both of them.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:299
>>                  // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
> 
> Nit:  Period at end of comment.

Fixed.

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:305
>> +                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, this, protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
> 
> Replace 'this' with 'protectedThis->get()' here.

Fixed.

[David Kilzer (:ddkilzer)]

Comment on attachment 310408 [details]
Patch

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

r=me if there are no issues with the WTFMove(nativeImage) inside the callOnMainThread().

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:303
> +            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage, frameRequest] () mutable {
> +                // The queue may have been closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called.
> +                if (protectedQueue.ptr() == protectedThis->m_decodingQueue && protectedDecoder.ptr() == protectedThis->m_decoder) {
> +                    ASSERT(protectedThis->m_frameCommitQueue.first() == frameRequest);
> +                    protectedThis->m_frameCommitQueue.removeFirst();
> +                    protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);

Is there a difference between using WTFMove(nativeImage) in the lambda (line 298) versus in the cacheNativeImageAtIndexAsync() call (line 303)?

I don't know WTFMove() well enough to reason about that, but everything else looks good.

[Said Abou-Hallawa]

Created attachment 310430 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 310430 [details]
Patch

r=me  Looks good!

[Said Abou-Hallawa]

Comment on attachment 310408 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageFrameCache.cpp:303
>> +                    protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
> 
> Is there a difference between using WTFMove(nativeImage) in the lambda (line 298) versus in the cacheNativeImageAtIndexAsync() call (line 303)?
> 
> I don't know WTFMove() well enough to reason about that, but everything else looks good.

This is a good point. Actually using WTFMove(nativeImage) is better. Given that the retain_count of nativeImage = 1 before callOnMainThread(), here is the difference:

-- For callOnMainThread([..., nativeImage, ...]() { ... }); the compiler generates the following implicit calls:
  - The copy constructor RetainPtr(PtrType ptr) is called which will call CFRetain(). So retain_count of nativeImage = 2.
  - The move constructor RetainPtr(RetainPtr&& o) is called to move the copied object to the argument object. So no change in retain_count.
  - After callOnMainThread() finishes, the destructor ~RetainPtr() is called for copied object. But nothing happens since this object was moved.
  - When the loop finishes, nativeImage goes out of scope so the destructor ~RetainPtr() is called for nativeImage which will call CFRelease(). So retain_count of nativeImage = 1.
  - The retain_count of nativeImage = 1 when the lambda function is dispatched. When calling cacheNativeImageAtIndexAsync(), nativeImage will be moved .nativeImage will be moved again when it is cached in the ImageFrame.

-- For callOnMainThread([..., nativeImage = WTFMove(nativeImage), ...]() { ... }); the compilerr generates the following implicit calls:
  - The move constructor RetainPtr(RetainPtr&& o) is called to move nativeImage. So retain_count of nativeImage = 1.
  - The move constructor RetainPtr(RetainPtr&& o) is called to move the first moved object to the argument object. So no change in retain_count.
  - After callOnMainThread() finishes, the destructor ~RetainPtr() is called for the first moved object. But nothing happens to retain_count since the object was moved.
  - When the loop finishes, nativeImage goes out of scope so the destructor ~RetainPtr() is called for nativeImage But nothing happens because it was moved.
  - The retain_count of nativeImage = 1 when the lambda function is dispatched. When calling cacheNativeImageAtIndexAsync(), nativeImage will be moved. nativeImage will be moved again when it is cached in the ImageFrame.

So using WTFMove() will make nativeImage be passed without any ref counting churns.

[WebKit Commit Bot]

Comment on attachment 310430 [details]
Patch

Clearing flags on attachment: 310430

Committed r216998: <http://trac.webkit.org/changeset/216998>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tobias Netzel]

Comment on attachment 310430 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageFrameCache.cpp:309
> -        while (m_frameRequestQueue.dequeue(frameRequest)) {
> +        while (protectedThis->m_frameRequestQueue.dequeue(frameRequest)) {
>              TraceScope tracingScope(AsyncImageDecodeStart, AsyncImageDecodeEnd);
>  
>              // Get the frame NativeImage on the decoding thread.
>              NativeImagePtr nativeImage = protectedDecoder->createFrameImageAtIndex(frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions);
>              if (nativeImage)
> -                LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld has been decoded]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
> -            else
> -                LOG(Images, "ImageFrameCache::%s - %p - url: %s [decoding for frame %ld has failed]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
> +                LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld has been decoded]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
> +            else {
> +                LOG(Images, "ImageFrameCache::%s - %p - url: %s [decoding for frame %ld has failed]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
> +                continue;
> +            }
>  
>              // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
> -            callOnMainThread([this, protectedQueue = protectedQueue.copyRef(), nativeImage, frameRequest] () mutable {
> -                // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
> -                if (protectedQueue.ptr() == m_decodingQueue) {
> -                    ASSERT(m_frameCommitQueue.first() == frameRequest);
> -                    m_frameCommitQueue.removeFirst();
> -                    cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
> +            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
> +                // The queue may have been closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called.
> +                if (protectedQueue.ptr() == protectedThis->m_decodingQueue && protectedDecoder.ptr() == protectedThis->m_decoder) {
> +                    ASSERT(protectedThis->m_frameCommitQueue.first() == frameRequest);
> +                    protectedThis->m_frameCommitQueue.removeFirst();
> +                    protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
>                  } else
> -                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
> +                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
>              });
>          }

Continuing the loop in line 296 causes the assertion in line 303 to fail in my port of WebKit where nativeImage can be null sometimes.
I think you have to remove the request from m_frameCommitQueue when continuing the loop here - at least that made it work correctly for me.

[David Kilzer (:ddkilzer)]

(In reply to Tobias Netzel from comment #20)
> Comment on attachment 310430 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310430&action=review
> 
> > Source/WebCore/platform/graphics/ImageFrameCache.cpp:309
> > -        while (m_frameRequestQueue.dequeue(frameRequest)) {
> > +        while (protectedThis->m_frameRequestQueue.dequeue(frameRequest)) {
> >              TraceScope tracingScope(AsyncImageDecodeStart, AsyncImageDecodeEnd);
> >  
> >              // Get the frame NativeImage on the decoding thread.
> >              NativeImagePtr nativeImage = protectedDecoder->createFrameImageAtIndex(frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions);
> >              if (nativeImage)
> > -                LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld has been decoded]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
> > -            else
> > -                LOG(Images, "ImageFrameCache::%s - %p - url: %s [decoding for frame %ld has failed]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
> > +                LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld has been decoded]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
> > +            else {
> > +                LOG(Images, "ImageFrameCache::%s - %p - url: %s [decoding for frame %ld has failed]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
> > +                continue;
> > +            }
> >  
> >              // Update the cached frames on the main thread to avoid updating the MemoryCache from a different thread.
> > -            callOnMainThread([this, protectedQueue = protectedQueue.copyRef(), nativeImage, frameRequest] () mutable {
> > -                // The queue may be closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called
> > -                if (protectedQueue.ptr() == m_decodingQueue) {
> > -                    ASSERT(m_frameCommitQueue.first() == frameRequest);
> > -                    m_frameCommitQueue.removeFirst();
> > -                    cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
> > +            callOnMainThread([protectedThis = protectedThis.copyRef(), protectedQueue = protectedQueue.copyRef(), protectedDecoder = protectedDecoder.copyRef(), nativeImage = WTFMove(nativeImage), frameRequest] () mutable {
> > +                // The queue may have been closed if after we got the frame NativeImage, stopAsyncDecodingQueue() was called.
> > +                if (protectedQueue.ptr() == protectedThis->m_decodingQueue && protectedDecoder.ptr() == protectedThis->m_decoder) {
> > +                    ASSERT(protectedThis->m_frameCommitQueue.first() == frameRequest);
> > +                    protectedThis->m_frameCommitQueue.removeFirst();
> > +                    protectedThis->cacheNativeImageAtIndexAsync(WTFMove(nativeImage), frameRequest.index, frameRequest.subsamplingLevel, frameRequest.decodingOptions, frameRequest.decodingStatus);
> >                  } else
> > -                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, this, sourceURL().string().utf8().data(), frameRequest.index);
> > +                    LOG(Images, "ImageFrameCache::%s - %p - url: %s [frame %ld will not cached]", __FUNCTION__, protectedThis.ptr(), protectedThis->sourceURL().string().utf8().data(), frameRequest.index);
> >              });
> >          }
> 
> Continuing the loop in line 296 causes the assertion in line 303 to fail in
> my port of WebKit where nativeImage can be null sometimes.
> I think you have to remove the request from m_frameCommitQueue when
> continuing the loop here - at least that made it work correctly for me.

Can you file a new bug for this issue, Tobias?

[Said Abou-Hallawa]

https://bugs.webkit.org/show_bug.cgi?id=181391 was filed to address https://bugs.webkit.org/show_bug.cgi?id=171614#c20