Don't decode AND mask for an icon that already has alpha information

Don't decode AND mask for an icon that already has alpha information
    
Normally BMP-format images in an ICO file contain data for two images:
first the color data for the icon, followed by a 1 bit transparency
mask ("AND" bitmap). If the color data includes alpha data which is not
completely transparent, we should ignore the mask bitmap data. In fact,
the mask bitmap data may be completely missing in this case, so the
decoder would have previously interpreted the start of the next image
in the icon as the mask.
    
BUG=424272
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185957

[Sami, 2014-11-17 20:31:13]

skyostil@chromium.org changed reviewers:
+ senorblanco@chromium.org

[Stephen White, 2014-11-17 20:33:48]

senorblanco@chromium.org changed reviewers:
+ pkasting@chromium.org

[Stephen White, 2014-11-17 20:34:00]

+pkasting

[Stephen White, 2014-11-17 20:43:05]

Leaving for Peter.

https://codereview.chromium.org/733063005/diff/1/Source/platform/image-decode...
File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):

https://codereview.chromium.org/733063005/diff/1/Source/platform/image-decode...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:318: entry.m_imageSize =
readUint32(8);
I'm probably just showing my ignorance of this code, but how was this "working"
before, if all ICOs have this field? Were we reading the size into
m_imageOffset, and ignoring the last field?

[Stephen White, 2014-11-17 20:46:46]

On 2014/11/17 20:43:05, Stephen White wrote:
> Leaving for Peter.
> 
>
https://codereview.chromium.org/733063005/diff/1/Source/platform/image-decode...
> File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):
> 
>
https://codereview.chromium.org/733063005/diff/1/Source/platform/image-decode...
> Source/platform/image-decoders/ico/ICOImageDecoder.cpp:318: entry.m_imageSize
=
> readUint32(8);
> I'm probably just showing my ignorance of this code, but how was this
"working"
> before, if all ICOs have this field? Were we reading the size into
> m_imageOffset, and ignoring the last field?

I see now that the read functions take an offset. So I guess we were reading the
imageOffset correctly, just not 
using the image size?

[Peter Kasting, 2014-11-17 20:47:15]

On 2014/11/17 20:46:46, Stephen White wrote:
> On 2014/11/17 20:43:05, Stephen White wrote:
> > Leaving for Peter.
> > 
> >
>
https://codereview.chromium.org/733063005/diff/1/Source/platform/image-decode...
> > File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):
> > 
> >
>
https://codereview.chromium.org/733063005/diff/1/Source/platform/image-decode...
> > Source/platform/image-decoders/ico/ICOImageDecoder.cpp:318:
entry.m_imageSize
> =
> > readUint32(8);
> > I'm probably just showing my ignorance of this code, but how was this
> "working"
> > before, if all ICOs have this field? Were we reading the size into
> > m_imageOffset, and ignoring the last field?
> 
> I see now that the read functions take an offset. So I guess we were reading
the
> imageOffset correctly, just not 
> using the image size?

Correct.

[Peter Kasting, 2014-11-17 20:59:32]

Ugh.

What about the bitmap info header for this entry:
* Is biHeight still doubled like it normally is for ICOs with AND masks?  If
not, we're going to adjust it wrong in BMPImageReader::readInfoHeader().
* Does the biSize field exist and contain the correct value?  Perhaps
BMPImageReader should clamp to at most that many bytes if so?  What if both are
specified and the two disagree?  What if one or both is zero, how do other
decoders behave?

https://codereview.chromium.org/733063005/diff/20001/Source/platform/image-de...
File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):

https://codereview.chromium.org/733063005/diff/20001/Source/platform/image-de...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:322: entry.m_imageSize =
std::min(entry.m_imageSize, m_data->size() - entry.m_imageOffset);
It's not safe to clamp to m_data->size() here; the remaining file data may not
have streamed in by this point.

[Sami, 2014-11-18 11:43:27]

On 2014/11/17 20:59:32, Peter Kasting wrote:
> Ugh.
> 
> What about the bitmap info header for this entry:
> * Is biHeight still doubled like it normally is for ICOs with AND masks?  If
> not, we're going to adjust it wrong in BMPImageReader::readInfoHeader().

