Correct uses of 'safeCast'
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 22 Sep 2016 01:23:26 +0000 (01:23 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 22 Sep 2016 01:23:26 +0000 (01:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162301
<rdar://problem/28343658>

Reviewed by Antti Koivisto.

Source/WebCore:

A number of integer calculations in BitmapImage and PDFDocumentImage
are not properly checked for overflow. Correct this.

Tested by fast/images/large-size-image-crash.html

* loader/cache/MemoryCache.cpp:
(WebCore::MemoryCache::adjustSize): RELEASE_ASSERT on overflow.
* platform/graphics/BitmapImage.cpp:
(WebCore::BitmapImage::destroyMetadataAndNotify):
(WebCore::BitmapImage::cacheFrame):
(WebCore::BitmapImage::didDecodeProperties):
(WebCore::BitmapImage::dataChanged):
(WebCore::BitmapImage::ensureFrameAtIndexIsCached):
(WebCore::BitmapImage::frameImageAtIndex):
* platform/graphics/BitmapImage.h:
* platform/graphics/cg/PDFDocumentImage.cpp:
(WebCore::PDFDocumentImage::decodedSizeChanged):
(WebCore::PDFDocumentImage::updateCachedImageIfNeeded):

Source/WTF:

* wtf/StdLibExtras.h:
(WTF::safeCast): RELEASE_ASSERT on overflow.

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

Source/WTF/ChangeLog
Source/WTF/wtf/StdLibExtras.h
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/MemoryCache.cpp
Source/WebCore/platform/graphics/BitmapImage.cpp
Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp

index eaab4b2..75ce973 100644 (file)
@@ -1,3 +1,14 @@
+2016-09-20  Brent Fulgham  <bfulgham@apple.com>
+
+        Correct uses of 'safeCast'
+        https://bugs.webkit.org/show_bug.cgi?id=162301
+        <rdar://problem/28343658>
+
+        Reviewed by Antti Koivisto.
+
+        * wtf/StdLibExtras.h:
+        (WTF::safeCast): RELEASE_ASSERT on overflow.
+
 2016-09-21  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r206222 and r206227.
index ebe30c9..6e8a214 100644 (file)
@@ -159,7 +159,7 @@ inline ToType bitwise_cast(FromType from)
 template<typename ToType, typename FromType>
 inline ToType safeCast(FromType value)
 {
-    ASSERT(isInBounds<ToType>(value));
+    RELEASE_ASSERT(isInBounds<ToType>(value));
     return static_cast<ToType>(value);
 }
 
index 6728603..368ca8c 100644 (file)
@@ -1,3 +1,30 @@
+2016-09-20  Brent Fulgham  <bfulgham@apple.com>
+
+        Correct uses of 'safeCast'
+        https://bugs.webkit.org/show_bug.cgi?id=162301
+        <rdar://problem/28343658>
+
+        Reviewed by Antti Koivisto.
+
+        A number of integer calculations in BitmapImage and PDFDocumentImage
+        are not properly checked for overflow. Correct this.
+
+        Tested by fast/images/large-size-image-crash.html
+
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::adjustSize): RELEASE_ASSERT on overflow.
+        * platform/graphics/BitmapImage.cpp:
+        (WebCore::BitmapImage::destroyMetadataAndNotify):
+        (WebCore::BitmapImage::cacheFrame):
+        (WebCore::BitmapImage::didDecodeProperties):
+        (WebCore::BitmapImage::dataChanged):
+        (WebCore::BitmapImage::ensureFrameAtIndexIsCached):
+        (WebCore::BitmapImage::frameImageAtIndex):
+        * platform/graphics/BitmapImage.h:
+        * platform/graphics/cg/PDFDocumentImage.cpp:
+        (WebCore::PDFDocumentImage::decodedSizeChanged):
+        (WebCore::PDFDocumentImage::updateCachedImageIfNeeded):
+
 2016-09-21  Chris Dumez  <cdumez@apple.com>
 
         Setting HTMLMeterElement's attributes to non-finite values throws wrong exception type
index 693501e..a078828 100644 (file)
@@ -644,10 +644,10 @@ void MemoryCache::removeFromLiveResourcesSize(CachedResource& resource)
 void MemoryCache::adjustSize(bool live, int delta)
 {
     if (live) {
-        ASSERT(delta >= 0 || ((int)m_liveSize + delta >= 0));
+        RELEASE_ASSERT(delta >= 0 || ((int)m_liveSize + delta >= 0));
         m_liveSize += delta;
     } else {
-        ASSERT(delta >= 0 || ((int)m_deadSize + delta >= 0));
+        RELEASE_ASSERT(delta >= 0 || ((int)m_deadSize + delta >= 0));
         m_deadSize += delta;
     }
 }
index 311294a..e27711f 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2006 Samuel Weinig (sam.weinig@gmail.com)
- * Copyright (C) 2004, 2005, 2006, 2008, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2006, 2008, 2015-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -36,6 +36,7 @@
 #include "MIMETypeRegistry.h"
 #include "TextStream.h"
 #include "Timer.h"
+#include <wtf/CheckedArithmetic.h>
 #include <wtf/CurrentTime.h>
 #include <wtf/Vector.h>
 #include <wtf/text/WTFString.h>
@@ -142,17 +143,20 @@ void BitmapImage::destroyMetadataAndNotify(unsigned frameBytesCleared, ClearedSo
     m_solidColor = Nullopt;
     invalidatePlatformData();
 
-    ASSERT(m_decodedSize >= frameBytesCleared);
-    m_decodedSize -= frameBytesCleared;
+    if (!WTF::safeSub(m_decodedSize, frameBytesCleared, m_decodedSize))
+        CRASH_WITH_SECURITY_IMPLICATION();
 
     // Clearing the ImageSource destroys the extra decoded data used for determining image properties.
+    long long adjustedFrameBytesCleared = frameBytesCleared;
     if (clearedSource == ClearedSource::Yes) {
-        frameBytesCleared += m_decodedPropertiesSize;
+        adjustedFrameBytesCleared += m_decodedPropertiesSize;
         m_decodedPropertiesSize = 0;
     }
 
-    if (frameBytesCleared && imageObserver())
-        imageObserver()->decodedSizeChanged(this, -safeCast<int>(frameBytesCleared));
+    if (adjustedFrameBytesCleared && imageObserver()) {
+        Checked<int> checkedDelta = adjustedFrameBytesCleared;
+        imageObserver()->decodedSizeChanged(this, -checkedDelta.unsafeGet());
+    }
 }
 
 void BitmapImage::cacheFrame(size_t index, SubsamplingLevel subsamplingLevel, ImageFrame::Caching caching)
@@ -172,14 +176,26 @@ void BitmapImage::cacheFrame(size_t index, SubsamplingLevel subsamplingLevel, Im
     LOG(Images, "BitmapImage %p cacheFrame %lu (%s%u bytes, complete %d)", this, index, caching == ImageFrame::Caching::Metadata ? "metadata only, " : "", m_frames[index].frameBytes(), m_frames[index].isComplete());
 
     if (m_frames[index].hasNativeImage()) {
-        int deltaBytes = safeCast<int>(m_frames[index].frameBytes());
-        m_decodedSize += deltaBytes;
+        if (!WTF::safeAdd(m_decodedSize, m_frames[index].frameBytes(), m_decodedSize)) {
+            LOG(Images, "BitmapImage %p cacheFrame m_decodedSize overflowed unsigned.", this);
+            destroyDecodedData(false);
+            return;
+        }
+
         // The fully-decoded frame will subsume the partially decoded data used
         // to determine image properties.
-        deltaBytes -= m_decodedPropertiesSize;
+        long long deltaBytes = m_frames[index].frameBytes() - m_decodedPropertiesSize;
         m_decodedPropertiesSize = 0;
+
+        Checked<int, RecordOverflow> checkedDeltaBytes = deltaBytes;
+        if (checkedDeltaBytes.hasOverflowed()) {
+            LOG(Images, "BitmapImage %p cacheFrame deltaBytes=%lld overflowed integer.", this, deltaBytes);
+            destroyDecodedData(false);
+            return;
+        }
+
         if (imageObserver())
-            imageObserver()->decodedSizeChanged(this, deltaBytes);
+            imageObserver()->decodedSizeChanged(this, checkedDeltaBytes.unsafeGet());
     }
 }
 
@@ -192,15 +208,17 @@ void BitmapImage::didDecodeProperties() const
     if (m_decodedPropertiesSize == updatedSize)
         return;
 
-    int deltaBytes = updatedSize - m_decodedPropertiesSize;
+    long long deltaBytes = updatedSize - m_decodedPropertiesSize;
 #if !ASSERT_DISABLED
     bool overflow = updatedSize > m_decodedPropertiesSize && deltaBytes < 0;
     bool underflow = updatedSize < m_decodedPropertiesSize && deltaBytes > 0;
     ASSERT(!overflow && !underflow);
 #endif
     m_decodedPropertiesSize = updatedSize;
-    if (imageObserver())
-        imageObserver()->decodedSizeChanged(this, deltaBytes);
+    if (imageObserver()) {
+        Checked<int> checkedDeltaBytes = deltaBytes;
+        imageObserver()->decodedSizeChanged(this, checkedDeltaBytes.unsafeGet());
+    }
 }
 
 void BitmapImage::updateSize() const
