Bug 89854 - memory corruption in png loader
Summary: memory corruption in png loader
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Red Hat Linux
Classification: Retired
Component: libpng
Version: 9
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Matthias Clasen
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2003-04-28 22:03 UTC by Bill Nottingham
Modified: 2014-03-17 02:36 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2004-06-17 13:55:56 UTC
Embargoed:


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

Description Bill Nottingham 2003-04-28 22:03:32 UTC
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 22:04:07 UTC
Created attachment 91372 [details]
simple test case

Comment 2 Owen Taylor 2003-04-28 22:28:29 UTC
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.)


Comment 3 Bill Nottingham 2003-04-28 23:02:28 UTC
OK, assigning to libpng.

Comment 4 Bill Nottingham 2003-04-28 23:22:45 UTC
Hm, the attached new libpng-1.0.9

Comment 5 Bill Nottingham 2003-04-28 23:23:40 UTC
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 16:10:39 UTC
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...


Comment 7 Bill Nottingham 2003-04-29 16:13:12 UTC
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 16:31:15 UTC
Ah, that makes more sense. So, basically, the problem was that
we had a defective local fix.


Comment 9 Bill Nottingham 2003-04-29 16:35:13 UTC
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 18:40:38 UTC
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 16:42:47 UTC
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 13:36:14 UTC
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.