Change the MemoryCache and CachedResource adjustSize functions to take a long argument
authorsaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Sep 2016 16:01:36 +0000 (16:01 +0000)
committersaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 30 Sep 2016 16:01:36 +0000 (16:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=162708
<rdar://problem/28555702>

Reviewed by Brent Fulgham.

Source/WebCore:

Because the MemoryCache stores the size of the cached memory in unsigned,
two problems my happen when reporting a change in the size of the memory:

1. Signed integer overflow -- which can happen because MemoryCache::adjustSize()
   takes a signed integer argument. If the allocated or the freed memory size is
   larger than the maximum of a signed integer, an overflow will happen.
   For the image caching code, this can be seen where the unsigned decodedSize
   is casted to an integer before passing it to ImageObserver::decodedSizeChanged().

2. Unsigned integer overflow -- which can happen if the new allocated memory
   size plus the currentSize exceeds the maximum of unsigned.
   This can be seen in MemoryCache::adjustSize() where we add delta to m_liveSize
   or m_deadSize without checking whether this addition will overflow or not. We
   do not assert for overflow although we assert for underflow.

The fix for these two problems can be the following:

1. Make all the adjustSize functions all the way till MemoryCache::adjustSize()
   take a signed long integer argument.

2. Do not create a NativeImagePtr for an ImageFrame if its frameBytes plus the
   ImageFrameCache::decodedSize() will exceed the maximum of an unsigned integer.

* loader/cache/CachedImage.cpp:
(WebCore::CachedImage::decodedSizeChanged): Change the argument to be long. No overflow will happen when casting the argument from unsigned to long.
* loader/cache/CachedImage.h:
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::setDecodedSize): Use long integer casting when calling MemoryCache::adjustSize().
(WebCore::CachedResource::setEncodedSize): Ditto.
* loader/cache/MemoryCache.cpp:
(WebCore::MemoryCache::MemoryCache): Add as static assert to ensure sizeof(long long) can hold any unsigned or its negation.
(WebCore::MemoryCache::revalidationSucceeded): Use long integer casting when calling MemoryCache::adjustSize().
(WebCore::MemoryCache::remove): Ditto.
(WebCore::MemoryCache::adjustSize): Change the function argument to long integer. No overflow will happen when casting the argument from unsigned to long.
* loader/cache/MemoryCache.h:
* platform/graphics/ImageFrameCache.cpp:
(WebCore::ImageFrameCache::destroyIncompleteDecodedData): Call a function with its new name.
(WebCore::ImageFrameCache::decodedSizeChanged): Change the function argument to long integer. No overflow will happen when casting the argument from unsigned to long.
(WebCore::ImageFrameCache::decodedSizeIncreased): Use long integer casting when calling decodedSizeChanged().
(WebCore::ImageFrameCache::decodedSizeDecreased): Ditto.
(WebCore::ImageFrameCache::decodedSizeReset): Ditto.
(WebCore::ImageFrameCache::didDecodeProperties): Ditto.
(WebCore::ImageFrameCache::frameAtIndex): Do not create the NativeImage if adding its frameByes to the MemoryCache will cause numerical overflow.
(WebCore::ImageFrameCache::decodedSizeIncremented): Deleted. This function is renamed decodedSizeIncreased().
(WebCore::ImageFrameCache::decodedSizeDecremented): Deleted. This function is renamed decodedSizeDecreased().
* platform/graphics/ImageFrameCache.h:
* platform/graphics/ImageObserver.h:
* platform/graphics/IntSize.h:
(WebCore::IntSize::unclampedArea): Returns the area of an IntSize in size_t.
* platform/graphics/cg/PDFDocumentImage.cpp:
(WebCore::PDFDocumentImage::decodedSizeChanged): Use long integer casting when calling ImageObserver::decodedSizeChanged().

LayoutTests:

* TestExpectations: Remove failed tests.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/loader/cache/CachedImage.cpp
Source/WebCore/loader/cache/CachedImage.h
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebCore/loader/cache/MemoryCache.cpp
Source/WebCore/loader/cache/MemoryCache.h
Source/WebCore/platform/graphics/ImageFrameCache.cpp
Source/WebCore/platform/graphics/ImageFrameCache.h
Source/WebCore/platform/graphics/ImageObserver.h
Source/WebCore/platform/graphics/IntSize.h
Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp

