624133 - Update libpng to version 1.4.7

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Glenn Randers-Pehrson]

Libpng-1.5.0 has been released.  We should update Mozilla's embedded libpng to version 1.5.0, but there are no security or performance issues for Mozilla that I know of, so there's no hurry.

Comment 1

[Glenn Randers-Pehrson]

It seems that libpng-1.5.0 may have a bug with regard to decoding interlaced PNG images, so let's wait for 1.5.1.

Comment 2

[Glenn Randers-Pehrson]

Changed title because we will be waiting for 1.5.1.

Comment 3

[Glenn Randers-Pehrson]

The bug was not with interlaced images but with the png_rgb_to_gray
transformation, which we don't use.  But 1.5.1 will be out soon so
we might as well wait anyhow.

Comment 4

[Max Stepin]

Attachment: png_handle_fcTL() fix

Glenn, please add this fix for png_handle_fcTL() too.

It fixed a problem which does *not* affect Firefox, but can cause trouble for people who would install Mozilla's version of libpng as a system library.

Comment 5

[NVD]

Libpng 1.5.2 has been released, can we please get it in to the trunk before Firefox 5.0 ships?

http://libpng.sourceforge.net/

"UPDATE 31 March 2011: The latest released versions are libpng-1.5.2"

Comment 6

[Glenn Randers-Pehrson]

The reduced libpng that we use doesn't require anything new from libpng-1.5.2.
We do need to apply the patch in bug #644519 though so users can run with a system libpng-1.5.x.

Comment 7

[NVD]

Bug 645519 is fixed now, would it be possible to update to libpng 1.5.2 before the code freeze for Firefox 5.0 on the 12th?

Comment 8

[Glenn Randers-Pehrson]

Probably not.  Applying the diff of system libpng-1.4.3 to the embedded apng-supporting libpng results in .rej files totaling 3793 lines.  So there's
a lot of manual slogging through that to be done and zero payoff, since as I
have said before, libpng-1.5.x doesn't offer anything new for mozilla.

Rushing through a big job like that would just open us up to mistakes and
possible security problems.

On the other hand, upgrading to libpng-1.4.6 would be fairly simple and
that is what I recommend.  I'd need help from someone who is able and
willing to to do triserver tests.  libpng-1.4.6 is scheduled for release
next week but we can test with libpng-1.4.6rc02 now.

Comment 9

[Glenn Randers-Pehrson]

Attachment: v00 Ugrade to libpng-1.4.6

Comment 10

[Glenn Randers-Pehrson]

Note the v00 patch internally calls itself libpng-1.4.6rc02.  This is just for testing; a final patch will call itself libpng-1.4.6.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

I'll run this through Try for you tonight (EDT) when I get home from work.

Comment 12

[Glenn Randers-Pehrson]

I've asked the PNG group for permission to push 1.4.6 out a little early (it's scheduled for April 14 but is ready to go now).

Comment 13

[Glenn Randers-Pehrson]

