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.

5 files changed, 20 insertions, 17 deletions

diff --git a/include/mupdf/fitz/image.h b/include/mupdf/fitz/image.h
index 5d363332..8a8f5007 100644
--- a/include/mupdf/fitz/image.h
+++ b/include/mupdf/fitz/image.h
@@ -230,6 +230,7 @@ fz_image *fz_new_image_from_buffer(fz_context *ctx, fz_buffer *buffer);
 fz_image *fz_new_image_from_file(fz_context *ctx, const char *path);
 
 void fz_drop_image_imp(fz_context *ctx, fz_storable *image);
+void fz_drop_image_base(fz_context *ctx, fz_image *image);
 fz_pixmap *fz_decomp_image_from_stream(fz_context *ctx, fz_stream *stm, fz_compressed_image *image, fz_irect *subarea, int indexed, int l2factor);
 fz_pixmap *fz_expand_indexed_pixmap(fz_context *ctx, const fz_pixmap *src, int alpha);
 size_t fz_image_size(fz_context *ctx, fz_image *im);
diff --git a/include/mupdf/fitz/store.h b/include/mupdf/fitz/store.h
index 81c7c1e2..6eab5064 100644
--- a/include/mupdf/fitz/store.h
+++ b/include/mupdf/fitz/store.h
@@ -55,10 +55,10 @@ void *fz_keep_storable(fz_context *, const fz_storable *);
 void fz_drop_storable(fz_context *, const fz_storable *);
 
 void *fz_keep_key_storable(fz_context *, const fz_key_storable *);
-int fz_drop_key_storable(fz_context *, const fz_key_storable *);
+void fz_drop_key_storable(fz_context *, const fz_key_storable *);
 
 void *fz_keep_key_storable_key(fz_context *, const fz_key_storable *);
-int fz_drop_key_storable_key(fz_context *, const fz_key_storable *);
+void fz_drop_key_storable_key(fz_context *, const fz_key_storable *);
 
 static inline int fz_key_storable_needs_reaping(fz_context *ctx, const fz_key_storable *ks)
 {
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 */
diff --git a/source/gprf/gprf-doc.c b/source/gprf/gprf-doc.c
index 53816a15..23731867 100644
--- a/source/gprf/gprf-doc.c
+++ b/source/gprf/gprf-doc.c
@@ -175,6 +175,7 @@ fz_drop_image_gprf_imp(fz_context *ctx, fz_storable *image_)
 
 	fz_drop_gprf_file(ctx, image->file);
 	fz_drop_separations(ctx, image->separations);
+	fz_drop_image_base(ctx, image->super);
 }
 
 static inline unsigned char *cmyk_to_rgba(unsigned char *out, uint32_t c, uint32_t m, uint32_t y, uint32_t k)