[cairo] How to free a buffer when subset PS Type1 generation is failed?

[mpsuzuki at hiroshima-u.ac.jp]

Hi,

Now I'm playing with pdftoppm (poppler utils) to attach
a shell-like interface, to cropping many fragments from
a signe huge PDF. During the development, I found a
problem that cairo_type1_font_subset_generate() does not
free a buffer allocated by _cairo_output_stream_create()
in some case, and it behaves like a memory leak during
PDF generation. I want to ask the appropriate method to
free it.

In following, I explain my understanding of current implementation.

cairo/src/cairo-type1-subset.c

   1214 static cairo_status_t
   1215 cairo_type1_font_subset_generate (void       *abstract_font,
   1216                                   const char *name)
   1217 
   1218 {
   1219     cairo_type1_font_subset_t *font = abstract_font;
   1220     cairo_ft_unscaled_font_t *ft_unscaled_font;
   1221     unsigned long ret;
   1222     cairo_status_t status;

   [snip]

   1258     font->output = _cairo_output_stream_create (type1_font_write, NULL, font);

   Here, _cairo_output_stream_create() is called, and a buffer
   is allocated by malloc() to hold the function pointers,
   type1_font_write, NULL (=type1_font_close), and font.
   
   1259     if (_cairo_output_stream_get_status (font->output)) {
   1260         status = _cairo_output_stream_destroy (font->output);
   1261         goto fail;
   1262     }

   Here, the status of the stream is found to be bad, so
   it's destroyed. _cairo_output_stream_destroy() invokes
   free() and the allocated buffer is freed.

   1263 
   1264     status = cairo_type1_font_subset_write (font, name);
   1265     if (unlikely (status))
   1266         goto fail;

   Here, if we got any error in the writing subsetted PS
   Type1 data to the stream, we jump to fail without
   freeing the allocated buffer.

   1267 
   1268     font->base.data = _cairo_array_index (&font->contents, 0);

   Here, if we successfully finished to write subsetted
   PS Type1 data to the stream, the base font information
   is updated to refer the generated font in the stream.
   The stream is referred, so it should not be freed.

   1269 
   1270  fail:
   1271     _cairo_ft_unscaled_font_unlock_face (ft_unscaled_font);
   1272 
   1273     return status;
   1274 }
   1275 

   Unlock the face and return the caller.

The problem I found is that if we got any error in the
writing of subssetted PS Type1 font to the stream
(in my case, the passed font is recognized as unsupported
format), the stream would not be referred anymore, but
the buffer holding the stream is not freed.

If I insert _cairo_output_stream_destroy() for failure
case, aslike

   1264     status = cairo_type1_font_subset_write (font, name);
   1265     if (unlikely (status))
   1266         _cairo_output_stream_destroy (font->output);
   1267     else
   1268         font->base.data = _cairo_array_index (&font->contents, 0);

the buffer is freed. Is this appropriate fix?

--

Also I have to note the reason why I don't check the
return value of _cairo_output_stream_destroy().

The return value of _cairo_output_stream_destroy() notices
the success/failure of the process to destroy the stream.
The most important informations to be returned to the caller
of cairo_type1_font_subset_generate() is the status of the
subsetted PS Type1 font generation. When the font generation
itself is failed, this failure information is more important
than the status of stream destroy process. So I didn't
overwrite the `status' by _cairo_output_stream_destroy().

The overwriting of the status by stream destroyer, aslike

   1264     status = cairo_type1_font_subset_write (font, name);
   1265     if (unlikely (status))
   1266         status = _cairo_output_stream_destroy (font->output);
   1267     else
   1268         font->base.data = _cairo_array_index (&font->contents, 0);
   
is not good idea, I think. Because it behaves aslike the
font generation finishes successfully when the font generation
is failed but the stream is destroyed successfully.

If I have any fallback for the failure of the stream
destroyer (if there's such please let me know), I have to
check the return value.

Because of similar reason, I'm questionable if the stream
destroyer should overwrite the status, like here:

   1259     if (_cairo_output_stream_get_status (font->output)) {
   1260         status = _cairo_output_stream_destroy (font->output);
   1261         goto fail;
   1262     }

--

Following is a patch summarizing my idea in above.
Please give me comment.

Regards,
mpsuzuki

diff --git a/src/cairo-type1-subset.c b/src/cairo-type1-subset.c
index 304353a..b5b3002 100644
--- a/src/cairo-type1-subset.c
+++ b/src/cairo-type1-subset.c
@@ -1256,16 +1256,17 @@ cairo_type1_font_subset_generate (void       *abstract_font,
 	goto fail;
 
     font->output = _cairo_output_stream_create (type1_font_write, NULL, font);
-    if (_cairo_output_stream_get_status (font->output)) {
-	status = _cairo_output_stream_destroy (font->output);
+    if (unlikely (status)) {
+	_cairo_output_stream_destroy (font->output);
+	status = CAIRO_STATUS_NO_MEMORY;
 	goto fail;
     }
 
     status = cairo_type1_font_subset_write (font, name);
     if (unlikely (status))
-	goto fail;
-
-    font->base.data = _cairo_array_index (&font->contents, 0);
+	_cairo_output_stream_destroy (font->output);
+    else
+	font->base.data = _cairo_array_index (&font->contents, 0);
 
  fail:
     _cairo_ft_unscaled_font_unlock_face (ft_unscaled_font);