(In reply to comment #4)
> Created attachment 509715 [details] [diff] [review]
> png_handle_fcTL() fix
> 
> Glenn, please add this fix for png_handle_fcTL() too.

@max, It's in the v00 patch.

Comment 14

[Glenn Randers-Pehrson]

Attachment: v01 Ugrade to libpng-1.4.6

Libpng-1.4.6 has been released.

Comment 15

[Glenn Randers-Pehrson]

@Ryan, there was one bug in v00's png.h (a stray "};" )
that's fixed in the v01 patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Bug 645519 landed on trunk, so the changes to nsPNGDecoder.cpp are already on mozilla-central. Also, apng.diff is not in the Mozilla tree, so I got rid of that as well. Pushed to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=8d146b477ab0

Builds will be available at the link below once finished:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8d146b477ab0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: libpng 1.4.6 v1 patch

As pushed to Try

Comment 18

[Glenn Randers-Pehrson]

Attachment: v02 Upgrade to libpng-1.4.6

Removed stray apng.diff file and the nsPNGDecoder.cpp changes.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Glenn, the diff I posted was in ready to apply hg mq format with proper attribution.

Comment 20

[Glenn Randers-Pehrson]

Oops, I thought I was making my own v01 patch obsolete!

I don't see a way to un-obsolete your v1 patch and
make my v02 obsolete.  If you know how, go ahead and do that.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 524788 [details] [diff] [review]
v02 Upgrade to libpng-1.4.6

You have to click the Edit link on the patch details to mark them obsolete. It changed with one of the recent Bugzilla updates.

Comment 22

[Glenn Randers-Pehrson]

Ok thanks.  I thought that button was just for editing the patch title.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Doesn't build on Windows:
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(711) : error C2143: syntax error : missing ';' before 'type'
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(712) : error C2143: syntax error : missing ';' before 'type'
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(713) : error C2065: 'half_z_window_size' : undeclared identifier
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(713) : warning C4018: '<=' : signed/unsigned mismatch
e:/builds/moz2_slave/try-w32/build/modules/libimg/png/pngwutil.c(716) : error C2065: 'z_cinfo' : undeclared identifier

Comment 24

[Ryan VanderMeulen [:RyanVM]]

I see the failure locally as well. Apparently MSVC doesn't like declaring variables inline?
http://andre.stechert.org/urwhatu/2006/01/error_c2143_syn.html

Comment 25

[Glenn Randers-Pehrson]

Ack.  Fixed that in libpng-1.5.2 last week.

Comment 26

[Glenn Randers-Pehrson]

The fix is being tested in libpng-1.4.7rc01.  Libpng-1.4.7 will be released once I see test results for the various platforms.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Seeing this error now repeatedly:
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(651) : error C2144: syntax error : 'int' should be preceded by ';'
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(651) : error C2208: 'int' : no members defined using this type
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(652) : error C2144: syntax error : 'int' should be preceded by ';'
e:\builds\moz2_slave\try-w32\build\obj-firefox\dist\include\png.h(652) : error C2208: 'int' : no members defined using this type

etc...

I'm seeing the same errors locally on a clobber build. This error likely predates the 1.4.7 patch but wasn't hit because of the other build failure. It appears that pngwutil.cpp builds without error now at least.

Comment 28

[Glenn Randers-Pehrson]

What compiler issued these errors?

Not sure what is happening here.  Can you try a build with
PNG_NO_PEDANTIC_WARNINGS defined in mozpngconf.h?  png.h line 651
is the first place PNG_DEPSTRUCT is used, and this will be blank with
PNG_NO_PEDANTIC_WARNINGS defined.

You can put the definition in mozpngconf.h

Comment 29

[Glenn Randers-Pehrson]

This is probably relevant.  The PNG_DEPSTRUCT was blank for
all versions of MSC prior to 1.4.6.  See the libpng CHANGES
file:

version 1.4.6beta07 [March 22, 2011]
  Added attribute definition (warnings) for MSC_VER >= 1300 in pngconf.h

Comment 30

[Ryan VanderMeulen [:RyanVM]]

I'm building with MSVC 2010 SP1. I'm doing a local build now with PNG_NO_PEDANTIC_WARNINGS defined in mozpngconf.h.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Attachment: libpng 1.4.7 v1 patch

Now we're in business. My local build succeeded with the mozpngconf.h change. Pushed to try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=69ea57309066

Builds when ready:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-69ea57309066

Glenn, I just arbitrarily added the define near the top. Not sure if that's exactly how you'd want it for a final check-in.

Comment 32

[Glenn Randers-Pehrson]

It doesn't make much difference where you put it.  I'll put it near the
top with a comment in the final version.

Do you know what MSC_VER is defined to on the compiler that emitted the
error messages?  In pngconf.h we turn on the PNG_DEPSTRUCT and other
attributes when MSC_VER >= 1300.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Mozilla uses MSVC 2005 for their official builds (_MSC_VER = 1400). Plan is to upgrade to MSVC 2010 in the future (once they can sort out building jemalloc with it), which is _MSC_VER = 1600.

Comment 34

[Glenn Randers-Pehrson]

Thanks.  I've passed that info along to the PNG developers.
I think we should play it safe and not use the feature for any
MSC version:

     in mozlibpng.h

     #ifdef _MSC_VER
     #  define PNG_NO_PEDANTIC_WARNINGS
     #endif

We don't need the warnings under MSC because we'll still see them while
building our same code with gcc.

Comment 35

[Glenn Randers-Pehrson]

Attachment: v05 Upgrade to libpng-1.4.7

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Attachment: v5 patch update to libpng 1.4.7

Same as the other v05 patch minus the unwanted makefile changes. Functionally the same as the 1.4.7rc1 patch, which passed on Try. Built locally on a clobber build without issue.

Comment 37

[Glenn Randers-Pehrson]

libpng-1.4.7 has been released.

Comment 38

[Glenn Randers-Pehrson]

(In reply to comment #5 and comment #7)
> Libpng 1.5.2 has been released, can we please get it in to the trunk before
> Firefox 5.0 ships?

Libpng-1.4.7 is out and the patch is ready to go, passed Try, and is
just waiting for review. @Ryan, thanks much for helping with this.

However, if it doesn't make it into 5.0 that's not a problem.  The current
embedded libpng-1.4.3 is adequate, and neither libpng-1.4.6 nor 1.5.2
offers performance or security improvements in the parts that are
not #ifdef'ed out in mozpngconf.h

Comment 39

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/7511736d309b

Comment 40

[Max Stepin]

Glenn, the patch I suggested in comment #4 landed incorrectly.

The patch should have 3 blocks like that, but it only has 2:

-        png_crc_finish(png_ptr, length);
+        png_crc_finish(png_ptr, length-4);
         return;

I also suggested that some existing piece of code that uses info_ptr should be put inside
if (info_ptr != NULL)
{
}

but it also landed a little bit incorrectly. Part of that code ended up both inside and outside. Could you check this, please?

Comment 41

[Glenn Randers-Pehrson]

(In reply to comment #40)
> Glenn, the patch I suggested in comment #4 landed incorrectly.
> ... Could you check this, please?

Yes, it's incorrect but fairly harmless.  The wrong length on
png_crc_finish() will cause a png_error when libpng attempts
to read the next chunk.  The extra code inside the if{} is
harmless except for the increase in the size of the executable.

I'll try to generate a new patch today (against the current trunk
which seems to be still at libpng-1.4.3) but I think it has missed
the Firefox-5.0 cutoff.

Comment 42

[Glenn Randers-Pehrson]

Attachment: v06 Upgrade to libpng-1.4.7

Revised per Max's comment.  Patch is against trunk (assumes v5 has not
been applied).

Comment 43

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/7511736d309b

Comment 44

[(no longer active)]

(In reply to comment #42)
> Revised per Max's comment.  Patch is against trunk (assumes v5 has not
> been applied).

Can you please attach a patch on top of the current trunk which addresses comment 41?

Comment 45

[Glenn Randers-Pehrson]

@ehsan, hopefully this evening (EST), don't have access to my files and tools right now.

Comment 46

[Glenn Randers-Pehrson]

While making a new patch I will also fix the fact that, when info_ptr is
NULL, max's (and the v06) patch still returns success (sets a mode bit).
It shouldn't set the mode bit in this case.  This error does not affect
Firefox, though.  The fix is easy, just move the mode-setting line inside
the if{}.

Comment 47

[(no longer active)]

Is this patch as landed safe to get on a nightly?  If not, I can just back it out and reland the fixed version...

Comment 48

[Glenn Randers-Pehrson]

Attachment: v07 Fix coding errors in png_handle_fcTL

See comment #40 and comment #46
This patch assumes the v5 patch has already been applied.

Comment 49

[Glenn Randers-Pehrson]

(In reply to comment #47)
> Is this patch as landed safe to get on a nightly?  If not, I can just back it
> out and reland the fixed version...

That's OK.  I made a fresh clone and built the small v07 patch against that.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

v07 builds fine locally. Running through Try:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=bf80756497cb

Comment 51

[Glenn Randers-Pehrson]

Both the v5 and v07 patches have been checked in to mozilla-central a while ago.

Comment 52

[Glenn Randers-Pehrson]

(In reply to comment #51)
> Both the v5 and v07 patches have been checked in to mozilla-central a while
> ago.

Correction: only the v5 patch was checked in.  Reopening.

Comment 53

[Glenn Randers-Pehrson]

checkin-needed (of the v07 patch only)

Comment 54

[Boris Zbarsky [:bzbarsky]]

Pushed v07: http://hg.mozilla.org/integration/mozilla-inbound/rev/88329c72d50c

Comment 55

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/88329c72d50c

Please check status of the bug, esp target milestone and such, since it's a bit unclear to me.