Memory leak in sixel_encoder_encode_bytes

[muetzenmann]

By using valgrind, I found a memory leak in sixel_encoder_encode_bytes.

frame is allocated via sixel_frame_new, but never free'd.

To fix it, you can change line 1798 to

sixel_frame_t *frame = NULL;

and add

sixel_frame_unref(frame);

before the return statement.

[j4james]

Unfortunately you can't use sixel_frame_unref to fix this, because the frame takes ownership of the pixels and palette that are passed to it, and it'll try and free them when the frame is destroyed. But in the sixel_encoder_encode_bytes function, the pixels and palette are coming from an external source, so the frame has no right to free them.

I think the simplest way to fix this would be to free the frame manually, and unref the allocator associated with it. It's a bit messy, but I can't see an easier way to do this without adding more functions to the API.

So this would be my recommended fix:

diff --git a/src/encoder.c b/src/encoder.c
index 0facfb7..5cfb918 100644
--- a/src/encoder.c
+++ b/src/encoder.c
@@ -1748,7 +1748,7 @@ sixel_encoder_encode_bytes(
     int                 /* in */    ncolors)
 {
     SIXELSTATUS status = SIXEL_FALSE;
-    sixel_frame_t *frame;
+    sixel_frame_t *frame = NULL;
 
     if (encoder == NULL || bytes == NULL) {
         status = SIXEL_BAD_ARGUMENT;
@@ -1774,6 +1774,13 @@ sixel_encoder_encode_bytes(
     status = SIXEL_OK;
 
 end:
+    /* we need to free the frame before exiting, but we can't use the
+       sixel_frame_destroy function, because that will also attempt to
+       free the pixels and palette, which we don't own */
+    if (frame != NULL && encoder->allocator != NULL) {
+        sixel_allocator_free(encoder->allocator, frame);
+        sixel_allocator_unref(encoder->allocator);
+    }
     return status;
 }
 

[j4james]

Can't comment in the libsixel/libsixel repo anymore, but note that https://github.com/libsixel/libsixel/issues/61 is a duplicate of this.

[saitoha]

Thanks for the report and for considering a patch. This is a bug in API usage, and I believe we need an API to return ownership of the pixel buffer to user code (e.g., sixel_frame_release_pixels). 269fcfd

[saitoha]

Since 269fcfd is a relatively large change, for the v1.8.7 release I’ve landed @j4james’s fix 92bb9b3 on master. Thank you.