msf_gif icon indicating copy to clipboard operation
msf_gif copied to clipboard

Assumption of 64k stack breaks various devices

Open randomouscrap98 opened this issue 1 year ago • 2 comments

I know it's a bit silly, but many devices have a stack size of 32k. As such, this library crashes with some pretty interesting results, and it was difficult to track down (the device in question was a 3DS, and I didn't have the luxury of messages telling me the stack died).

I know it's a bit of an ask, but would it be at all possible to have a compiler flag for "small stacks" which mallocs these 64k arrays?

randomouscrap98 avatar Sep 15 '24 08:09 randomouscrap98

Thanks for letting me know. I didn't expect someone would use this library on the 3DS, that's very cool! In the interest of keeping things simple, I'll probably just make it always allocate, rather than adding a flag, since there shouldn't be any major detriment to doing so. I'll try and put that together with some other small improvements for a v2.3 of the library when I have time this week.

notnullnotvoid avatar Sep 16 '24 20:09 notnullnotvoid

Oh awesome! There's no rush, I already modified the library locally to use malloc and it works great; thank you for making such a cool library!

randomouscrap98 avatar Sep 16 '24 21:09 randomouscrap98

WebAssembly is another place where 64K on the stack might sometimes be too much as Emscripten default stack size is 64K. For example, TIC-80 is using your library for screenshots and video recording. This is the way I patched it to have it work with Firefox. Thank you for your great library!

diff --git a/msf_gif.h b/msf_gif.h
index 9374c8b..6062ba7 100644
--- a/msf_gif.h
+++ b/msf_gif.h
@@ -409,7 +409,7 @@ static MsfGifBuffer * msf_compress_frame(void * allocContext, int width, int hei
     //allocate tlb
     int totalBits = frame.rbits + frame.gbits + frame.bbits;
     int tlbSize = (1 << totalBits) + 1;
-    uint8_t tlb[(1 << 16) + 1]; //only 64k, so stack allocating is fine
+    uint8_t * tlb = (uint8_t *) MSF_GIF_MALLOC(allocContext, (1 << 16) + 1);
 
     //generate palette
     typedef struct { uint8_t r, g, b; } Color3;
@@ -505,6 +505,8 @@ static MsfGifBuffer * msf_compress_frame(void * allocContext, int width, int hei
         }
     }
 
+    MSF_GIF_FREE(allocContext, tlb, (1 << 16) + 1);
+
     //write code for leftover index buffer contents, then the end code
     msf_put_code(&writeHead, &blockBits, msf_imin(12, msf_bit_log(lzw.len - 1)), lastCode);
     msf_put_code(&writeHead, &blockBits, msf_imin(12, msf_bit_log(lzw.len)), tableSize + 1);
@@ -594,12 +596,13 @@ int msf_gif_frame(MsfGifState * handle, uint8_t * pixelData, int centiSecondsPer
     if (pitchInBytes == 0) pitchInBytes = handle->width * 4;
     if (pitchInBytes < 0) pixelData -= pitchInBytes * (handle->height - 1);
 
-    uint8_t used[(1 << 16) + 1]; //only 64k, so stack allocating is fine
+    uint8_t *used = (uint8_t *) MSF_GIF_MALLOC(handle->customAllocatorContext, (1 << 16) + 1);
     msf_cook_frame(&handle->currentFrame, pixelData, used, handle->width, handle->height, pitchInBytes,
         msf_imin(maxBitDepth, handle->previousFrame.depth + 160 / msf_imax(1, handle->previousFrame.count)));
 
     MsfGifBuffer * buffer = msf_compress_frame(handle->customAllocatorContext, handle->width, handle->height,
         centiSecondsPerFame, handle->currentFrame, handle, used, handle->lzwMem);
+    MSF_GIF_FREE(handle->customAllocatorContext, used, (1 << 16) + 1);
     if (!buffer) { msf_free_gif_state(handle); return 0; }
     handle->listTail->next = buffer;
     handle->listTail = buffer;

nopid avatar Jan 21 '25 08:01 nopid

I didn't know TIC-80 used msf_gif, that's pretty cool to learn.

notnullnotvoid avatar Feb 17 '25 04:02 notnullnotvoid

This should now be fixed in the latest release.

notnullnotvoid avatar Feb 17 '25 07:02 notnullnotvoid