CRASH in CVPixelBufferGetBytePointerCallback()
authorjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Oct 2018 19:28:31 +0000 (19:28 +0000)
committerjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Oct 2018 19:28:31 +0000 (19:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190092

Reviewed by Eric Carlson.

Speculative fix for crash that occurs when callers of CVPixelBufferGetBytePointerCallback() attempt
to read the last byte of a CVPixelBuffer (as a pre-flight check) and crash due to a memory access
error. It's speculated that mismatching CVPixelBufferLockBytePointer / CVPixelBufferUnlockBytePointer
calls could result in an incorrect state inside the CVPixelBuffer. Add log count checks, locking, and
release logging to try to pinpoint if mismatch lock counts are occurring in this code path.

* platform/graphics/cv/PixelBufferConformerCV.cpp:
(WebCore::CVPixelBufferGetBytePointerCallback):
(WebCore::CVPixelBufferReleaseBytePointerCallback):
(WebCore::CVPixelBufferReleaseInfoCallback):
(WebCore::PixelBufferConformerCV::createImageFromPixelBuffer):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236806 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cv/PixelBufferConformerCV.cpp

index 64636c1..c7f4f1e 100644 (file)
@@ -1,3 +1,22 @@
+2018-10-03  Jer Noble  <jer.noble@apple.com>
+
+        CRASH in CVPixelBufferGetBytePointerCallback()
+        https://bugs.webkit.org/show_bug.cgi?id=190092
+
+        Reviewed by Eric Carlson.
+
+        Speculative fix for crash that occurs when callers of CVPixelBufferGetBytePointerCallback() attempt
+        to read the last byte of a CVPixelBuffer (as a pre-flight check) and crash due to a memory access
+        error. It's speculated that mismatching CVPixelBufferLockBytePointer / CVPixelBufferUnlockBytePointer
+        calls could result in an incorrect state inside the CVPixelBuffer. Add log count checks, locking, and
+        release logging to try to pinpoint if mismatch lock counts are occurring in this code path.
+
+        * platform/graphics/cv/PixelBufferConformerCV.cpp:
+        (WebCore::CVPixelBufferGetBytePointerCallback):
+        (WebCore::CVPixelBufferReleaseBytePointerCallback):
+        (WebCore::CVPixelBufferReleaseInfoCallback):
+        (WebCore::PixelBufferConformerCV::createImageFromPixelBuffer):
+
 2018-10-03  Chris Dumez  <cdumez@apple.com>
 
         Regression(r236779): Crash when changing the input element type from inside an 'input' event listener
index e47dc5b..e7e11c7 100644 (file)
@@ -29,6 +29,7 @@
 #if HAVE(CORE_VIDEO)
 
 #include "GraphicsContextCG.h"
+#include "Logging.h"
 #include <wtf/SoftLinking.h>
 
 #include "CoreVideoSoftLink.h"
@@ -55,23 +56,87 @@ PixelBufferConformerCV::PixelBufferConformerCV(CFDictionaryRef attributes)
 #endif
 }
 
-static const void* CVPixelBufferGetBytePointerCallback(void* info)
+struct CVPixelBufferInfo {
+    RetainPtr<CVPixelBufferRef> pixelBuffer;
+    int lockCount { 0 };
+};
+
+static const void* CVPixelBufferGetBytePointerCallback(void* refcon)
 {
-    CVPixelBufferRef pixelBuffer = static_cast<CVPixelBufferRef>(info);
-    CVPixelBufferLockBaseAddress(pixelBuffer, kCVPixelBufferLock_ReadOnly);
-    return CVPixelBufferGetBaseAddress(pixelBuffer);
+    ASSERT(refcon);
+    if (!refcon) {
+        RELEASE_LOG_ERROR(Media, "CVPixelBufferGetBytePointerCallback() called with NULL refcon");
+        RELEASE_LOG_STACKTRACE(Media);
+        return nullptr;
+    }
+    auto info = static_cast<CVPixelBufferInfo*>(refcon);
+
+    CVReturn result = CVPixelBufferLockBaseAddress(info->pixelBuffer.get(), kCVPixelBufferLock_ReadOnly);
+
+    ASSERT(result == kCVReturnSuccess);
+    if (result != kCVReturnSuccess) {
+        RELEASE_LOG_ERROR(Media, "CVPixelBufferLockBaseAddress() returned error code %d", result);
+        RELEASE_LOG_STACKTRACE(Media);
+        return nullptr;
+    }
+
+    ++info->lockCount;
+    void* address = CVPixelBufferGetBaseAddress(info->pixelBuffer.get());
+    RELEASE_LOG_INFO(Media, "CVPixelBufferGetBytePointerCallback() returning bytePointer: %p, size: %zu", address, CVPixelBufferGetDataSize(info->pixelBuffer.get()));
+    return address;
 }
 