@@ -256,7 +274,7 @@ bool BitmapImage::dataChanged(bool allDataReceived)
     // start of the frame data), and any or none of them might be the particular
     // frame affected by appending new data here. Thus we have to clear all the
     // incomplete frames to be safe.
-    unsigned frameBytesCleared = 0;
+    Checked<unsigned> frameBytesCleared = 0;
     for (auto& frame : m_frames) {
         // NOTE: Don't call frameIsCompleteAtIndex() here, that will try to
         // decode any uncached (i.e. never-decoded or
@@ -264,10 +282,10 @@ bool BitmapImage::dataChanged(bool allDataReceived)
         if (frame.hasMetadata() && !frame.isComplete())
             frameBytesCleared += frame.clear();
     }
-    destroyMetadataAndNotify(frameBytesCleared, ClearedSource::No);
+    destroyMetadataAndNotify(frameBytesCleared.unsafeGet(), ClearedSource::No);
 #else
     // FIXME: why is this different for iOS?
-    int deltaBytes = 0;
+    Checked<int> deltaBytes = 0;
     if (!m_frames.isEmpty()) {
         if (int bytes = m_frames[m_frames.size() - 1].clear()) {
             deltaBytes += bytes;
@@ -275,7 +293,7 @@ bool BitmapImage::dataChanged(bool allDataReceived)
             m_decodedPropertiesSize = 0;
         }
     }