index 075efc6..042c9b0 100644 (file)
@@ -1,3 +1,13 @@
+2016-09-30  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Change the MemoryCache and CachedResource adjustSize functions to take a long argument
+        https://bugs.webkit.org/show_bug.cgi?id=162708
+        <rdar://problem/28555702>
+
+        Reviewed by Brent Fulgham.
+
+        * TestExpectations: Remove failed tests.
+
 2016-09-30  Chris Dumez  <cdumez@apple.com>
 
         Add support for ImageData.data attribute
index 5bc4d85..a63319a 100644 (file)
@@ -985,18 +985,3 @@ webkit.org/b/162414 imported/w3c/web-platform-tests/resource-timing/idlharness.h
 
 # Only iOS has implemented lettepress.
 fast/text/letterpress-different.html [ ImageOnlyFailure ]
-
-webkit.org/b/162696 [ Release ] fast/images/paletted-png-with-color-profile.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/paint-subrect.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/paint-subrect-grid.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-crop-box.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/link-body-content-imageDimensionChanged-crash.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/object-data-url-case-insensitivity.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/move-image-to-new-document.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/pdf-as-background.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-landscape.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-with-annotations-expected.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-too-big.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/object-image.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-with-annotations.html [ Crash ]
-webkit.org/b/162696 [ Release ] fast/images/load-img-with-empty-src.html [ Crash ]
index 651efd0..5334b7b 100644 (file)
@@ -1,3 +1,63 @@
+2016-09-30  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Change the MemoryCache and CachedResource adjustSize functions to take a long argument
+        https://bugs.webkit.org/show_bug.cgi?id=162708
+        <rdar://problem/28555702>
+
+        Reviewed by Brent Fulgham.
+
+        Because the MemoryCache stores the size of the cached memory in unsigned,
+        two problems my happen when reporting a change in the size of the memory:
+        
+        1. Signed integer overflow -- which can happen because MemoryCache::adjustSize()
+           takes a signed integer argument. If the allocated or the freed memory size is
+           larger than the maximum of a signed integer, an overflow will happen.
+           For the image caching code, this can be seen where the unsigned decodedSize
+           is casted to an integer before passing it to ImageObserver::decodedSizeChanged().
+
+        2. Unsigned integer overflow -- which can happen if the new allocated memory
+           size plus the currentSize exceeds the maximum of unsigned.
+           This can be seen in MemoryCache::adjustSize() where we add delta to m_liveSize
+           or m_deadSize without checking whether this addition will overflow or not. We
+           do not assert for overflow although we assert for underflow.
+           
+        The fix for these two problems can be the following:
+        
+        1. Make all the adjustSize functions all the way till MemoryCache::adjustSize()
+           take a signed long integer argument.
+           
+        2. Do not create a NativeImagePtr for an ImageFrame if its frameBytes plus the
+           ImageFrameCache::decodedSize() will exceed the maximum of an unsigned integer.
+
+        * loader/cache/CachedImage.cpp:
+        (WebCore::CachedImage::decodedSizeChanged): Change the argument to be long. No overflow will happen when casting the argument from unsigned to long.
+        * loader/cache/CachedImage.h:
+        * loader/cache/CachedResource.cpp: 
+        (WebCore::CachedResource::setDecodedSize): Use long integer casting when calling MemoryCache::adjustSize().
+        (WebCore::CachedResource::setEncodedSize): Ditto.
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::MemoryCache): Add as static assert to ensure sizeof(long long) can hold any unsigned or its negation.
+        (WebCore::MemoryCache::revalidationSucceeded): Use long integer casting when calling MemoryCache::adjustSize().
+        (WebCore::MemoryCache::remove): Ditto.
+        (WebCore::MemoryCache::adjustSize): Change the function argument to long integer. No overflow will happen when casting the argument from unsigned to long.
+        * loader/cache/MemoryCache.h:
+        * platform/graphics/ImageFrameCache.cpp:
+        (WebCore::ImageFrameCache::destroyIncompleteDecodedData): Call a function with its new name.
+        (WebCore::ImageFrameCache::decodedSizeChanged): Change the function argument to long integer. No overflow will happen when casting the argument from unsigned to long.
+        (WebCore::ImageFrameCache::decodedSizeIncreased): Use long integer casting when calling decodedSizeChanged().
+        (WebCore::ImageFrameCache::decodedSizeDecreased): Ditto.
+        (WebCore::ImageFrameCache::decodedSizeReset): Ditto.
+        (WebCore::ImageFrameCache::didDecodeProperties): Ditto.
+        (WebCore::ImageFrameCache::frameAtIndex): Do not create the NativeImage if adding its frameByes to the MemoryCache will cause numerical overflow.
+        (WebCore::ImageFrameCache::decodedSizeIncremented): Deleted. This function is renamed decodedSizeIncreased().
+        (WebCore::ImageFrameCache::decodedSizeDecremented): Deleted. This function is renamed decodedSizeDecreased().
+        * platform/graphics/ImageFrameCache.h:
+        * platform/graphics/ImageObserver.h:
+        * platform/graphics/IntSize.h:
+        (WebCore::IntSize::unclampedArea): Returns the area of an IntSize in size_t.
+        * platform/graphics/cg/PDFDocumentImage.cpp:
+        (WebCore::PDFDocumentImage::decodedSizeChanged): Use long integer casting when calling ImageObserver::decodedSizeChanged().
+
 2016-09-30  Chris Dumez  <cdumez@apple.com>
 
         Add support for ImageData.data attribute
