Bug 296305 - kdelibs-4.8.1/khtml/imload/decoders/pngloader.cpp contains duplicate call to png_start_read_image
Summary: kdelibs-4.8.1/khtml/imload/decoders/pngloader.cpp contains duplicate call to ...
Status: RESOLVED FIXED
Alias: None
Product: konqueror
Classification: Applications
Component: khtml (show other bugs)
Version: unspecified
Platform: Gentoo Packages Linux
: NOR normal
Target Milestone: ---
Assignee: Konqueror Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-18 20:02 UTC by John Bowler
Modified: 2012-04-14 20:57 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.8.3


Attachments
Removes extraneous call to png_start_read_image (465 bytes, patch)
2012-03-18 20:42 UTC, Michael Pyne
Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Bowler 2012-03-18 20:02:41 UTC
Line 194 of the git head (revision aeb7ff3b) should be deleted; png_read_update_info calls png_read_start_image, the second call is harmless (I believe) but is unexpected by libpng and does unnecessary work.

There's no warning prior to libpng-1.6.0beta18, beta18 is calling png_error so konqueror (for example) won't display PNGs, the final release will almost certainly just emit a warning message; libpng can't distinguish a genuine bug where an app tries to make a second call to re-do the initialization from when pngloader.cpp is doing.
Comment 1 John Bowler 2012-03-18 20:07:10 UTC
In fact it is more serious than I thought; each call attempts to initialize the transformations and these make permanent changes to the png_struct data, a second call can't be guaranteed not to mess things up.

I'll probably find a fix for 1.6, but the issue will still be around in earlier versions (and 1.6 won't be released for some time.)
Comment 2 Christoph Feck 2012-03-18 20:26:42 UTC
Can line "png_start_read_image" be deleted even when compiling against older versions of libpng, or do we need conditional code?
Comment 3 Michael Pyne 2012-03-18 20:37:29 UTC
John: Your proposed solution works here (I'll attach the simple patch separately). One question I have is how to know that the two calls shouldn't be mixed. I don't have libpng experience and I can't really seem to find good 'precautions' about these two functions by searching online. Best I've found is the suggestion that you only have to call one or the other.

I'm CC'ing one of the KHTML devs to get this looked at as well, thanks for the report.
Comment 4 Michael Pyne 2012-03-18 20:37:53 UTC
Adding Maks
Comment 5 Michael Pyne 2012-03-18 20:42:24 UTC
Created attachment 69718 [details]
Removes extraneous call to png_start_read_image

The patch against kdelibs.
Comment 6 John Bowler 2012-03-18 21:35:44 UTC
The API has always been 'either png_read_update_info OR png_start_read_image'.  It's always safe to call png_read_update_info.  It is only safe to call png_start_read_image instead of png_read_update_info if you don't do any transforms on the image.  Because you do ask for transforms (e.g. swapping the byte order of RGB data) you need to call it.  If you don't png_get_rowbytes will return information about the memory required for rows of the original, untransformed PNG rows.

In fact the calls are technically optional if no transformations are done.  However having libpng handle interlaced PNG data is a transformation and means that one or other of the functions must be called if png_set_interlace_handling is called.  This means that, in practice, you must call one of the routines (since you call png_set_interlace_handling for interlaced images.)
Comment 7 Christoph Feck 2012-03-18 22:32:40 UTC
Thanks for the clarification, John.

I suggest Michael commits the patch to 4.8 branch. I tested with PNG 1.4.9, and still works.
Comment 8 Michael Pyne 2012-04-14 20:57:35 UTC
Git commit a5cc5111d6dd640e63779e0bb46ca75dee80f714 by Michael Pyne.
Committed on 18/03/2012 at 21:28.
Pushed by mpyne into branch 'KDE/4.8'.

imload_png: Remove incorrect extra libpng call.

KHTML's PNG decoder has an apparently extraneous call to a libpng
function, as reported by libpng developer John Bowler.

png_read_update_info already does the work required by
png_read_start_image. This extra work causes the upcoming
libpng-1.6.0-beta18 to fail to load images at all (they will probably
downgrade this to a warning before the 1.6.0 release, but should still
be fixed).

Unfortunately I neglected to commit this when the patch was first
approved, so it will not be present in a release until 4.8.3.
FIXED-IN:4.8.3

M  +0    -1    khtml/imload/decoders/pngloader.cpp

http://commits.kde.org/kdelibs/a5cc5111d6dd640e63779e0bb46ca75dee80f714