Yes, the y size is still doubled. The only bug is that the section ends right
when the AND bitmap data should begin.

> * Does the biSize field exist and contain the correct value?  Perhaps
> BMPImageReader should clamp to at most that many bytes if so?  What if both
are
> specified and the two disagree?  What if one or both is zero, how do other
> decoders behave?

biSize exists and is set to 40 bytes (the header size). If you mean biSizeImage,
it's set to 0 which seems to be the custom for uncompressed icons. I think
looking at the section size is the only way to work around this breakage :\ In
any case, it looks like we always completely ignore biSizeImage which is
probably safer.

For this particular problem where the section cuts off prematurely, according to
the bug report other browsers assume the icon is completely opaque.
 
>
https://codereview.chromium.org/733063005/diff/20001/Source/platform/image-de...
> File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):
> 
>
https://codereview.chromium.org/733063005/diff/20001/Source/platform/image-de...
> Source/platform/image-decoders/ico/ICOImageDecoder.cpp:322: entry.m_imageSize
=
> std::min(entry.m_imageSize, m_data->size() - entry.m_imageOffset);
> It's not safe to clamp to m_data->size() here; the remaining file data may not
> have streamed in by this point.

Ah, good point. Moved it to right before the BMP decoding.

[Sami, 2014-11-18 11:44:15]

On 2014/11/17 20:46:46, Stephen White wrote:
> I see now that the read functions take an offset. So I guess we were reading
the
> imageOffset correctly, just not 
> using the image size?

That's right, the offset was read correctly but the size was implicitly set to
the entire size of the file.

[Peter Kasting, 2014-11-18 20:59:05]

https://codereview.chromium.org/733063005/diff/40001/Source/platform/image-de...
File Source/platform/image-decoders/ico/ICOImageDecoder.cpp (right):

https://codereview.chromium.org/733063005/diff/40001/Source/platform/image-de...
Source/platform/image-decoders/ico/ICOImageDecoder.cpp:223:
m_bmpReaders[index]->setData(bmpData.get());
I have three concerns with this code, in decreasing order.

(1) We also call setData() on the BMP reader in ICOImageDecoder::setData()
above, for when e.g. more data comes in after an initial decode attempt.  If we
want to be correct, we have to either mirror this clamp there, or we have to
instead inform the BMP decoder about the size limits so it will always avoid
running over its data limit.

(2) Is the image size in the directory entry always trustworthy?  Do other
browsers always use it, do they ignore it if it's 0, do they use some other
number to calculate this limit?  There are so many malformed images on the web
that I'm worried about this patch fixing this case and breaking others.  Perhaps
instead of trusting the image size field, we should instead clamp to "however
much data we have before the start of the next entry"?  That seems potentially
safer.

(3) There's also a setDataForPNGDecoderAtIndex() function above.  You're only
clamping for BMP.  Perhaps we should also clamp for other types of embedded
images?

[Sami, 2014-11-21 19:36:47]

Thanks for the good ideas.

On 2014/11/18 20:59:05, Peter Kasting wrote:
> (1) We also call setData() on the BMP reader in ICOImageDecoder::setData()
> above, for when e.g. more data comes in after an initial decode attempt.  If
we
> want to be correct, we have to either mirror this clamp there, or we have to
> instead inform the BMP decoder about the size limits so it will always avoid
> running over its data limit.
>
> (2) Is the image size in the directory entry always trustworthy?  Do other
> browsers always use it, do they ignore it if it's 0, do they use some other
> number to calculate this limit?  There are so many malformed images on the web
> that I'm worried about this patch fixing this case and breaking others. 
Perhaps
> instead of trusting the image size field, we should instead clamp to "however
> much data we have before the start of the next entry"?  That seems potentially
> safer.

I had a look at what Mozilla is doing. Their implementation[1] seems to scan the
BMP data and ignore the AND mask if the bitmap data already had some non-zero
alpha values. I've now changed this patch to do something similar. Turns out the
real bug was the optimization here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(Clamping to the next entry wouldn't work when there's only one entry like in
the attached test file.)

