556117 - Either breakage or missed fix in nsPNGDecoder.cpp in 1.9.0.19

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Mike Hommey [:glandium]]

nsPNGDecoder.cpp includes a fix for bug 497056 to which I don't have access, which is why I'm checking the security box on this bug report, just in case. There is, however, a reference to bug 497056 in modules/libimg/png/MOZCHANGES, saying:
Ported performance improvements to pngrutil.c and pngpread.c from libpng-1.4.1.
Is that supposed to only be about it ?

Anyways, about the breakage or missed fix, it happens that the code changes in nsPNGDecoder.cpp are all #ifdef'ed on PNG_USER_MEM_SUPPORTED. It also appears that in modoles/libimg/png/mozpngconf.h, PNG_NO_USER_MEM is set, which means PNG_USER_MEM_SUPPORTED is never set in mozilla builds. So whatever these ifdef'ed statements are supposed to do, they don't do it on mozilla builds. If that happens to be a security fix (which the 4MB limit mentionned in a comment could suggest), the security issue is not fixed.

On the other hand, when using a system libpng library, the PNG_USER_MEM_SUPPORTED macro is defined, and we end up compiling the ifdef'ed code, and this is where it breaks, because the code uses gfxPlatform::GetCMSMode(), which is defined nowhere in 1.9.0.

So I guess all the (gfxPlatform::GetCMSMode() != eCMSMode_Off) should be replaced with (gfxPlatform::IsCMSEnabled()), though as I don't know what this code is supposed to be fixing, and as it's obviously not compiled in mozilla builds anyway, I'll just go with replacing the #ifdef's with #if 0 for now.

Comment 2

[Mike Hommey [:glandium]]

This is not bug 551438, 1.9.0.19 has not been updated with libpng 1.4.1, and the libpng version is not responsible for the definition of gfxPlatform::GetCMSMode, which comes from gfx/thebes/src/gfxPlatform.cpp.

Comment 3

[Glenn Randers-Pehrson]

It's not a dupe of bug 551458.  It is correct that the code is #ifdef'ed out when the bundled libpng is used.  The PNG_USER_MEM_SUPPORTED test is supposed to be detecting whether a system libpng is being used and if so, if the user memory replacement function capability is present.  The reporter is correct about the use of GetCMSMode() which replaced IsCMSEnabled(); the patch should be using IsCMSEnabled() instead.  What the code is supposed to be fixing is the inefficient decoding of the iCCP chunk, which is only needed when CMS is enabled.

Comment 4

[Mike Hommey [:glandium]]

Attachment: v00: Patch

I haven't tested the result yet, but this patch at least allows to build.

Comment 5

[Glenn Randers-Pehrson]

I'm running the Tryserver now with a revised version of Mike's patch that is also more suitable when the system libpng is 1.4.1 or later.

Comment 6

[Glenn Randers-Pehrson]

Attachment: v01: no eCMSMode, uses png_set_chunk_malloc_max() if available

TryServer builds are at
https://build.mozilla.org/tryserver-builds/[e-mail]-v01-556117-Mar31-noCMSMode (with bundled libpng)
and at
https://build.mozilla.org/tryserver-builds/[e-mail]-v01s-556117-Mar30-syslib-noCMSMode (with system libpng-1.2.35)

Comment 7

[Glenn Randers-Pehrson]

Comment on attachment 436184 [details] [diff] [review]
v01: no eCMSMode, uses png_set_chunk_malloc_max() if available

The v01 patch is incorrect; "!gfxPlatform::IsCMSEnabled()" should be "gfxPlatform::IsCMSEnabled()".  The v00 patch is correct.

Comment 8

[Glenn Randers-Pehrson]

Tryserver builds with Mike's v00 patch:
With bundled libpng:
https://build.mozilla.org/tryserver-builds/[e-mail]-v00-556117-mike-Apr02-noCMSMode/

With system libpng-1.2.35:
https://build.mozilla.org/tryserver-builds/[e-mail]-v00-556117-mike-1.2.35-Apr02-noCMSMode/

with system libpng-1.4.1
https://build.mozilla.org/tryserver-builds/[e-mail]-v00-556117-mike-1.4.1-Apr02-noCMSMode/

Comment 9

[Glenn Randers-Pehrson]

Comment on attachment 436048 [details] [diff] [review]
v00: Patch

I'm not a reviewer but it looks OK.  Joe, additional review?

Comment 10

[Boris Zbarsky [:bzbarsky]]

So is this ready to land?  Is something else holding this up?  Does it just need branch approvals? If so, which branches?  Is it relevant on m-c?

Comment 11

[Glenn Randers-Pehrson]

It's only for releases prior to 1.9.1,
when
    gfxPlatform::IsCMSEnabled()
was changed to
    gfxPlatform::GetCMSMode() != eCMSMode_Off)

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK.  So should there be branch approval requests?

Comment 13

[Joe Drew (not getting mail)]

I don't know. Depends on whether we care about 1.9.0 anymore.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Well, the reporter of this bug certainly does, since last I checked he's shipping it to (some) users so wants to backport security fixes to it...

Comment 15

[Mike Hommey [:glandium]]

On the other hand, I already shipped a fixed version and don't wait for approvals ;)

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK. Daniel, Mike, are we still doing 1.9.0 approvals, or should I just check this into CVS?

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 436048 [details] [diff] [review]
v00: Patch

Approved for 1.9.0.20, a=dveditz

Please add the "fixed1.9.0.20" keyword after you check in.

Comment 18

[(no longer active)]

Smokey, are you willing to land this on 1.9.0, please?

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is solely a build-bustage fix that's a no-op with in-tree libpng, right?  (I'm happy to land it; I just want to make sure I understand what the underlying bug/expected results are, in case of unexpected results ;) )

Comment 20

[(no longer active)]

(In reply to comment #19)
> This is solely a build-bustage fix that's a no-op with in-tree libpng, right? 
> (I'm happy to land it; I just want to make sure I understand what the
> underlying bug/expected results are, in case of unexpected results ;) )

I don't really know.  I just got tired of seeing this in the checkin-needed list, and I don't have a CVS tree to land this myself!  :-)

Comment 21

[Mike Hommey [:glandium]]

(In reply to comment #19)
> This is solely a build-bustage fix that's a no-op with in-tree libpng, right?

Yes.

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Checking in modules/libpr0n/decoders/png/nsPNGDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/png/nsPNGDecoder.cpp,v  <--  nsPNGDecoder.cpp
new revision: 1.87; previous revision: 1.86
done