fix: uncaught allocation exceptions in decoders, NULL deref in GIF decoder, unchecked malloc in PNG encoder - #7
Open
MinhyukHong wants to merge 4 commits into
Conversation
decode_jpeg_buffer() only handled libjpeg's own C-style errors via setjmp/longjmp, but CImg's allocation calls (buffimg construction and cimg->assign()) can throw a C++ CImgException when the image dimensions in the header are large enough to trigger a huge allocation attempt. This exception was never caught, causing std::terminate() and a hard crash of the whole process. Now it's caught and reported as a decoding error, consistent with the other error paths in this function.
decode_png_buffer() only handled libpng's own errors via setjmp/longjmp, but the raw `new[]` allocations for imgData (sized directly from the PNG header's width/height, up to libpng's compiled-in PNG_USER_WIDTH_MAX/HEIGHT_MAX of 1,000,000 each, since png_set_user_limits() was never called to lower this) and CImg::assign() can both throw uncaught C++ exceptions (std::bad_alloc / CImgException) when the declared dimensions are large enough. This crashed the whole process via std::terminate(). Now both are caught and reported as a decoding error.
decode_gif_buffer() dereferenced cmap->Colors[ci] with no check that cmap itself was non-NULL, or that ci (a pixel's color table index) was within cmap->ColorCount. A GIF with neither a global nor a local color table crashes on the NULL dereference; a malformed pixel index beyond the color table's actual size caused an out-of-bounds read. Both cases are now treated as fully transparent, consistent with how this function already handles the transparent-color case.
encodeToPngBuffer() allocated a buffer for the metadata string but never checked whether malloc() succeeded before calling WriteUtf8() on it. If the allocation failed (e.g. under memory pressure, or with a metadata string large enough to make the size computation overflow), this wrote into a NULL pointer and crashed the process. Now a failed allocation is reported as a normal JS exception instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: weixin/node-lwip appears to share its codebase with EyalAr/lwip (the decoder and encoder files affected by this PR are byte-for-byte identical between the two repos). I've submitted the same fix to EyalAr/lwip as well.