index 46f9dd4..b80b785 100644 (file)
@@ -450,11 +450,12 @@ void CachedImage::destroyDecodedData()
         m_image->destroyDecodedData();
 }
 
-void CachedImage::decodedSizeChanged(const Image* image, int delta)
+void CachedImage::decodedSizeChanged(const Image* image, long long delta)
 {
     if (!image || image != m_image)
         return;
-    
+
+    ASSERT(delta >= 0 || decodedSize() + delta >= 0);
     setDecodedSize(decodedSize() + delta);
 }
 
index 8e82394..8cfcc92 100644 (file)
@@ -118,7 +118,7 @@ private:
     bool stillNeedsLoad() const override { return !errorOccurred() && status() == Unknown && !isLoading(); }
 
     // ImageObserver
-    void decodedSizeChanged(const Image*, int delta) override;
+    void decodedSizeChanged(const Image*, long long delta) override;
     void didDraw(const Image*) override;
 
     void animationAdvanced(const Image*) override;
index 11bfdff..3bac916 100644 (file)
@@ -649,13 +649,13 @@ void CachedResource::setDecodedSize(unsigned size)
     if (size == m_decodedSize)
         return;
 
-    int delta = size - m_decodedSize;
+    long long delta = static_cast<long long>(size) - m_decodedSize;
 
     // The object must be moved to a different queue, since its size has been changed.
     // Remove before updating m_decodedSize, so we find the resource in the correct LRU list.
     if (allowsCaching() && inCache())
         MemoryCache::singleton().removeFromLRUList(*this);
-    
+
     m_decodedSize = size;
    
     if (allowsCaching() && inCache()) {
@@ -686,7 +686,7 @@ void CachedResource::setEncodedSize(unsigned size)
     if (size == m_encodedSize)
         return;
 
-    int delta = size - m_encodedSize;
+    long long delta = static_cast<long long>(size) - m_encodedSize;
 
     // The object must be moved to a different queue, since its size has been changed.
     // Remove before updating m_encodedSize, so we find the resource in the correct LRU list.
index 63334c5..75e1d7e 100644 (file)
@@ -71,6 +71,7 @@ MemoryCache::MemoryCache()
     , m_deadSize(0)
     , m_pruneTimer(*this, &MemoryCache::prune)
 {
+    static_assert(sizeof(long long) > sizeof(unsigned), "Numerical overflow can happen when adjusting the size of the cached memory.");
 }
 
 auto MemoryCache::sessionResourceMap(SessionID sessionID) const -> CachedResourceMap*
@@ -148,7 +149,7 @@ void MemoryCache::revalidationSucceeded(CachedResource& revalidatingResource, co
     resource.setInCache(true);
     resource.updateResponseAfterRevalidation(response);
     insertInLRUList(resource);
-    int delta = resource.size();
+    long long delta = resource.size();
     if (resource.decodedSize() && resource.hasClients())
         insertInLiveDecodedResourcesList(resource);
     if (delta)
@@ -448,7 +449,7 @@ void MemoryCache::remove(CachedResource& resource)
             // Remove from the appropriate LRU list.
             removeFromLRUList(resource);
             removeFromLiveDecodedResourcesList(resource);
-            adjustSize(resource.hasClients(), -static_cast<int>(resource.size()));
+            adjustSize(resource.hasClients(), -static_cast<long long>(resource.size()));
         } else
             ASSERT(resources->get(key) != &resource);
     }