-    destroyMetadataAndNotify(deltaBytes, ClearedSource::No);
+    destroyMetadataAndNotify(deltaBytes.unsafeGet(), ClearedSource::No);
 #endif
     
     // Feed all the data we've seen so far to the image decoder.
@@ -356,11 +374,24 @@ NativeImagePtr BitmapImage::frameImageAtIndex(size_t index, float presentationSc
         LOG(Images, "  subsamplingLevel was %d, resampling", m_frames[index].subsamplingLevel());
 
         // If the image is already cached, but at too small a size, re-decode a larger version.
-        int sizeChange = -m_frames[index].clear();
+        unsigned sizeChange = m_frames[index].clear();
         invalidatePlatformData();
-        m_decodedSize += sizeChange;
+
+        if (WTF::safeSub(m_decodedSize, sizeChange, m_decodedSize)) {
+            LOG(Images, "BitmapImage %p frameImageAtIndex m_decodedSize overflowed unsigned.", this);
+            destroyDecodedData(false);
+            return nullptr;
+        }
+
+        Checked<int, RecordOverflow> checkedSizeChange = -sizeChange;
+        if (checkedSizeChange.hasOverflowed()) {
+            LOG(Images, "BitmapImage %p frameImageAtIndex sizeChange=%u overflowed integer.", this, -sizeChange);
+            destroyDecodedData(false);
+            return nullptr;
+        }
+
         if (imageObserver())
-            imageObserver()->decodedSizeChanged(this, sizeChange);
+            imageObserver()->decodedSizeChanged(this, checkedSizeChange.unsafeGet());
     }
 
     // If we haven't fetched a frame yet, do so.
index 80cb454..e8e6b10 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004, 2005, 2006, 2013 Apple Inc.  All rights reserved.
+ * Copyright (C) 2004-2016 Apple Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 #include "ImageObserver.h"
 #include "IntRect.h"
 #include "Length.h"
