Bug 89854 - memory corruption in png loader
memory corruption in png loader
Product: Red Hat Linux
Classification: Retired
Component: libpng (Show other bugs)
All Linux
medium Severity medium
: ---
: ---
Assigned To: Matthias Clasen
Depends On:
  Show dependency treegraph
Reported: 2003-04-28 18:03 EDT by Bill Nottingham
Modified: 2014-03-16 22:36 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Last Closed: 2004-06-17 09:55:56 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Attachments (Terms of Use)
simple test case (220 bytes, text/plain)
2003-04-28 18:04 EDT, Bill Nottingham
no flags Details
new badchunks patch (636 bytes, text/plain)
2003-04-28 19:23 EDT, Bill Nottingham
no flags Details

  None (edit)
Description Bill Nottingham 2003-04-28 18:03:32 EDT
The attached corrupts memory, discernable under ElectricFence or valgrind.
This *appears* to be the problem behind bug 85205, as best I can tell.
Comment 1 Bill Nottingham 2003-04-28 18:04:07 EDT
Created attachment 91372 [details]
simple test case
Comment 2 Owen Taylor 2003-04-28 18:28:29 EDT

 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.)
Comment 3 Bill Nottingham 2003-04-28 19:02:28 EDT
OK, assigning to libpng.
Comment 4 Bill Nottingham 2003-04-28 19:22:45 EDT
Hm, the attached new libpng-1.0.9
Comment 5 Bill Nottingham 2003-04-28 19:23:40 EDT
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.
Comment 6 Owen Taylor 2003-04-29 12:10:39 EDT
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 
Comment 7 Bill Nottingham 2003-04-29 12:13:12 EDT
The patch is a replacement for the badchunks patch already in the SRPM... it
changes one of the function calls.
Comment 8 Owen Taylor 2003-04-29 12:31:15 EDT
Ah, that makes more sense. So, basically, the problem was that
we had a defective local fix.
Comment 9 Bill Nottingham 2003-04-29 12:35:13 EDT
It appears so... another set of eyes confirming that I was doing the right
change to the length passed would be good. :)
Comment 10 Owen Taylor 2003-04-29 14:40:38 EDT
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.
Comment 11 Matthias Clasen 2004-05-13 12:42:47 EDT
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.
Comment 12 Matthias Clasen 2004-05-19 09:36:14 EDT
I've removed the badchunks in 1.2.5-1

Note You need to log in before you can comment on or make changes to this bug.