@@ -641,13 +642,13 @@ void MemoryCache::removeFromLiveResourcesSize(CachedResource& resource)
     m_deadSize += resource.size();
 }
 
-void MemoryCache::adjustSize(bool live, int delta)
+void MemoryCache::adjustSize(bool live, long long delta)
 {
     if (live) {
-        ASSERT(delta >= 0 || ((int)m_liveSize + delta >= 0));
+        ASSERT(delta >= 0 || (static_cast<long long>(m_liveSize) + delta >= 0));
         m_liveSize += delta;
     } else {
-        ASSERT(delta >= 0 || ((int)m_deadSize + delta >= 0));
+        ASSERT(delta >= 0 || (static_cast<long long>(m_deadSize) + delta >= 0));
         m_deadSize += delta;
     }
 }
index 0774948..502972a 100644 (file)
@@ -133,7 +133,7 @@ public:
     void removeFromLRUList(CachedResource&);
 
     // Called to adjust the cache totals when a resource changes size.
-    void adjustSize(bool live, int delta);
+    void adjustSize(bool live, long long delta);
 
     // Track decoded resources that are in the cache and referenced by a Web page.
     void insertInLiveDecodedResourcesList(CachedResource&);
index 50cccfa..fbfd601 100644 (file)
@@ -35,6 +35,9 @@
 #include "ImageDecoder.h"
 #endif
 
+#include <wtf/CheckedArithmetic.h>
+
+
 namespace WebCore {
 
 ImageFrameCache::ImageFrameCache(Image* image)
@@ -66,7 +69,7 @@ void ImageFrameCache::destroyDecodedData(bool destroyAll, size_t count)
     unsigned decodedSize = 0;
     for (size_t i = 0; i <  count; ++i)
         decodedSize += m_frames[i].clearImage();
-    
+
     decodedSizeReset(decodedSize);
 }
 
@@ -93,12 +96,11 @@ void ImageFrameCache::destroyIncompleteDecodedData()
         
         decodedSize += frame.clear();
     }
-    
-    decodedSizeDecremented(decodedSize);
-}
 
+    decodedSizeDecreased(decodedSize);
+}
 
-void ImageFrameCache::decodedSizeChanged(int decodedSize)
+void ImageFrameCache::decodedSizeChanged(long long decodedSize)
 {
     if (!decodedSize || !m_image || !m_image->imageObserver())
         return;
@@ -106,7 +108,7 @@ void ImageFrameCache::decodedSizeChanged(int decodedSize)
     m_image->imageObserver()->decodedSizeChanged(m_image, decodedSize);
 }
 
-void ImageFrameCache::decodedSizeIncremented(unsigned decodedSize)
+void ImageFrameCache::decodedSizeIncreased(unsigned decodedSize)
 {
     if (!decodedSize)
         return;
@@ -115,39 +117,39 @@ void ImageFrameCache::decodedSizeIncremented(unsigned decodedSize)
     
     // The fully-decoded frame will subsume the partially decoded data used
     // to determine image properties.
-    int changeSize = decodedSize - m_decodedPropertiesSize;
+    long long changeSize = static_cast<long long>(decodedSize) - m_decodedPropertiesSize;
     m_decodedPropertiesSize = 0;
     decodedSizeChanged(changeSize);
 }
 
-void ImageFrameCache::decodedSizeDecremented(unsigned decodedSize)
+void ImageFrameCache::decodedSizeDecreased(unsigned decodedSize)
 {
     if (!decodedSize)
         return;
-    
+
     ASSERT(m_decodedSize >= decodedSize);
     m_decodedSize -= decodedSize;
-    decodedSizeChanged(-safeCast<int>(decodedSize));
+    decodedSizeChanged(-static_cast<long long>(decodedSize));
 }
 
 void ImageFrameCache::decodedSizeReset(unsigned decodedSize)
 {
     ASSERT(m_decodedSize >= decodedSize);
     m_decodedSize -= decodedSize;
-    
+
     // Clearing the ImageSource destroys the extra decoded data used for
     // determining image properties.
     decodedSize += m_decodedPropertiesSize;
     m_decodedPropertiesSize = 0;
-    decodedSizeChanged(-safeCast<int>(decodedSize));
+    decodedSizeChanged(-static_cast<long long>(decodedSize));
 }
 
 void ImageFrameCache::didDecodeProperties(unsigned decodedPropertiesSize)
 {
     if (m_decodedSize)
         return;
-    
-    int decodedSize = decodedPropertiesSize - m_decodedPropertiesSize;
+
+    long long decodedSize = static_cast<long long>(decodedPropertiesSize) - m_decodedPropertiesSize;
     m_decodedPropertiesSize = decodedPropertiesSize;
     decodedSizeChanged(decodedSize);
 }
