346927 - drawImage corrupts transparent 24-bit PNGs with 1-bit-convertible alpha

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Philip Taylor]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060801 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b1) Gecko/20060801 BonEcho/2.0b1

When drawing a 24-bit PNG (not indexed colour) with an alpha channel that can be represented in 1 bit (i.e. it has some 0, some 255, and nothing in between), the output is corrupted - it appears to squash the alpha channel sideways, and repeat it eight times across the width of the image. It looks very much like it's packing the alpha channel into bits, and then reading it back as bytes (plus reading random garbage towards the bottom of the picture when it reads past the alpha channel's end, which could perhaps be considered a (rather minor) security issue when combined with getImageData...)

This works correctly on Windows, but fails on Linux. I don't believe it is the same as bug 320510 - that affected indexed-colour PNGs/GIFs on Windows and seems fixed in Firefox 2.0, whereas this affects 24-bit PNGs on Linux in FF1.5 and 2.0. (And between the two different problems, it's a bit tricky making games with any kind of masked image that they can correctly draw in multiple browser versions, which makes sprites fun ;-) )

Reproducible: Always

Steps to Reproduce:
1. View the forthcoming test case.


Actual Results:  
An approximation of a slightly ugly circular smiley face, but with seven chunks taken out of the top and bottom (between the eight repetitions of the circular alpha channel).

Expected Results:  
A slightly ugly circular smiley face.

Comment 1

[Philip Taylor]

Attachment: image for test case

Comment 2

[Philip Taylor]

Attachment: test case

Comment 3

[Philip Taylor]

Attachment: unexpected output

Comment 4

[Boris Zbarsky [:bzbarsky]]

I can confirm with a non-cairo build, but the problem seems to be gone in cairo ones.  Philip, can you confirm that with a trunk Firefox build?

Comment 5

[Philip Taylor]

Confirmed - the test case works correctly for me in
  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060908 Minefield/3.0a1
It still fails (as in the "unexpected output" case) with
  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1b2) Gecko/20060908 BonEcho/2.0b2
and fails the same with
  Mozilla/5.0 (X11; U; Linux i686; en-GB; rv:1.8.0.5) Gecko/20060731 Ubuntu/dapper-security Firefox/1.5.0.5

The situation where I encountered this problem (the sprites in http://canvex.lazyilluminati.com/) has the same behaviour, working on the trunk but not on the earlier branches.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, excellent.  Marking worksforme.

Comment 7

[Philip Taylor]

Attachment: image for new test case

Comment 8

[Philip Taylor]

Attachment: new test case

Comment 9

[Philip Taylor]

I've added a new test case (and reopened the bug) because it seems to cause a not-entirely-trivial security problem in Firefox 2.0 (on Linux). The example has a 32767x1 image which triggers the incorrect behaviour - the alpha channel is (I assume) stored in 32K bits, but 32K bytes are drawn, with most of them being whatever happens to be in the heap after the image data. getImageData allows that data to be read back by the script (which works anywhere on the web, as long as it's the same domain as the image file).

When running the test case several times, I see various bits of dictionary and URLs and font filenames and HTML contents of other tabs, which is probably not a good thing to give scripts access to.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er, yes.  That would be very very bad.  Sounds like this is a branch-only security bug at this point....

Comment 11

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 12

[Daniel Veditz [:dveditz]]

Vlad or Stuart: can one of you look at this or assign to a willing helper?

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ugh.  I'll look at this this week, it should be a straightforward fix.

Comment 14

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: grab the correct alpha depth from linux nsIImage

Stuart, does this look right?  Is there anything else that needs to be handled?

Also, do we want this code on for other platforms, or just GTK2?  As far as I can tell, nothing else lies about the image format in the same way.

Comment 15

[Stuart Parmenter]

Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

looks good to me.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

Requesting 1.8.1.1 approval; should be a low risk privacy fix.  Separate patch (but with essentially the same code) coming for 1.8.0.9.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

Actually, the patch is identical for 1.8.0.9 as well; requesting approval for there.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 246856 [details] [diff] [review]
grab the correct alpha depth from linux nsIImage

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in to 1.8.0.9 and 1.8.1.1.

Comment 20

[alice nodelman [:alice] [:anode]]

verified 'new test case' on fedora core release 5, checked that test case fails for 2.0.0.0 release.

verified 1.8.0.9
1.5.0.9pre 20061130

verified 1.8.1.1
2.0.0.1pre 2006113004