From d93f772f1fe89f66eec783e8d36451dc06350710 Mon Sep 17 00:00:00 2001
From: John Cupitt <jcupitt@gmail.com>
Date: Thu, 23 Feb 2012 12:42:21 +0000
Subject: [PATCH] fix deadlock with generate failing

and better error msg from libpng
---
 TODO                         |  4 ----
 libvips/foreign/vipspng.c    | 18 +++++++++++++++---
 libvips/iofuncs/sinkdisc.c   | 16 +++++++++-------
 libvips/iofuncs/sinkmemory.c | 18 ++++++++++--------
 4 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/TODO b/TODO
index 8a7660e0..51cc4874 100644
--- a/TODO
+++ b/TODO
@@ -9,10 +9,6 @@ blocking bugs
 
   similarly for vips_max() of an image which contains NaN, -INFINITY etc.
 
-- clicking on bad pngs in the file browser will lock nip2
-
-  they don't seem to bother "header" though, strange
-
 - new binding is still missing constants 
 
   how do boxed types work? confusing
diff --git a/libvips/foreign/vipspng.c b/libvips/foreign/vipspng.c
index a664a51a..c94b7f8a 100644
--- a/libvips/foreign/vipspng.c
+++ b/libvips/foreign/vipspng.c
@@ -36,6 +36,8 @@
  * 7/2/12
  * 	- mild refactoring
  * 	- add support for sequential reads
+ * 23/2/12
+ * 	- add a longjmp() to our error handler to stop the default one running
  */
 
 /*
@@ -93,13 +95,18 @@
 static void
 user_error_function( png_structp png_ptr, png_const_charp error_msg )
 {
-	vips_error( "png2vips", _( "PNG error: \"%s\"" ), error_msg );
+	vips_error( "png2vips", "%s", error_msg );
+
+	/* This function must not return, or the default error handler will be
+	 * invoked.
+	 */
+	longjmp( png_jmpbuf( png_ptr ), -1 ); 
 }
 
 static void
 user_warning_function( png_structp png_ptr, png_const_charp warning_msg )
 {
-	vips_error( "png2vips", _( "PNG warning: \"%s\"" ), warning_msg );
+	vips_error( "png2vips", "%s", warning_msg );
 }
 
 /* What we track during a PNG read.
@@ -354,8 +361,13 @@ png2vips_generate( VipsRegion *or,
 	g_assert( r->width == or->im->Xsize );
 	g_assert( VIPS_RECT_BOTTOM( r ) <= or->im->Ysize );
 
-	if( setjmp( png_jmpbuf( read->pPng ) ) ) 
+	if( setjmp( png_jmpbuf( read->pPng ) ) ) {
+#ifdef DEBUG
+		printf( "png2vips_generate: failing in setjmp\n" ); 
+#endif /*DEBUG*/
+
 		return( -1 );
+	}
 
 	for( y = 0; y < r->height; y++ ) {
 		png_bytep q = (png_bytep) VIPS_REGION_ADDR( or, 0, r->top + y );
diff --git a/libvips/iofuncs/sinkdisc.c b/libvips/iofuncs/sinkdisc.c
index 5902e059..432a0992 100644
--- a/libvips/iofuncs/sinkdisc.c
+++ b/libvips/iofuncs/sinkdisc.c
@@ -7,6 +7,8 @@
  * 	- better buffer handling for single-line images
  * 17/7/10
  * 	- we could get stuck if allocate failed (thanks Tim)
+ * 23/2/12
+ * 	- we could deadlock if generate failed
  */
 
 /*
@@ -404,21 +406,21 @@ wbuffer_work_fn( VipsThreadState *state, void *a )
 {
 	WriteThreadState *wstate = (WriteThreadState *) state;
 
+	int result;
+
 	VIPS_DEBUG_MSG( "wbuffer_work_fn: %p %d x %d\n", 
 		state, state->pos.left, state->pos.top );
 
-	if( vips_region_prepare_to( state->reg, wstate->buf->region, 
-		&state->pos, state->pos.left, state->pos.top ) ) {
-		VIPS_DEBUG_MSG( "wbuffer_work_fn: %p error!\n", state );
-		return( -1 );
-	}
-	VIPS_DEBUG_MSG( "wbuffer_work_fn: %p done\n", state );
+	result = vips_region_prepare_to( state->reg, wstate->buf->region, 
+		&state->pos, state->pos.left, state->pos.top );
+
+	VIPS_DEBUG_MSG( "wbuffer_work_fn: %p result = %d\n", state, result );
 
 	/* Tell the bg write thread we've left.
 	 */
 	vips_semaphore_upn( &wstate->buf->nwrite, 1 );
 
-	return( 0 );
+	return( result );
 }
 
 static void
diff --git a/libvips/iofuncs/sinkmemory.c b/libvips/iofuncs/sinkmemory.c
index 0565f019..375d5916 100644
--- a/libvips/iofuncs/sinkmemory.c
+++ b/libvips/iofuncs/sinkmemory.c
@@ -6,6 +6,8 @@
  * 
  * 17/2/12
  * 	- from sinkdisc.c
+ * 23/2/12
+ * 	- we could deadlock if generate failed
  */
 
 /*
@@ -250,22 +252,22 @@ sink_memory_area_work_fn( VipsThreadState *state, void *a )
 	SinkMemoryThreadState *wstate = (SinkMemoryThreadState *) state;
 	SinkMemoryArea *area = wstate->area;
 
+	int result;
+
 	VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p %d x %d\n", 
 		state, state->pos.left, state->pos.top );
 
-	if( vips_region_prepare_to( state->reg, memory->region, 
-		&state->pos, state->pos.left, state->pos.top ) ) {
-		VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p error!\n", 
-			state );
-		return( -1 );
-	}
-	VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p done\n", state );
+	result = vips_region_prepare_to( state->reg, memory->region, 
+		&state->pos, state->pos.left, state->pos.top );
+
+	VIPS_DEBUG_MSG( "sink_memory_area_work_fn: %p result = %d\n", 
+		state, result );
 
 	/* Tell the allocator we're done.
 	 */
 	vips_semaphore_upn( &area->nwrite, 1 );
 
-	return( 0 );
+	return( result );
 }
 
 static void