@@ -215,15 +217,20 @@ const ImageFrame& ImageFrameCache::frameAtIndex(size_t index, SubsamplingLevel s
     
     if (frame.hasInvalidNativeImage(subsamplingLevel)) {
         unsigned decodedSize = frame.clear();
-        decodedSizeDecremented(decodedSize);
+        decodedSizeDecreased(decodedSize);
     }
     
     if (!frame.isComplete() && caching == ImageFrame::Caching::Metadata)
         setFrameMetadata(index, subsamplingLevel);
     
     if (!frame.hasNativeImage() && caching == ImageFrame::Caching::MetadataAndImage) {
-        setFrameNativeImage(m_decoder->createFrameImageAtIndex(index, subsamplingLevel), index, subsamplingLevel);
-        decodedSizeIncremented(frame.frameBytes());
+        size_t frameBytes = size().unclampedArea() * sizeof(RGBA32);
+
+        // Do not create the NativeImage if adding its frameByes to the MemoryCache will cause numerical overflow.
+        if (WTF::isInBounds<unsigned>(frameBytes + decodedSize())) {
+            setFrameNativeImage(m_decoder->createFrameImageAtIndex(index, subsamplingLevel), index, subsamplingLevel);
+            decodedSizeIncreased(frame.frameBytes());
+        }
     }
     
     return frame;
index 9a355f4..e12c323 100644 (file)
@@ -87,10 +87,10 @@ private:
     T frameMetadataAtIndex(size_t index, SubsamplingLevel = SubsamplingLevel::Undefinded, ImageFrame::Caching = ImageFrame::Caching::Empty, Optional<T>* = nullptr);
 
     bool isDecoderAvailable() const { return m_decoder; }
-    void decodedSizeChanged(int decodedSize);
+    void decodedSizeChanged(long long decodedSize);
     void didDecodeProperties(unsigned decodedPropertiesSize);
-    void decodedSizeIncremented(unsigned decodedSize);
-    void decodedSizeDecremented(unsigned decodedSize);
+    void decodedSizeIncreased(unsigned decodedSize);
+    void decodedSizeDecreased(unsigned decodedSize);
     void decodedSizeReset(unsigned decodedSize);
 
     void setNativeImage(NativeImagePtr&&);
index ed6b017..8fa5983 100644 (file)
@@ -37,7 +37,7 @@ class ImageObserver {
 protected:
     virtual ~ImageObserver() {}
 public:
-    virtual void decodedSizeChanged(const Image*, int delta) = 0;
+    virtual void decodedSizeChanged(const Image*, long long delta) = 0;
     virtual void didDraw(const Image*) = 0;
 
     virtual void animationAdvanced(const Image*) = 0;
index fb3a3cb..2db1aca 100644 (file)
@@ -136,6 +136,11 @@ public:
         return abs(m_width) * abs(m_height);
     }
 
+    size_t unclampedArea() const
+    {
+        return static_cast<size_t>(abs(m_width)) * abs(m_height);
+    }
+
     int diagonalLengthSquared() const
     {
         return m_width * m_width + m_height * m_height;
index 80cb454..450a5c0 100644 (file)
@@ -182,7 +182,7 @@ void PDFDocumentImage::decodedSizeChanged(size_t newCachedBytes)
         return;
 
     if (imageObserver())
-        imageObserver()->decodedSizeChanged(this, -safeCast<int>(m_cachedBytes) + newCachedBytes);
+        imageObserver()->decodedSizeChanged(this, -static_cast<long long>(m_cachedBytes) + newCachedBytes);
 
     ASSERT(s_allDecodedDataSize >= m_cachedBytes);
     // Update with the difference in two steps to avoid unsigned underflow subtraction.