Fix key_storable operations leading to leaks of fz_image.

key_storable objects can either be freed 'directly' by an API call
(such as fz_drop_image), or 'internally' by the store realising that
the sole thing holding onto a key_storable is a key.

The current code frees more structure in the direct call than it
does in the internal call. Clearly this is wrong and leads to
leaks.

The fix is to do ALL the freeing in the internal 'drop' function
within the key_storable.

This means we don't need (or want) either fz_drop_key_storable_key or
fz_drop_key_storable to return an int to tell us whether they were
actually dropped, so we make that change to simplify the code.

This shifts the responsibility for freeing the extra internal
structure into the innermost drop functions - fz_drop_image_imp and
fz_drop_image_gprf_imp. To avoid duplicating code, we put the
extra freeing logic into a fz_drop_image_base function.

2 files changed, 16 insertions, 15 deletions

diff --git a/source/fitz/image.c b/source/fitz/image.c
index 8495098e..5c4147f4 100644
--- a/source/fitz/image.c
+++ b/source/fitz/image.c
@@ -46,8 +46,7 @@ fz_keep_image_store_key(fz_context *ctx, fz_image *image)
 void
 fz_drop_image_store_key(fz_context *ctx, fz_image *image)
 {
-	if (fz_drop_key_storable_key(ctx, &image->key_storable))
-		fz_free(ctx, image);
+	fz_drop_key_storable_key(ctx, &image->key_storable);
 }
 
 static int
@@ -114,12 +113,7 @@ static const fz_store_type fz_image_store_type =
 void
 fz_drop_image(fz_context *ctx, fz_image *image)
 {
-	if (fz_drop_key_storable(ctx, &image->key_storable))
-	{
-		fz_drop_colorspace(ctx, image->colorspace);
-		fz_drop_image(ctx, image->mask);
-		fz_free(ctx, image);
-	}
+	fz_drop_key_storable(ctx, &image->key_storable);
 }
 
 static void
@@ -376,11 +370,20 @@ fz_decomp_image_from_stream(fz_context *ctx, fz_stream *stm, fz_compressed_image
 }
 
 void
+fz_drop_image_base(fz_context *ctx, fz_image *image)
+{
+	fz_drop_colorspace(ctx, image->colorspace);
+	fz_drop_image(ctx, image->mask);
+	fz_free(ctx, image);
+}
+
+void
 fz_drop_image_imp(fz_context *ctx, fz_storable *image_)
 {
 	fz_image *image = (fz_image *)image_;
 
 	image->drop_image(ctx, image);
+	fz_drop_image_base(ctx, image);
 }
 
 static void
diff --git a/source/fitz/store.c b/source/fitz/store.c
index e2ce8d01..dfde067f 100644
--- a/source/fitz/store.c
+++ b/source/fitz/store.c
@@ -170,7 +170,7 @@ do_reap(fz_context *ctx)
 	}
 }
 
-int fz_drop_key_storable(fz_context *ctx, const fz_key_storable *sc)
+void fz_drop_key_storable(fz_context *ctx, const fz_key_storable *sc)
 {
 	/* Explicitly drop const to allow us to use const
 	 * sanely throughout the code. */
@@ -179,7 +179,7 @@ int fz_drop_key_storable(fz_context *ctx, const fz_key_storable *sc)
 	int unlock = 1;
 
 	if (s == NULL)
-		return 0;
+		return;
 
 	if (s->storable.refs > 0)
 		(void)Memento_dropRef(s);
@@ -213,7 +213,6 @@ int fz_drop_key_storable(fz_context *ctx, const fz_key_storable *sc)
 	 */
 	if (drop)
 		s->storable.drop(ctx, &s->storable);
-	return drop;
 }
 
 void *fz_keep_key_storable_key(fz_context *ctx, const fz_key_storable *sc)
@@ -237,7 +236,7 @@ void *fz_keep_key_storable_key(fz_context *ctx, const fz_key_storable *sc)
 	return s;
 }
 
-int fz_drop_key_storable_key(fz_context *ctx, const fz_key_storable *sc)
+void fz_drop_key_storable_key(fz_context *ctx, const fz_key_storable *sc)
 {
 	/* Explicitly drop const to allow us to use const
 	 * sanely throughout the code. */
@@ -245,7 +244,7 @@ int fz_drop_key_storable_key(fz_context *ctx, const fz_key_storable *sc)
 	int drop;
 
 	if (s == NULL)
-		return 0;
+		return;
 
 	if (s->storable.refs > 0)
 		(void)Memento_dropRef(s);
@@ -263,7 +262,6 @@ int fz_drop_key_storable_key(fz_context *ctx, const fz_key_storable *sc)
 	 */
 	if (drop)
 		s->storable.drop(ctx, &s->storable);
-	return drop;
 }
 
 static void
@@ -900,7 +898,7 @@ void fz_filter_store(fz_context *ctx, fz_store_filter_fn *fn, void *arg, const f
 		remove = item->next;
 
 		/* Drop a reference to the value (freeing if required) */
-		if (item->prev)
+		if (item->prev) /* See above for our abuse of prev here */
 			item->val->drop(ctx, item->val);
 
 		/* Always drops the key and drop the item */