Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2252723003:
Fix fragmented image signature handling (Closed)
|
Created:
4 years, 4 months ago by f(malita) Modified:
4 years, 3 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, jbroman, Justin Novosad, Rik, f(malita), blink-reviews, danakj+watch_chromium.org, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix fragmented image signature handling
ImageDecoder::determineImageType() is currently only examining the first
buffer segment (getSomeData). But the signature is not guaranteed to be contiguous,
nor do we ever re-consolidate the buffer after receiving more data.
As a consequence, when the signature is fragmented (e.g. due to a slow
image load), we cannot detect the image type - even after all the data is
later received.
Refactor determineImageType() to consolidate the signature when needed.
BUG=637556
R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org
Committed: https://crrev.com/1b80a74055acae8a0296afe644513f52e34dec79
Cr-Commit-Position: refs/heads/master@{#412631}
Patch Set 1 #Patch Set 2 : minor cleanup #
Total comments: 2
Patch Set 3 : refactor using FastSharedBufferReader #
Total comments: 9
Patch Set 4 : replace useless assert with a comment #
Messages
Total messages: 32 (20 generated)
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment. But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even when all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org ========== to ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment (getSomeData). But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even when all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org ==========
Description was changed from ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment (getSomeData). But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even when all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org ========== to ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment (getSomeData). But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even after all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org ==========
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
scroggo is out this week; pkasting, schenney PTAL. Thanks!
It looks fine to me but I think a second set of eyes on all this fancy new C++11 code would be good. What do you think the chances are of merging this back as far as m52?
https://codereview.chromium.org/2252723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2252723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:110: namespace { Instead of adding all this, why not just use FastSharedBufferReader, which seems like it's designed to do pretty much exactly what you want here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/16 20:02:22, Stephen Chennney wrote: > What do you think the chances are of merging this back as far as m52? The chance of a clean/automatic merge are pretty slim (apparently there was a lot of churn in this area). I think a manual merge is manageable though. https://codereview.chromium.org/2252723003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2252723003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:110: namespace { On 2016/08/16 20:12:25, Peter Kasting wrote: > Instead of adding all this, why not just use FastSharedBufferReader, which seems > like it's designed to do pretty much exactly what you want here? I wasn't familiar with FastSharedBufferReader, good idea. Done. The only downside is an extra SegmentReader allocation, but, since ImageDecoder stores a SegmentReader anyway, I think we can get rid of this temp allocation by refactoring/fusing these methods with the factory. Planning to look at that in a follow-up. https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:140: RefPtr<SegmentReader> reader = SegmentReader::createFromSharedBuffer(const_cast<SharedBuffer*>(&data)); I'm trying to minimize the footprint in order to facilitate merging. Will clean up in a separate CL.
LGTM https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:58: return !memcmp(contents, "\x89\x50\x4E\x47\x0D\x0A\x1A\x0A", 8); Random aside: I just realized that bytes 2-4 here are "PNG"... it'd be a little more readable IMO if this code said that... https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:114: static_assert(kLongestSignatureLength == 14, "kLongestSignatureLength mismatch"); Hmm. This static_assert duplicates the old DCHECK, but it seems a little unhelpful to me. For example, it doesn't prevent us from introducing a memory error bug by changing/adding a matchesXXXSignature function that simply reads more than 14 bytes. It mostly feels like a change detector. In theory, determineImageType() doesn't really need to wait for the longest signature to come in (and I suppose said signature could be longer than the shortest valid file of some other type, although that seems unlikely). I'm imagining some sort of system where each filetype detector asks for as many bytes as it needs and they're auto-tried in shortest-to-longest order, but that's all probably way over-engineered for what we need. Perhaps just replacing the assertion here with a simple comment that this must be updated if we add a matchesXXXLength() function that requires more data is good enough. https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:140: RefPtr<SegmentReader> reader = SegmentReader::createFromSharedBuffer(const_cast<SharedBuffer*>(&data)); On 2016/08/16 21:54:03, f(malita) wrote: > I'm trying to minimize the footprint in order to facilitate merging. Will clean > up in a separate CL. It might be easier to understand to make this CL do the clean thing you want, and then make the merge CLs be more hacky. That way we can decide at merge time if it's easy enough to just go ahead and land the cleaner version.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:114: static_assert(kLongestSignatureLength == 14, "kLongestSignatureLength mismatch"); On 2016/08/16 22:10:18, Peter Kasting wrote: > Perhaps just replacing the assertion here with a simple comment that this must > be updated if we add a matchesXXXLength() function that requires more data is > good enough. Done. Another thing we could do is add static asserts to all matchesFooSignature() methods, to check against their actual strings. I'll do that in a follow-up if it sounds like a good idea. https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:140: RefPtr<SegmentReader> reader = SegmentReader::createFromSharedBuffer(const_cast<SharedBuffer*>(&data)); On 2016/08/16 22:10:18, Peter Kasting wrote: > On 2016/08/16 21:54:03, f(malita) wrote: > > I'm trying to minimize the footprint in order to facilitate merging. Will > clean > > up in a separate CL. > > It might be easier to understand to make this CL do the clean thing you want, > and then make the merge CLs be more hacky. That way we can decide at merge time > if it's easy enough to just go ahead and land the cleaner version. This is what I have in mind (not ready for review just yet): https://codereview.chromium.org/2257513002/ Essentially fold the signature sniffer into the factory, and get rid of the public sniffer methods if possible. It's significantly more involved, so I'd rather stick with a localized fix for the regression and handle the cleanup separately.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fmalita@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2252723003/#ps60001 (title: "replace useless assert with a comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment (getSomeData). But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even after all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org ========== to ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment (getSomeData). But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even after all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment (getSomeData). But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even after all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org ========== to ========== Fix fragmented image signature handling ImageDecoder::determineImageType() is currently only examining the first buffer segment (getSomeData). But the signature is not guaranteed to be contiguous, nor do we ever re-consolidate the buffer after receiving more data. As a consequence, when the signature is fragmented (e.g. due to a slow image load), we cannot detect the image type - even after all the data is later received. Refactor determineImageType() to consolidate the signature when needed. BUG=637556 R=pkasting@chromium.org,scroggo@chromium.org,schenney@chromium.org Committed: https://crrev.com/1b80a74055acae8a0296afe644513f52e34dec79 Cr-Commit-Position: refs/heads/master@{#412631} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1b80a74055acae8a0296afe644513f52e34dec79 Cr-Commit-Position: refs/heads/master@{#412631}
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:114: static_assert(kLongestSignatureLength == 14, "kLongestSignatureLength mismatch"); On 2016/08/17 17:09:31, f(malita) wrote: > On 2016/08/16 22:10:18, Peter Kasting wrote: > > Perhaps just replacing the assertion here with a simple comment that this must > > be updated if we add a matchesXXXLength() function that requires more data is > > good enough. > > Done. > > Another thing we could do is add static asserts to all matchesFooSignature() > methods, to check against their actual strings. I'll do that in a follow-up if > it sounds like a good idea. You mean, like, asserting that the length of "BM" is really 2? That also seems unlikely to catch problems since we basically write these functions once and don't modify the signatures later. I'd skip it. https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:140: RefPtr<SegmentReader> reader = SegmentReader::createFromSharedBuffer(const_cast<SharedBuffer*>(&data)); On 2016/08/17 17:09:31, f(malita) wrote: > On 2016/08/16 22:10:18, Peter Kasting wrote: > > On 2016/08/16 21:54:03, f(malita) wrote: > > > I'm trying to minimize the footprint in order to facilitate merging. Will > > clean > > > up in a separate CL. > > > > It might be easier to understand to make this CL do the clean thing you want, > > and then make the merge CLs be more hacky. That way we can decide at merge > time > > if it's easy enough to just go ahead and land the cleaner version. > > This is what I have in mind (not ready for review just yet): > https://codereview.chromium.org/2257513002/ > > Essentially fold the signature sniffer into the factory, and get rid of the > public sniffer methods if possible. > > It's significantly more involved, so I'd rather stick with a localized fix for > the regression and handle the cleanup separately. Sounds like a good plan.
Message was sent while issue was closed.
https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (right): https://codereview.chromium.org/2252723003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:114: static_assert(kLongestSignatureLength == 14, "kLongestSignatureLength mismatch"); On 2016/08/17 22:46:12, Peter Kasting wrote: > On 2016/08/17 17:09:31, f(malita) wrote: > > On 2016/08/16 22:10:18, Peter Kasting wrote: > > > Perhaps just replacing the assertion here with a simple comment that this > must > > > be updated if we add a matchesXXXLength() function that requires more data > is > > > good enough. > > > > Done. > > > > Another thing we could do is add static asserts to all matchesFooSignature() > > methods, to check against their actual strings. I'll do that in a follow-up > if > > it sounds like a good idea. > > You mean, like, asserting that the length of "BM" is really 2? That also seems > unlikely to catch problems since we basically write these functions once and > don't modify the signatures later. I'd skip it. More like refactoring all signature matchers to call a common helper instead of direct memcmp(), and then assert that what we're trying to match <= kLongestSignatureLength. But yeah, that won't prevent anyone from adding a new signature method which doesn't call the helper. A better approach would be to use some sort of signature->factory registry + generalized signature matcher. That would force everyone onto a common code path where we can check the invariant, but unfortunately we have the two-piece "RIFF????WEBPVP" to complicate things. Solvable, but we're in over-engineered territory by now - so I'll take your advice and pass :)
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
