[Webkit-unassigned] [Bug 172552] New: REGRESSION (r206481): Don't assume frameCount() is larger than or equal to the size of the frame cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=172552

            Bug ID: 172552
           Summary: REGRESSION (r206481): Don't assume frameCount() is
                    larger than or equal to the size of the frame cache
           Product: WebKit
           Version: WebKit Nightly Build
          Hardware: Unspecified
                OS: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Images
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: sabouhallawa at apple.com

Before <http://trac.webkit.org/changeset/206481>, BitmapImage::cacheFrame() had the following statement:

    if (m_frames.size() < numFrames)
        m_frames.grow(numFrames)

Since this change was landed, ImageFrameCache::growFrames() has the following code:

    ASSERT(m_frames.size() <= frameCount());
    m_frames.grow(frameCount());

The assumption was ImageDecoder may change the value of the frameCount() when it receives more data but for sure the value of the new frameCount() is larger than the previous value. And since in ImageFrameCache::growFrames() we want to match m_frames.size() with the current frameCount(), m_frames.size() should be equal to the previous or the current frameCount() which should be equal to or less than the current frameCount(). This means m_frames.size() should be less than or equal to frameCount() always. And this why I replaced the if-satament with just an assertion.

But this assumption does not consider the case when the image is cached and its encoded data is released. In this case, the encoded data will be read incrementally and the current m_frames.size() will be larger than the initial frameCount().

Aside form the assertion will fire in debug build, this would not be a problem if Vector::grow() handles this case safely but it does not. Vector::grow() asserts ASSERT(size >= m_size) but it does not return if (size < m_size). The real problem happens because of this statement in Vector::grow()

    if (begin())
        TypeOperations::initialize(end(), begin() + size);

If size < m_size => begin() + size < begin() + m_size => begin() + size() < end(). So we are sending two pointers to TypeOperations::initialize(), the first is larger than the second. In the case of ImageFrame, this will be VectorInitializer to be called:

template<typename T>
struct VectorInitializer<true, false, T>
{
    static void initialize(T* begin, T* end) 
    {
        for (T* cur = begin; cur != end; ++cur)
            new (NotNull, cur) T;
    }
};

if begin > end, the for loop above will never terminates and will crash because of memory access violation and get a crash like this:

0   com.apple.WebCore                   0x00007fffc0688481 WebCore::ImageFrame::ImageFrame() + 17
1   com.apple.WebCore                   0x00007fffc0689038 WebCore::ImageFrameCache::growFrames() + 152
2   com.apple.WebCore                   0x00007fffc068f71b WebCore::ImageSource::dataChanged(WebCore::SharedBuffer*, bool) + 91
3   com.apple.WebCore                   0x00007fffc0224095 WebCore::CachedImage::setImageDataBuffer(WebCore::SharedBuffer*, bool) + 53
4   com.apple.WebCore                   0x00007fffc0223f8f WebCore::CachedImage::addIncrementalDataBuffer(WebCore::SharedBuffer&) + 63
5   com.apple.WebCore                   0x00007fffc0224132 WebCore::CachedImage::addDataBuffer(WebCore::SharedBuffer&) + 18
6   com.apple.WebCore                   0x00007fffc0fd1ef7 WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::RefPtr<WebCore::SharedBuffer>&&, long long, WebCore::DataPayloadType) + 183
7   com.apple.WebCore                   0x00007fffc0fd1f8a WebCore::SubresourceLoader::didReceiveBuffer(WTF::Ref<WebCore::SharedBuffer>&&, long long, WebCore::DataPayloadType) + 42

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170524/86bed08d/attachment.html>