pdfwrite - review use of sclose().

The stream interface essentially leaves the buffer management in the
hands of the creator; sclose() does not free the stream buffer, but it
does set the pointer in the stream state to NULL.

This can be problematic; if the only reference we have to the original
buffer is the pointer in the stream state, then we must copy the pointer
before calling sclose() and then free the buffer afterwards.

s_close_filters() does this BUT it can't know whether a given buffer
was allocated in memory, from the C heap or some other fixed allocation.
It simply frees all the buffers. Obviously this can cause problems if
we use it indiscriminately.

I've reviewed all the places pdfwrite uses sclose() and where we can
use s_close_filters() I've modified the code to do so (to avoid memory
leaks in non-GC memory allocators). Where we must not attempt to free
the buffer I've left the sclose() but commented on the reason.

4 files changed, 24 insertions, 1 deletions

diff --git a/devices/vector/gdevpdf.c b/devices/vector/gdevpdf.c
index 1d2010dd6..6752fabba 100644
--- a/devices/vector/gdevpdf.c
+++ b/devices/vector/gdevpdf.c
@@ -467,6 +467,12 @@ pdf_compute_fileID(gx_device_pdf * pdev)
     pdev->KeyLength = KeyLength;
     if (code < 0)
         return code;
+    /* Generally we would call s_close_filters() here in order to free the data buffer
+     * associated with the MD5 filter, but the data buffer we passed in to s_MD5E_make_stream()
+     * is part of the device structure, so we must *NOT* free that buffer. Therefore we must
+     * instead call sclose(). This confusion over ownership of the stream buffers causes
+     * a lot of problems......
+     */
     sclose(s);
     gs_free_object(mem, s, "pdf_compute_fileID");
     return 0;
diff --git a/devices/vector/gdevpdfc.c b/devices/vector/gdevpdfc.c
index a48701d6e..3a3096c41 100644
--- a/devices/vector/gdevpdfc.c
+++ b/devices/vector/gdevpdfc.c
@@ -643,6 +643,10 @@ pdf_indexed_color_space(gx_device_pdf *pdev, const gs_gstate * pgs, cos_value_t
     }
     stream_write(&es, palette, table_size);
     gs_free_string(mem, palette, table_size, "pdf_color_space(palette)");
+    /* Another case where we use sclose() and not sclose_filters(), because the
+     * buffer we supplied to s_init_filter is a heap based C object, so we
+     * must not free it.
+     */
     sclose(&es);
     sflush(&s);
     string_used = (uint)stell(&s);
@@ -874,6 +878,11 @@ pdf_color_space_named(gx_device_pdf *pdev, const gs_gstate * pgs,
         if (code < 0)
             return_error(gs_error_unregistered); /* Must not happen. */
         serialized_size = stell(&s);
+        /* I think this is another case where we use sclose() and not sclose_filters().
+         * It seems like we don't actually write anything, but it allows us to find the
+         * length of the serialised data. No buffer hre, so we must no call
+         * s_close_filters() as that will try to free it.
+         */
         sclose(&s);
         if (serialized_size <= sizeof(serialized0))
             serialized = serialized0;
diff --git a/devices/vector/gdevpdfo.c b/devices/vector/gdevpdfo.c
index 77ef7f044..70b66c6b9 100644
--- a/devices/vector/gdevpdfo.c
+++ b/devices/vector/gdevpdfo.c
@@ -1132,6 +1132,10 @@ static int write_key_as_string_encrypted(const gx_device_pdf *pdev, const byte *
     memcpy(buffer, str, size);
     s_arcfour_process_buffer(&sarc4, buffer, size);
     stream_write(&sout, buffer, size);
+    /* Another case where we use sclose() and not s_close_filters(), because the
+     * buffer we supplied to s_init_filter is a heap based C object, so we
+     * must not free it.
+     */
     sclose(&sout); /* Writes ')'. */
     gs_free_object(pdev->pdf_memory, buffer, "Free encryption buffer");
     return 0;
diff --git a/devices/vector/gdevpdfu.c b/devices/vector/gdevpdfu.c
index 4dfa8712d..710eab108 100644
--- a/devices/vector/gdevpdfu.c
+++ b/devices/vector/gdevpdfu.c
@@ -2117,6 +2117,10 @@ pdf_encrypt_encoded_string(const gx_device_pdf *pdev, const byte *str, uint size
             break;
         }
     }
+    /* Another case where we use sclose() and not sclose_filters(), because the
+     * buffer we supplied to s_init_filter is a heap based C object, so we
+     * must not free it.
+     */
     sclose(&sout); /* Writes ')'. */
     return (int)stell(&sinp) + 1;
 }
@@ -2681,7 +2685,7 @@ pdf_function_aux(gx_device_pdf *pdev, const gs_function_t *pfn,
                 stream_write(writer.strm, ptr, count);
             }
             code = psdf_end_binary(&writer);
-            sclose(s);
+            s_close_filters(&s, s->strm);
         }
         pdev->strm = save;
         if (code < 0)