> (3) There's also a setDataForPNGDecoderAtIndex() function above.  You're only
> clamping for BMP.  Perhaps we should also clamp for other types of embedded
> images?

I wrote an intermediate version that did clamping for PNG too (patch set 4), but
now I'm not doing any clamping any longer.

[1]
http://dxr.mozilla.org/mozilla-central/source/image/decoders/nsICODecoder.cpp...

[Peter Kasting, 2014-11-21 20:05:54]

If we put artificially truncated values into the image size fields you were
previously clamping to, do other browsers and image viewers respect them or
ignore them?  If they respect them, perhaps we need both changes.

https://codereview.chromium.org/733063005/diff/80001/Source/platform/image-de...
File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right):

https://codereview.chromium.org/733063005/diff/80001/Source/platform/image-de...
Source/platform/image-decoders/bmp/BMPImageReader.cpp:141: if (m_isInICO &&
!m_decodingAndMask && !m_buffer->hasAlpha() && !m_seenNonZeroAlphaPixel) {
You shouldn't need to check m_buffer->hasAlpha() anymore.

I wonder if you need to check m_bitMasks[3], though:

  ... && (!m_bitMasks[3] || !m_seenNonZeroAlphaPixel)

If m_bitMasks[3] is zero, the image has explicitly said it has no alpha channel.
 In this case, m_seenNonZeroAlphaPixel is always going to be true, since we
pretend the alpha value is always 255.  But I think you want to use the AND mask
anyway.  I don't know if Mozilla does this -- it looks to me like they may not
handle BI_BITFIELDS correctly for 32bpp images (but rather assume the image is
always in 8888 form).  But it might be worth creating a test image that is 32bpp
with a zero-width alpha channel and seeing whether e.g. Windows respects the AND
mask.  We could use that in a layout test too.

[Sami, 2014-11-24 13:53:00]

On 2014/11/21 20:05:54, Peter Kasting wrote:
> If we put artificially truncated values into the image size fields you were
> previously clamping to, do other browsers and image viewers respect them or
> ignore them?  If they respect them, perhaps we need both changes.

I did some experiments:

1. Icon with image data size field set to 0:
   - Chrome loads the icon normally.
   - Firefox loads the icon normally.
   - Gimp loads the icon normally.
   - Safari refuses to load the icon.
   - IE refuses to load the icon.

2. Icon with image data size field truncated so that the AND data is excluded:
   - Chrome loads the icon normally.
   - Firefox loads the icon normally.
   - Gimp loads the icon normally.
   - Safari loads the icon normally.
   - IE loads the icon normally.

3. Icon with image data size field truncated by half.
   - Chrome loads the icon normally.
   - Firefox loads the icon normally.
   - Gimp loads the icon normally.
   - Safari shows the icon as blank (and the file picker shows a corrupted mess
in the thumbnail).
   - IE refuses to load the icon.

Based on this only IE and Safari are paying attention to the size field with IE
being the stricter of the two. In terms of correctness it doesn't look like
using the size field is important -- the most we could do is refuse loading
icons that have inconsistent size data.
 
>
https://codereview.chromium.org/733063005/diff/80001/Source/platform/image-de...
> File Source/platform/image-decoders/bmp/BMPImageReader.cpp (right):
> 
>
https://codereview.chromium.org/733063005/diff/80001/Source/platform/image-de...
> Source/platform/image-decoders/bmp/BMPImageReader.cpp:141: if (m_isInICO &&
> !m_decodingAndMask && !m_buffer->hasAlpha() && !m_seenNonZeroAlphaPixel) {
> You shouldn't need to check m_buffer->hasAlpha() anymore.
> 
> I wonder if you need to check m_bitMasks[3], though:
> 
>   ... && (!m_bitMasks[3] || !m_seenNonZeroAlphaPixel)
> 
> If m_bitMasks[3] is zero, the image has explicitly said it has no alpha
channel.
>  In this case, m_seenNonZeroAlphaPixel is always going to be true, since we
> pretend the alpha value is always 255.  But I think you want to use the AND
mask
> anyway.  I don't know if Mozilla does this -- it looks to me like they may not
> handle BI_BITFIELDS correctly for 32bpp images (but rather assume the image is
> always in 8888 form).  But it might be worth creating a test image that is
32bpp
> with a zero-width alpha channel and seeing whether e.g. Windows respects the
AND
> mask.  We could use that in a layout test too.

Checking m_bitMasks[3] == 0 seems plausible. If I understood the documentation
right, you're not supposed to have alpha data in the main bitmap when using
BI_BITFIELDS. I couldn't find and app that could generate an icon with
BI_BITFIELDS, so I tried to put one together by hand. With this patch Chrome
loads it happily and applies the AND mask. Firefox interprets the top byte as
alpha and ignores the AND mask. Safari does the same. IE doesn't like the file
at all.

Based on this the most compatible option would be to not check m_bitMasks[3]. It
feels like BI_BITFIELDS support is generally shaky at best[1], so those types of
files may not be all that common anyway.

[1] http://www.virtualdub.org/blog/pivot/entry.php?id=177

[Peter Kasting, 2014-11-24 20:49:11]

Thanks for doing all the research here.  I'm comfortable ignoring the size
field.  For the alpha computation, despite Firefox and Safari's behavior, I
think I would feel more comfortable with the check I proposed:

... && (!m_bitMasks[3] || !m_seenNonZeroAlphaPixel)

I think, as you note, use of BI_BITFIELDS is relatively rare, so the practical
effect should be minimal, especially since this will only affect cases that not
only use BI_BITFIELDS, but have unused space that would normally be considered
an alpha channel save for the alpha bitmask of 0, and then put nonzero data into
that field.  In this obscure case, we're already ignoring the alpha channel in
the processXXX() functions (the bitmask of zero means we always treat alpha as
0xff regardless of what's in the image data), so being consistent with that here
seems correct.  We could elect to _not_ ignore the alpha channel in this case
(and instead assume the bitmask is wrong), but I'd want to change not only this
location but the decoding loops as well, and doing so correctly seems a bit
tricky.  Absent an existing in-the-wild image that demonstrates author intent
one way or the other, we're left guessing whether Firefox' behavior is actually
correct or buggy, so I think the change above is the safer way to go for now.

LGTM with that change done.

[Sami, 2014-11-25 10:24:22]

On 2014/11/24 20:49:11, Peter Kasting wrote:
> Thanks for doing all the research here.  I'm comfortable ignoring the size
> field.  For the alpha computation, despite Firefox and Safari's behavior, I
> think I would feel more comfortable with the check I proposed:
> 
> ... && (!m_bitMasks[3] || !m_seenNonZeroAlphaPixel)
> 
> I think, as you note, use of BI_BITFIELDS is relatively rare, so the practical
> effect should be minimal, especially since this will only affect cases that
not
> only use BI_BITFIELDS, but have unused space that would normally be considered
> an alpha channel save for the alpha bitmask of 0, and then put nonzero data
into
> that field.  In this obscure case, we're already ignoring the alpha channel in
> the processXXX() functions (the bitmask of zero means we always treat alpha as
> 0xff regardless of what's in the image data), so being consistent with that
here
> seems correct.  We could elect to _not_ ignore the alpha channel in this case
> (and instead assume the bitmask is wrong), but I'd want to change not only
this
> location but the decoding loops as well, and doing so correctly seems a bit
> tricky.  Absent an existing in-the-wild image that demonstrates author intent
> one way or the other, we're left guessing whether Firefox' behavior is
actually
> correct or buggy, so I think the change above is the safer way to go for now.

Okay, that sounds reasonable. If we come across an image that violates these
assumptions we can always revisit this.

> LGTM with that change done.

Done. Thanks for the thorough review!

Stephen, could I get your rubber stamp to land this?

[Stephen White, 2014-11-25 13:03:35]

On 2014/11/25 10:24:22, Sami wrote:
> On 2014/11/24 20:49:11, Peter Kasting wrote:
> > Thanks for doing all the research here.  I'm comfortable ignoring the size
> > field.  For the alpha computation, despite Firefox and Safari's behavior, I
> > think I would feel more comfortable with the check I proposed:
> > 
> > ... && (!m_bitMasks[3] || !m_seenNonZeroAlphaPixel)
> > 
> > I think, as you note, use of BI_BITFIELDS is relatively rare, so the
practical
> > effect should be minimal, especially since this will only affect cases that
> not
> > only use BI_BITFIELDS, but have unused space that would normally be
considered
> > an alpha channel save for the alpha bitmask of 0, and then put nonzero data
> into
> > that field.  In this obscure case, we're already ignoring the alpha channel
in
> > the processXXX() functions (the bitmask of zero means we always treat alpha
as
> > 0xff regardless of what's in the image data), so being consistent with that
> here
> > seems correct.  We could elect to _not_ ignore the alpha channel in this
case
> > (and instead assume the bitmask is wrong), but I'd want to change not only
> this
> > location but the decoding loops as well, and doing so correctly seems a bit
> > tricky.  Absent an existing in-the-wild image that demonstrates author
intent
> > one way or the other, we're left guessing whether Firefox' behavior is
> actually
> > correct or buggy, so I think the change above is the safer way to go for
now.
> 
> Okay, that sounds reasonable. If we come across an image that violates these
> assumptions we can always revisit this.
> 
> > LGTM with that change done.
> 
> Done. Thanks for the thorough review!
> 
> Stephen, could I get your rubber stamp to land this?

LGTM (bots willing)

[Sami, 2014-11-25 13:28:05]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-11-25 13:52:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/733063005/140001

[commit-bot: I haz the power, 2014-11-25 13:52:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 13:52:22]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
41327b8985ace31a6b598bfc4bed1e0e83852f44..bd0b8e604f7728069fc2e48d2dfe0e129b07e6c6
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
41327b8985ace31a6b598bfc4bed1e0e83852f44..bd0b8e604f7728069fc2e48d2dfe0e129b07e6c6
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1379,6 +1379,8 @@ crbug.com/385014
accessibility/canvas-fallback-content-2.html [ Failure ]
 crbug.com/385014 accessibility/clickable.html [ Failure ]
 crbug.com/385014 accessibility/nochildren-elements.html [ Failure ]
 
+crbug.com/424272 fast/images/icon-decoding.html [ NeedsRebaseline ]
+
 crbug.com/169564 accessibility/canvas-fallback-content.html [ Failure ]
 crbug.com/169564 accessibility/canvas-fallback-content-labels.html [ Failure ]
 crbug.com/169564 accessibility/canvas-accessibilitynodeobject.html [ Failure ]

[Sami, 2014-11-25 13:54:39]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-11-25 14:56:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/733063005/160001

[commit-bot: I haz the power, 2014-11-25 14:56:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 14:56:15]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: LayoutTests/TestExpectations
  |diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
  |index
f32d5ce8af5bd83074bf796acfb6a6279e963908..d3c314cfbc89b1853f51ab46884f44c009485e25
100644
  |--- a/LayoutTests/TestExpectations
  |+++ b/LayoutTests/TestExpectations
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
f32d5ce8af5bd83074bf796acfb6a6279e963908..d3c314cfbc89b1853f51ab46884f44c009485e25
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1379,6 +1379,8 @@ crbug.com/385014
accessibility/canvas-fallback-content-2.html [ Failure ]
 crbug.com/385014 accessibility/clickable.html [ Failure ]
 crbug.com/385014 accessibility/nochildren-elements.html [ Failure ]
 
+crbug.com/424272 fast/images/icon-decoding.html [ NeedsRebaseline ]
+
 crbug.com/169564 accessibility/canvas-fallback-content.html [ Failure ]
 crbug.com/169564 accessibility/canvas-fallback-content-labels.html [ Failure ]
 crbug.com/169564 accessibility/canvas-accessibilitynodeobject.html [ Failure ]

[Sami, 2014-11-25 15:04:40]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-11-25 15:11:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/733063005/180001

[commit-bot: I haz the power, 2014-11-25 16:31:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-25 16:31:43]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/38388)

[Sami, 2014-11-25 16:41:39]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-11-25 16:42:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/733063005/200001

[commit-bot: I haz the power, 2014-11-25 17:56:30]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185957