Red Hat Bugzilla – Bug 89854
memory corruption in png loader
Last modified: 2014-03-16 22:36:00 EDT
The attached corrupts memory, discernable under ElectricFence or valgrind.
This *appears* to be the problem behind bug 85205, as best I can tell.
Created attachment 91372 [details]
simple test case
A) valgrind reports only read errors, so it's unlikely to be
your memory corruption problem.
B) The bug is pretty clearly inside libpng, though it looks like
a pain to track down. (Some sort of off-by-one error when inflating
certain types of data chunks.)
OK, assigning to libpng.
Hm, the attached new libpng-1.0.9
Created attachment 91375 [details]
new badchunks patch
The attached seems to fix my test case. It doesn't make xscreensaver work...
perhaps this is just a red herring noticed along the way.
I'm a little confused ... I was testing your test case with libpng-1.2,
and the attached patch seems to *add* code that I recognize from
The patch is a replacement for the badchunks patch already in the SRPM... it
changes one of the function calls.
Ah, that makes more sense. So, basically, the problem was that
we had a defective local fix.
It appears so... another set of eyes confirming that I was doing the right
change to the length passed would be good. :)
Frankly, the badchunks patch doesn't make a bit of sense to me to
start with. I don't claim to understand the libpng code,
but, for example:
- Why does it make sense to call png_set_iCCP twice?
- If chunkdata can be NULL on return from png_decompress_chunk,
then the routine is going to segfault further on.
and if it is a legitimate bugfix, that we have had in our packages
since 7.1, with multiple version upgrades, why isn't it upstream???
The ChangeLog entry for it is:
- Copy SuSE 7.0's patch to handle bad chunks
Which doesn't give much background. But my instinct would be that
this patch might be entirely bogus.
The badchunks patch doesn't make sense to me either. Since it uses
NULL for the profile argument, the call in the else branch is a NOP
anyway, it just returns. And, as Owen points out, calling png_set_iCCP
twicee is dubious, even more so as the call inserted by the badchunks
patch uses a wrong profile_len...
png_decompress_chunk can't return NULL either.
I've removed the badchunks in 1.2.5-1