Crash in Chromium 4.0.251 on gentoo with libpng 1.4.0 beta102

[John Bowler]

I'm seeing a SIGSEGV from fflush called from png_write_end when I let chrome
idle (I think it is probably caching PNGs in the background, probably for
the thumbnails page.)

Chromium needs minimal patches to remove the use of _NULL pointer values (no
longer in 1.4), but apart from that it's a straight build with just the
standard gentoo patches.

I don't have a debug build (this is a straight gentoo build - so the symbols
get thrown away.)  Anyone out there building chromium?  (It will probably
repro on Ubuntu too.)

My guess is that this is exposing either a libpng 1.4 bug or a genuine
compatibility issue (i.e. something bad that used to work in 1.2 and crashes
1.4).

John Bowler <jbowler@...>


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[John Bowler]

From: John Bowler [mailto:jbowler@...]
>I'm seeing a SIGSEGV from fflush called from png_write_end when I let
chrome
>idle.

This is apparently because png_flush has not been removed from 1.4.0,
contrary to the comments in 1.2/pngwrite.c.  Rather the crashing bug caused
by calling png_flush in 1.2 (and then removed by removing the call) has been
reintroduced.

Png_flush and all the flush function stuff needs to be removed from 1.4.0.

John Bowler <jbowler@...>



------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[John Bowler]

From: John Bowler [mailto:jbowler@...]
>Png_flush and all the flush function stuff needs to be removed from 1.4.0.

Wrong, just the png_flush call at the end of png_write_end need be removed.
There is a bug in chromium too, and it probably exists in several
applications because the comments in png.h are self-contrdictory.  Here's
the chromium code:

http://src.chromium.org/viewvc/chrome/releases/4.0.251.0/src/app/gfx/codec/p
ng_codec.cc?revision=32321&view=markup

In gfx::PNGCodec::Encode it does this:

  // Set our callback for libpng to give us the data.
  PngEncoderState state(output);
  png_set_write_fn(png_ptr, &state, EncoderWriteCallback, NULL);

So you can see the 'io_ptr' is a PNGEncoderState* and 'NULL' has been passed
as the flush function.  Then in libpng pngwio.c:

http://libpng.git.sourceforge.net/git/gitweb.cgi?p=libpng/libpng;a=blob;f=pn
gwio.c;h=401fa77118e4945abd8c134bf07b748840f9799c;hb=cd7d63a805a7eada2675626
b2edc690dd17e1ee5

 127 png_default_flush(png_structp png_ptr)
 128 {
 129    png_FILE_p io_ptr;
 130    if (png_ptr == NULL)
 131       return;
 132    io_ptr = (png_FILE_p)CVT_PTR((png_ptr->io_ptr));
 133    fflush(io_ptr);
 134 }

And a (PNGEncoderState*) is cast as a (FILE*) hence the fflush crash.

The chromium code writer apparently believed the second sentence in the
comment for png_set_write_fn in png.h:

1800 /* Replace the default data output functions with a user supplied
one(s).
1801  * If buffered output is not used, then output_flush_fn can be set to
NULL.
________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
^^^^

This is not true.  The sentence should read:

"If io_ptr is not a png_FILE_p then both the write function and flush
function must be provided.  If io_ptr *IS* a png_FILE_p then either or both
may be NULL - the built in defaults will be used."

The explanation that follows does actually say this but in a way that is
only comprehensible if you have detailed knowledge of the library.

John Bowler <jbowler@...>


------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[John Bowler-3]

I've attached the patch for libpng 1.4.0 beta102 - verified against chromium
4.0.251 on gentoo.

John Bowler <jbowler@...>



------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[Glenn Randers-Pehrson]

On Fri, Nov 20, 2009 at 6:56 PM, John Bowler <jbowler@...> wrote:
> From: John Bowler [mailto:jbowler@...]
>>I'm seeing a SIGSEGV from fflush called from png_write_end when I let
> chrome
>>idle.
>
> This is apparently because png_flush has not been removed from 1.4.0,
> contrary to the comments in 1.2/pngwrite.c.  Rather the crashing bug caused
> by calling png_flush in 1.2 (and then removed by removing the call) has been
> reintroduced.

We didn't promise to remove png_flush() at that point; we promised to
remove the " #if ...  defined(PNG_WRITE_FLUSH_AFTER_IEND_SUPPORTED)"
kludge, and we did that.

> Png_flush and all the flush function stuff needs to be removed from 1.4.0.

We would be going around in circles then, because there was another
circumstance that needs it (I don't remember the details right now but
it should be in the list archives).

The problem seems to be that you have PNG_STDIO_SUPPORTED
defined but png_file_p is not properly defined, which means you
don't really have stdio support.

... I checked the list archives and bug tracker and cannot find the
discussion.  I'll go ahead and remove the png_flush() call and
hopefully the person who needs it will remind us.  As I recall,
that person did not look at beta and rc distributions, though, so
we should not expect to hear from him until 1.4.0 is out.

Glenn

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[Glenn Randers-Pehrson]

On Sat, Nov 21, 2009 at 1:50 AM, Glenn Randers-Pehrson
<glennrp@...> wrote:
> I'll go ahead and remove the png_flush() call and
> hopefully the person who needs it will remind us.  As I recall,
> that person did not look at beta and rc distributions, though, so
> we should not expect to hear from him until 1.4.0 is out.

On second thought, since we've already been around this circle two
or three times, I'll simply restore this to the 1.2.41 condition.  That is,
png_flush() will normally not be present, but it can be restored by
defining PNG_WRITE_FLUSH_AFTER_IEND_SUPPORTED.

It's in the GIT devel branch now.

Glenn

------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement

[John Bowler]

From: Glenn Randers-Pehrson [mailto:glennrp@...]
>We didn't promise to remove png_flush() at that point; we promised to
>remove the " #if ...  defined(PNG_WRITE_FLUSH_AFTER_IEND_SUPPORTED)"
>kludge, and we did that.

OK, but the kludge allowed the google engineers to add calls to libpng where
io_ptr was changed from the default but the flush function was not
re-implemented.

They'll probably fix it - I've filed a bug against chromium - but it seems
very wrong to permanently kludge 1.2 and unkludge in 1.4, particularly as
the comments in the 1.2 png.h don't make the requirements clear.

Your later suggestion to preserve the 1.2 behavior (complete with double
hush top secret define) seems like the best solution.

>The problem seems to be that you have PNG_STDIO_SUPPORTED
>defined but png_file_p is not properly defined, which means you
>don't really have stdio support.

The build of libpng is provided by the system builder (effectively me in
this case) and I have to have PNG_STDIO_SUPPORTED because so many apps
depend on it.  The choice to use a different io_ptr (it is defined as a
(void*) after all) is made in the individual applications and is therefore
outside my control.

In any case calling png_flush at the end of a write is, plain and simply,
*wrong*.  A call to fflush has major performance implications.  It's not
just that it flushes out the application write buffer, the problem is that
in doing so it can cause a disc write of a partial block and that can cause
the file system to thrash around needlessly.

This isn't the application's responsibility.  An application shouldn't have
to re-write the flush function to disable it.  There is, so far as I am
aware, only one reason for calling fflush - because your are about to call
fseek!  The application has no right to assume that using libpng to write
into a (FILE*) guarantees the write is terminated by fflush and, anyway, it
is really easy for the app to call fflush itself.

That said, maybe png_write_end should call png_flush if *and only if*
png_set_flush with nrows > 0 has been called.

John Bowler <jbowler@...>



------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now.  http://p.sf.net/sfu/bobj-july
_______________________________________________
png-mng-implement mailing list
png-mng-implement@...
https://lists.sourceforge.net/lists/listinfo/png-mng-implement