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
Hmm, 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 libpng-1.2...
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