Changeset 206635 in webkit


Ignore:
Timestamp:
Sep 30, 2016, 9:01:36 AM (9 years ago)
Author:
Said Abou-Hallawa
Message:

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.

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().
  1. 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.


  1. 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:

Location:
trunk
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r206634 r206635  
     12016-09-30  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        Change the MemoryCache and CachedResource adjustSize functions to take a long argument
     4        https://bugs.webkit.org/show_bug.cgi?id=162708
     5        <rdar://problem/28555702>
     6
     7        Reviewed by Brent Fulgham.
     8
     9        * TestExpectations: Remove failed tests.
     10
    1112016-09-30  Chris Dumez  <cdumez@apple.com>
    212

  • trunk/LayoutTests/TestExpectations

    r206602 r206635  
    986986# Only iOS has implemented lettepress.
    987987fast/text/letterpress-different.html [ ImageOnlyFailure ]
    988 
    989 webkit.org/b/162696 [ Release ] fast/images/paletted-png-with-color-profile.html [ Crash ]
    990 webkit.org/b/162696 [ Release ] fast/images/paint-subrect.html [ Crash ]
    991 webkit.org/b/162696 [ Release ] fast/images/paint-subrect-grid.html [ Crash ]
    992 webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-crop-box.html [ Crash ]
    993 webkit.org/b/162696 [ Release ] fast/images/link-body-content-imageDimensionChanged-crash.html [ Crash ]
    994 webkit.org/b/162696 [ Release ] fast/images/object-data-url-case-insensitivity.html [ Crash ]
    995 webkit.org/b/162696 [ Release ] fast/images/move-image-to-new-document.html [ Crash ]
    996 webkit.org/b/162696 [ Release ] fast/images/pdf-as-background.html [ Crash ]
    997 webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-landscape.html [ Crash ]
    998 webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-with-annotations-expected.html [ Crash ]
    999 webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-too-big.html [ Crash ]
    1000 webkit.org/b/162696 [ Release ] fast/images/object-image.html [ Crash ]
    1001 webkit.org/b/162696 [ Release ] fast/images/pdf-as-image-with-annotations.html [ Crash ]
    1002 webkit.org/b/162696 [ Release ] fast/images/load-img-with-empty-src.html [ Crash ]

  • trunk/Source/WebCore/ChangeLog

    r206634 r206635  
     12016-09-30  Said Abou-Hallawa  <sabouhallawa@apple.com>
     2
     3        Change the MemoryCache and CachedResource adjustSize functions to take a long argument
     4        https://bugs.webkit.org/show_bug.cgi?id=162708
     5        <rdar://problem/28555702>
     6
     7        Reviewed by Brent Fulgham.
     8
     9        Because the MemoryCache stores the size of the cached memory in unsigned,
     10        two problems my happen when reporting a change in the size of the memory:
     11       
     12        1. Signed integer overflow -- which can happen because MemoryCache::adjustSize()
     13           takes a signed integer argument. If the allocated or the freed memory size is
     14           larger than the maximum of a signed integer, an overflow will happen.
     15           For the image caching code, this can be seen where the unsigned decodedSize
     16           is casted to an integer before passing it to ImageObserver::decodedSizeChanged().
     17
     18        2. Unsigned integer overflow -- which can happen if the new allocated memory
     19           size plus the currentSize exceeds the maximum of unsigned.
     20           This can be seen in MemoryCache::adjustSize() where we add delta to m_liveSize
     21           or m_deadSize without checking whether this addition will overflow or not. We
     22           do not assert for overflow although we assert for underflow.
     23           
     24        The fix for these two problems can be the following:
     25       
     26        1. Make all the adjustSize functions all the way till MemoryCache::adjustSize()
     27           take a signed long integer argument.
     28           
     29        2. Do not create a NativeImagePtr for an ImageFrame if its frameBytes plus the
     30           ImageFrameCache::decodedSize() will exceed the maximum of an unsigned integer.
     31
     32        * loader/cache/CachedImage.cpp:
     33        (WebCore::CachedImage::decodedSizeChanged): Change the argument to be long. No overflow will happen when casting the argument from unsigned to long.
     34        * loader/cache/CachedImage.h:
     35        * loader/cache/CachedResource.cpp:
     36        (WebCore::CachedResource::setDecodedSize): Use long integer casting when calling MemoryCache::adjustSize().
     37        (WebCore::CachedResource::setEncodedSize): Ditto.
     38        * loader/cache/MemoryCache.cpp:
     39        (WebCore::MemoryCache::MemoryCache): Add as static assert to ensure sizeof(long long) can hold any unsigned or its negation.
     40        (WebCore::MemoryCache::revalidationSucceeded): Use long integer casting when calling MemoryCache::adjustSize().
     41        (WebCore::MemoryCache::remove): Ditto.
     42        (WebCore::MemoryCache::adjustSize): Change the function argument to long integer. No overflow will happen when casting the argument from unsigned to long.
     43        * loader/cache/MemoryCache.h:
     44        * platform/graphics/ImageFrameCache.cpp:
     45        (WebCore::ImageFrameCache::destroyIncompleteDecodedData): Call a function with its new name.
     46        (WebCore::ImageFrameCache::decodedSizeChanged): Change the function argument to long integer. No overflow will happen when casting the argument from unsigned to long.
     47        (WebCore::ImageFrameCache::decodedSizeIncreased): Use long integer casting when calling decodedSizeChanged().
     48        (WebCore::ImageFrameCache::decodedSizeDecreased): Ditto.
     49        (WebCore::ImageFrameCache::decodedSizeReset): Ditto.
     50        (WebCore::ImageFrameCache::didDecodeProperties): Ditto.
     51        (WebCore::ImageFrameCache::frameAtIndex): Do not create the NativeImage if adding its frameByes to the MemoryCache will cause numerical overflow.
     52        (WebCore::ImageFrameCache::decodedSizeIncremented): Deleted. This function is renamed decodedSizeIncreased().
     53        (WebCore::ImageFrameCache::decodedSizeDecremented): Deleted. This function is renamed decodedSizeDecreased().
     54        * platform/graphics/ImageFrameCache.h:
     55        * platform/graphics/ImageObserver.h:
     56        * platform/graphics/IntSize.h:
     57        (WebCore::IntSize::unclampedArea): Returns the area of an IntSize in size_t.
     58        * platform/graphics/cg/PDFDocumentImage.cpp:
     59        (WebCore::PDFDocumentImage::decodedSizeChanged): Use long integer casting when calling ImageObserver::decodedSizeChanged().
     60
    1612016-09-30  Chris Dumez  <cdumez@apple.com>
    262

  • trunk/Source/WebCore/loader/cache/CachedImage.cpp

    r206435 r206635  
    451451}
    452452
    453 void CachedImage::decodedSizeChanged(const Image* image, int delta)
     453void CachedImage::decodedSizeChanged(const Image* image, long long delta)
    454454{
    455455    if (!image || image != m_image)
    456456        return;
    457    
     457
     458    ASSERT(delta >= 0 || decodedSize() + delta >= 0);
    458459    setDecodedSize(decodedSize() + delta);
    459460}

  • trunk/Source/WebCore/loader/cache/CachedImage.h

    r206435 r206635  
    119119
    120120    // ImageObserver
    121     void decodedSizeChanged(const Image*, int delta) override;
     121    void decodedSizeChanged(const Image*, long long delta) override;
    122122    void didDraw(const Image*) override;
    123123

  • trunk/Source/WebCore/loader/cache/CachedResource.cpp

    r206370 r206635  
    650650        return;
    651651
    652     int delta = size - m_decodedSize;
     652    long long delta = static_cast<long long>(size) - m_decodedSize;
    653653
    654654    // The object must be moved to a different queue, since its size has been changed.
    … …  
    656656    if (allowsCaching() && inCache())
    657657        MemoryCache::singleton().removeFromLRUList(*this);
    658    
     658
    659659    m_decodedSize = size;
    660660   
    … …  
    687687        return;
    688688
    689     int delta = size - m_encodedSize;
     689    long long delta = static_cast<long long>(size) - m_encodedSize;
    690690
    691691    // The object must be moved to a different queue, since its size has been changed.

  • trunk/Source/WebCore/loader/cache/MemoryCache.cpp

    r206435 r206635  
    7272    , m_pruneTimer(*this, &MemoryCache::prune)
    7373{
     74    static_assert(sizeof(long long) > sizeof(unsigned), "Numerical overflow can happen when adjusting the size of the cached memory.");
    7475}
    7576
    … …  
    149150    resource.updateResponseAfterRevalidation(response);
    150151    insertInLRUList(resource);
    151     int delta = resource.size();
     152    long long delta = resource.size();
    152153    if (resource.decodedSize() && resource.hasClients())
    153154        insertInLiveDecodedResourcesList(resource);
    … …  
    449450            removeFromLRUList(resource);
    450451            removeFromLiveDecodedResourcesList(resource);
    451             adjustSize(resource.hasClients(), -static_cast<int>(resource.size()));
     452            adjustSize(resource.hasClients(), -static_cast<long long>(resource.size()));
    452453        } else
    453454            ASSERT(resources->get(key) != &resource);
    … …  
    642643}
    643644
    644 void MemoryCache::adjustSize(bool live, int delta)
     645void MemoryCache::adjustSize(bool live, long long delta)
    645646{
    646647    if (live) {
    647         ASSERT(delta >= 0 || ((int)m_liveSize + delta >= 0));
     648        ASSERT(delta >= 0 || (static_cast<long long>(m_liveSize) + delta >= 0));
    648649        m_liveSize += delta;
    649650    } else {
    650         ASSERT(delta >= 0 || ((int)m_deadSize + delta >= 0));
     651        ASSERT(delta >= 0 || (static_cast<long long>(m_deadSize) + delta >= 0));
    651652        m_deadSize += delta;
    652653    }

  • trunk/Source/WebCore/loader/cache/MemoryCache.h

    r205682 r206635  
    134134
    135135    // Called to adjust the cache totals when a resource changes size.
    136     void adjustSize(bool live, int delta);
     136    void adjustSize(bool live, long long delta);
    137137
    138138    // Track decoded resources that are in the cache and referenced by a Web page.

  • trunk/Source/WebCore/platform/graphics/ImageFrameCache.cpp

    r206481 r206635  
    3636#endif
    3737
     38#include <wtf/CheckedArithmetic.h>
     39
     40
    3841namespace WebCore {
    3942
    … …  
    6770    for (size_t i = 0; i <  count; ++i)
    6871        decodedSize += m_frames[i].clearImage();
    69    
     72
    7073    decodedSizeReset(decodedSize);
    7174}
    … …  
    9497        decodedSize += frame.clear();
    9598    }
    96    
    97     decodedSizeDecremented(decodedSize);
    98 }
    99 
    100 
    101 void ImageFrameCache::decodedSizeChanged(int decodedSize)
     99
     100    decodedSizeDecreased(decodedSize);
     101}
     102
     103void ImageFrameCache::decodedSizeChanged(long long decodedSize)
    102104{
    103105    if (!decodedSize || !m_image || !m_image->imageObserver())
    … …  
    107109}
    108110
    109 void ImageFrameCache::decodedSizeIncremented(unsigned decodedSize)
     111void ImageFrameCache::decodedSizeIncreased(unsigned decodedSize)
    110112{
    111113    if (!decodedSize)
    … …  
    116118    // The fully-decoded frame will subsume the partially decoded data used
    117119    // to determine image properties.
    118     int changeSize = decodedSize - m_decodedPropertiesSize;
     120    long long changeSize = static_cast<long long>(decodedSize) - m_decodedPropertiesSize;
    119121    m_decodedPropertiesSize = 0;
    120122    decodedSizeChanged(changeSize);
    121123}
    122124
    123 void ImageFrameCache::decodedSizeDecremented(unsigned decodedSize)
     125void ImageFrameCache::decodedSizeDecreased(unsigned decodedSize)
    124126{
    125127    if (!decodedSize)
    126128        return;
    127    
     129
    128130    ASSERT(m_decodedSize >= decodedSize);
    129131    m_decodedSize -= decodedSize;
    130     decodedSizeChanged(-safeCast<int>(decodedSize));
     132    decodedSizeChanged(-static_cast<long long>(decodedSize));
    131133}
    132134
    … …  
    135137    ASSERT(m_decodedSize >= decodedSize);
    136138    m_decodedSize -= decodedSize;
    137    
     139
    138140    // Clearing the ImageSource destroys the extra decoded data used for
    139141    // determining image properties.
    140142    decodedSize += m_decodedPropertiesSize;
    141143    m_decodedPropertiesSize = 0;
    142     decodedSizeChanged(-safeCast<int>(decodedSize));
     144    decodedSizeChanged(-static_cast<long long>(decodedSize));
    143145}
    144146
    … …  
    147149    if (m_decodedSize)
    148150        return;
    149    
    150     int decodedSize = decodedPropertiesSize - m_decodedPropertiesSize;
     151
     152    long long decodedSize = static_cast<long long>(decodedPropertiesSize) - m_decodedPropertiesSize;
    151153    m_decodedPropertiesSize = decodedPropertiesSize;
    152154    decodedSizeChanged(decodedSize);
    … …  
    216218    if (frame.hasInvalidNativeImage(subsamplingLevel)) {
    217219        unsigned decodedSize = frame.clear();
    218         decodedSizeDecremented(decodedSize);
     220        decodedSizeDecreased(decodedSize);
    219221    }
    220222   
    … …  
    223225   
    224226    if (!frame.hasNativeImage() && caching == ImageFrame::Caching::MetadataAndImage) {
    225         setFrameNativeImage(m_decoder->createFrameImageAtIndex(index, subsamplingLevel), index, subsamplingLevel);
    226         decodedSizeIncremented(frame.frameBytes());
     227        size_t frameBytes = size().unclampedArea() * sizeof(RGBA32);
     228
     229        // Do not create the NativeImage if adding its frameByes to the MemoryCache will cause numerical overflow.
     230        if (WTF::isInBounds<unsigned>(frameBytes + decodedSize())) {
     231            setFrameNativeImage(m_decoder->createFrameImageAtIndex(index, subsamplingLevel), index, subsamplingLevel);
     232            decodedSizeIncreased(frame.frameBytes());
     233        }
    227234    }
    228235   

  • trunk/Source/WebCore/platform/graphics/ImageFrameCache.h

    r206481 r206635  
    8888
    8989    bool isDecoderAvailable() const { return m_decoder; }
    90     void decodedSizeChanged(int decodedSize);
     90    void decodedSizeChanged(long long decodedSize);
    9191    void didDecodeProperties(unsigned decodedPropertiesSize);
    92     void decodedSizeIncremented(unsigned decodedSize);
    93     void decodedSizeDecremented(unsigned decodedSize);
     92    void decodedSizeIncreased(unsigned decodedSize);
     93    void decodedSizeDecreased(unsigned decodedSize);
    9494    void decodedSizeReset(unsigned decodedSize);
    9595

  • trunk/Source/WebCore/platform/graphics/ImageObserver.h

    r165676 r206635  
    3838    virtual ~ImageObserver() {}
    3939public:
    40     virtual void decodedSizeChanged(const Image*, int delta) = 0;
     40    virtual void decodedSizeChanged(const Image*, long long delta) = 0;
    4141    virtual void didDraw(const Image*) = 0;
    4242

  • trunk/Source/WebCore/platform/graphics/IntSize.h

    r205881 r206635  
    137137    }
    138138
     139    size_t unclampedArea() const
     140    {
     141        return static_cast<size_t>(abs(m_width)) * abs(m_height);
     142    }
     143
    139144    int diagonalLengthSquared() const
    140145    {

  • trunk/Source/WebCore/platform/graphics/cg/PDFDocumentImage.cpp

    r206249 r206635  
    183183
    184184    if (imageObserver())
    185         imageObserver()->decodedSizeChanged(this, -safeCast<int>(m_cachedBytes) + newCachedBytes);
     185        imageObserver()->decodedSizeChanged(this, -static_cast<long long>(m_cachedBytes) + newCachedBytes);
    186186
    187187    ASSERT(s_allDecodedDataSize >= m_cachedBytes);
Note: See TracChangeset for help on using the changeset viewer.