-static void CVPixelBufferReleaseBytePointerCallback(void* info, const void*)
+static void CVPixelBufferReleaseBytePointerCallback(void* refcon, const void*)
 {
-    CVPixelBufferRef pixelBuffer = static_cast<CVPixelBufferRef>(info);
-    CVPixelBufferUnlockBaseAddress(pixelBuffer, kCVPixelBufferLock_ReadOnly);
+    ASSERT(refcon);
+    if (!refcon) {
+        RELEASE_LOG_ERROR(Media, "CVPixelBufferReleaseBytePointerCallback() called with NULL refcon");
+        RELEASE_LOG_STACKTRACE(Media);
+        return;
+    }
+    auto info = static_cast<CVPixelBufferInfo*>(refcon);
+
+    CVReturn result = CVPixelBufferUnlockBaseAddress(info->pixelBuffer.get(), kCVPixelBufferLock_ReadOnly);
+    ASSERT(result == kCVReturnSuccess);
+    if (result != kCVReturnSuccess) {
+        RELEASE_LOG_ERROR(Media, "CVPixelBufferLockBaseAddress() returned error code %d", result);
+        RELEASE_LOG_STACKTRACE(Media);
+        return;
+    }
+
+    ASSERT(info->lockCount);
+    if (!info->lockCount) {
+        RELEASE_LOG_ERROR(Media, "CVPixelBufferReleaseBytePointerCallback() called without matching CVPixelBufferGetBytePointerCallback()");
+        RELEASE_LOG_STACKTRACE(Media);
+    }
+    --info->lockCount;
 }
 
-static void CVPixelBufferReleaseInfoCallback(void* info)
+static void CVPixelBufferReleaseInfoCallback(void* refcon)
 {
-    CVPixelBufferRef pixelBuffer = static_cast<CVPixelBufferRef>(info);
-    CFRelease(pixelBuffer);
+    ASSERT(refcon);
+    if (!refcon) {
+        RELEASE_LOG_ERROR(Media, "CVPixelBufferReleaseInfoCallback() called with NULL refcon");
+        RELEASE_LOG_STACKTRACE(Media);
+        return;
+    }
+    auto info = static_cast<CVPixelBufferInfo*>(refcon);
+
+    ASSERT(!info->lockCount);
+    if (info->lockCount) {
+        RELEASE_LOG_ERROR(Media, "CVPixelBufferReleaseInfoCallback() called with a non-zero lockCount: %d", info->lockCount);
+        RELEASE_LOG_STACKTRACE(Media);
+
+        CVReturn result = CVPixelBufferUnlockBaseAddress(info->pixelBuffer.get(), kCVPixelBufferLock_ReadOnly);
+        ASSERT(result == kCVReturnSuccess);
+        if (result != kCVReturnSuccess) {
+            RELEASE_LOG_ERROR(Media, "CVPixelBufferLockBaseAddress() returned error code %d", result);
+            RELEASE_LOG_STACKTRACE(Media);
+        }
+    }
+
+    info->pixelBuffer = nullptr;
+    delete info;
 }
 
 RetainPtr<CVPixelBufferRef> PixelBufferConformerCV::convert(CVPixelBufferRef rawBuffer)
@@ -112,9 +177,12 @@ RetainPtr<CGImageRef> PixelBufferConformerCV::createImageFromPixelBuffer(CVPixel
     size_t bytesPerRow = CVPixelBufferGetBytesPerRow(buffer.get());
     size_t byteLength = CVPixelBufferGetDataSize(buffer.get());
 
-    CFRetain(buffer.get()); // Balanced by CVPixelBufferReleaseInfoCallback in providerCallbacks.
+    CVPixelBufferInfo* info = new CVPixelBufferInfo();
+    info->pixelBuffer = WTFMove(buffer);
+    info->lockCount = 0;
+
     CGDataProviderDirectCallbacks providerCallbacks = { 0, CVPixelBufferGetBytePointerCallback, CVPixelBufferReleaseBytePointerCallback, 0, CVPixelBufferReleaseInfoCallback };
-    RetainPtr<CGDataProviderRef> provider = adoptCF(CGDataProviderCreateDirect(buffer.get(), byteLength, &providerCallbacks));
+    RetainPtr<CGDataProviderRef> provider = adoptCF(CGDataProviderCreateDirect(info, byteLength, &providerCallbacks));
 
     return adoptCF(CGImageCreate(width, height, 8, 32, bytesPerRow, sRGBColorSpaceRef(), bitmapInfo, provider.get(), nullptr, false, kCGRenderingIntentDefault));
 }