+#include "Logging.h"
 #include "SharedBuffer.h"
 #include "TextStream.h"
 #include <CoreGraphics/CGContext.h>
 #include <CoreGraphics/CGPDFDocument.h>
+#include <wtf/CheckedArithmetic.h>
 #include <wtf/MathExtras.h>
 #include <wtf/RAMSize.h>
 #include <wtf/RetainPtr.h>
@@ -181,13 +183,19 @@ void PDFDocumentImage::decodedSizeChanged(size_t newCachedBytes)
     if (!m_cachedBytes && !newCachedBytes)
         return;
 
+    long long deltaBytes = m_cachedBytes - newCachedBytes;
+
+    Checked<int> checkedDeltaBytes = deltaBytes;
     if (imageObserver())
-        imageObserver()->decodedSizeChanged(this, -safeCast<int>(m_cachedBytes) + newCachedBytes);
+        imageObserver()->decodedSizeChanged(this, -checkedDeltaBytes.unsafeGet());
 
     ASSERT(s_allDecodedDataSize >= m_cachedBytes);
     // Update with the difference in two steps to avoid unsigned underflow subtraction.
-    s_allDecodedDataSize -= m_cachedBytes;
-    s_allDecodedDataSize += newCachedBytes;
+    if (!WTF::safeSub(s_allDecodedDataSize, m_cachedBytes, s_allDecodedDataSize))
+        CRASH_WITH_SECURITY_IMPLICATION();
+
+    if (!WTF::safeAdd(s_allDecodedDataSize, newCachedBytes, s_allDecodedDataSize))
+        CRASH_WITH_SECURITY_IMPLICATION();
 
     m_cachedBytes = newCachedBytes;
 }
@@ -235,7 +243,17 @@ void PDFDocumentImage::updateCachedImageIfNeeded(GraphicsContext& context, const
     // Cache the PDF image only if the size of the new image won't exceed the cache threshold.
     if (m_pdfImageCachingPolicy == PDFImageCachingBelowMemoryLimit) {
         IntSize scaledSize = ImageBuffer::compatibleBufferSize(cachedImageSize, context);
-        if (s_allDecodedDataSize + safeCast<size_t>(scaledSize.width()) * scaledSize.height() * 4 - m_cachedBytes > s_maxDecodedDataSize) {
+        Checked<size_t, RecordOverflow> scaledBytes = scaledSize.area() * 4;
+
+        if (scaledBytes.hasOverflowed()) {
+            LOG(Images, "PDFDocumentImage %p updateCachedImageIfNeeded scaledBytes overflowed size_t.", this);
+            destroyDecodedData();
+            return;
+        }
+
+        Checked<size_t, RecordOverflow> potentialDecodedDataSize = s_allDecodedDataSize + scaledBytes - m_cachedBytes;
+        if (potentialDecodedDataSize.hasOverflowed() || potentialDecodedDataSize.unsafeGet() > s_maxDecodedDataSize) {
+            LOG(Images, "PDFDocumentImage %p updateCachedImageIfNeeded potentialDecodedDataSize overflowed size_t.", this);
             destroyDecodedData();
             return;
         }
@@ -259,7 +277,14 @@ void PDFDocumentImage::updateCachedImageIfNeeded(GraphicsContext& context, const
     m_cachedSourceRect = srcRect;
 
     IntSize internalSize = m_cachedImageBuffer->internalSize();
-    decodedSizeChanged(safeCast<size_t>(internalSize.width()) * internalSize.height() * 4);
+    Checked<size_t, RecordOverflow> scaledBytes = internalSize.area() * 4;
+    if (scaledBytes.hasOverflowed()) {
+        LOG(Images, "PDFDocumentImage %p updateCachedImageIfNeeded scaledBytes overflowed size_t.", this);
+        destroyDecodedData();
+        return;
+    }
+
+    decodedSizeChanged(scaledBytes.unsafeGet());
 }
 
 void PDFDocumentImage::draw(GraphicsContext& context, const FloatRect& dstRect, const FloatRect& srcRect, CompositeOperator op, BlendMode, ImageOrientationDescription)