| Summary: | Refine assertions in WebCore::ImageData constructors | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
| Component: | Images | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bfulgham, cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim, simon.fraser, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | Safari 10 | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=163696 https://bugs.webkit.org/show_bug.cgi?id=163757 https://bugs.webkit.org/show_bug.cgi?id=163762 |
||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 163696 | ||||||
| Attachments: |
|
||||||
|
Description
David Kilzer (:ddkilzer)
2016-10-19 10:35:10 PDT
Created attachment 292082 [details]
Patch v1
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 (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)) (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. Committed r207560: <http://trac.webkit.org/changeset/207560> What protects the area and * 4 computations from overflowing? (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. (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 |