Bug 172552 – REGRESSION (r206481): Don't assume frameCount() is larger than or equal to the size of the image frame cache

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 311144 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/32296566>

[Jer Noble]

(In reply to Said Abou-Hallawa from comment #2)
> <rdar://problem/32296566>

I talked about this patch with Geoff, who said that the purpose of grow() and shrink() was that they were an optimization if you knew the size was increasing (or decreasing, respectively).  So I think you should back out the change to Vector.h and leave in the change to ImageFrameCache.cpp.

[Geoffrey Garen]

Comment on attachment 311144 [details]
Patch

The purpose of shrink() and grow() is to optimize the case when you know that you're shrinking or growing, by avoiding a check.

Clients that don't know what kind of size change they're making can call resize().

[Said Abou-Hallawa]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 311144 [details]
> Patch
> 
> The purpose of shrink() and grow() is to optimize the case when you know
> that you're shrinking or growing, by avoiding a check.
> 
> Clients that don't know what kind of size change they're making can call
> resize().

I disagree with leaving the vector code as it. The image frame caching used this optimization because I assumed the size of the vector would be always increasing. The problem is my assumption itself was worng and this optimization in the grow() function did not try to help in any way asserting the problem. The code just goes in infinite loop and causes memory access violation.

But since you dislike changing grow() and shrink() function, how about changing the loops in VectorInitializer::initialize() and VectorDestructor::destruct() to be like this:

    for (T* cur = begin; cur < end; ++cur)

instead of 

    for (T* cur = begin; cur != end; ++cur)

I am not sure why these function use cur != end instead of cur < end. Is it another optimization? Is != is faster than <?

Another suggestion is to replace the ASSERT() in grow() and shrink with RELEASE_ASSERT().

The reason for me making these suggestions is I spent long time trying to figure out the reason of this crash from looking at the crash trace and I could not figure it out. I could figure it out only when Jer could reproduce the bug and debug it and knew that the crash is happening because the new size is less than the current size. I just don't want someone else to waste his time because of this silent optimization.

[Said Abou-Hallawa]

Created attachment 311156 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 311156 [details]
Patch

I filed https://bugs.webkit.org/show_bug.cgi?id=172556 to address the WFT::Vector problem. Here I will address the ImageFrameCache problem only.

[Said Abou-Hallawa]

Created attachment 311162 [details]
Patch

[Geoffrey Garen]

Comment on attachment 311162 [details]
Patch

r=me

[Geoffrey Garen]

> But since you dislike changing grow() and shrink() function, how about
> changing the loops in VectorInitializer::initialize() and
> VectorDestructor::destruct() to be like this:
> 
>     for (T* cur = begin; cur < end; ++cur)
> 
> instead of 
> 
>     for (T* cur = begin; cur != end; ++cur)

I don't think we should build support for invalid pointers into our low level data structures. That way madness lies.

[WebKit Commit Bot]

Comment on attachment 311162 [details]
Patch

Clearing flags on attachment: 311162

Committed r217392: <http://trac.webkit.org/changeset/217392>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

*** Bug 168396 has been marked as a duplicate of this bug. ***