Bug 163670 – Refine assertions in WebCore::ImageData constructors

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

[David Kilzer (:ddkilzer)]

The ASSERT_WITH_SECURITY_IMPLICATION() macros in WebCore::ImageData constructors identify nullptr dereferences as security issues, which they are not.

[David Kilzer (:ddkilzer)]

<rdar://problem/27497338>

[David Kilzer (:ddkilzer)]

Created attachment 292082 [details]
Patch v1

[Brent Fulgham]

Comment on attachment 292082 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=292082&action=review

r=me

> Source/WebCore/html/ImageData.cpp:126
> +    ASSERT_WITH_SECURITY_IMPLICATION(!m_data || static_cast<unsigned>(size.width() * size.height() * 4) <= m_data->length());

Suggest: size.area() * 4

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Comment on attachment 292082 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292082&action=review
> 
> r=me
> 
> > Source/WebCore/html/ImageData.cpp:126
> > +    ASSERT_WITH_SECURITY_IMPLICATION(!m_data || static_cast<unsigned>(size.width() * size.height() * 4) <= m_data->length());
> 
> Suggest: size.area() * 4

Should I change the other constructor as well?

ImageData::ImageData(const IntSize& size)
    : m_size(size)
    , m_data(Uint8ClampedArray::createUninitialized(size.width() * size.height() * 4))

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 292082 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=292082&action=review
> > 
> > r=me
> > 
> > > Source/WebCore/html/ImageData.cpp:126
> > > +    ASSERT_WITH_SECURITY_IMPLICATION(!m_data || static_cast<unsigned>(size.width() * size.height() * 4) <= m_data->length());
> > 
> > Suggest: size.area() * 4
> 
> Should I change the other constructor as well?
> 
> ImageData::ImageData(const IntSize& size)
>     : m_size(size)
>     , m_data(Uint8ClampedArray::createUninitialized(size.width() *
> size.height() * 4))

Talked to Brent in person, and we agreed that we should change this one as well to match.

[David Kilzer (:ddkilzer)]

Committed r207560: <http://trac.webkit.org/changeset/207560>

[Darin Adler]

What protects the area and * 4 computations from overflowing?

[Brent Fulgham]

(In reply to comment #7)
> What protects the area and * 4 computations from overflowing?

Nothing!

David and I were talking about this specific issue yesterday. David is working on a separate patch to do CheckedArithmetic math on these calculations to avoid overflowing.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> (In reply to comment #7)
> > What protects the area and * 4 computations from overflowing?
> 
> Nothing!
> 
> David and I were talking about this specific issue yesterday. David is
> working on a separate patch to do CheckedArithmetic math on these
> calculations to avoid overflowing.

I originally filed:

Bug 163696: Extract common code for computing byte count for images

But Simon didn't like the direction there, so I broke out that patch into:

Bug 163757: Use IntSize::unclampedArea() in PDFDocumentImage::updateCachedImageIfNeeded()
Bug 163762: InSize::area() should used checked arithmetic