Fix: free the PNG row pointers when libpng reports an error - #874
Open
DTW-Thalion wants to merge 1 commit into
Open
Fix: free the PNG row pointers when libpng reports an error#874DTW-Thalion wants to merge 1 commit into
DTW-Thalion wants to merge 1 commit into
Conversation
-_initBitmapFromPNG: keeps the decoded buffer and the row pointer array in plain locals. Both are set after setjmp() and read again in the longjmp() handler, where their values are indeterminate: a local held in a register is restored to what it was at the setjmp call, so the handler can see NULL and skip the free. Decoding a PNG that libpng rejects part way through leaked the row pointer array, one pointer per row of the declared image height. Declare the two pointers volatile so they survive the jump. The bitmap data planes argument takes a plain copy, since it wants an unqualified pointer.
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.
-_initBitmapFromPNG: holds the decoded buffer and the row pointer array in plain locals. Both are assigned after setjmp() and read again in the longjmp() handler, where the value of a local that has been written since the setjmp call is indeterminate. In practice one of them is held in a register that longjmp restores to its earlier value, so the handler sees NULL and skips the free. A PNG that libpng rejects part way through decoding therefore leaks the row pointer array, which holds one pointer per row of the height declared in the header.
Declaring the two pointers volatile makes them survive the jump. The bitmap data planes argument now takes a plain copy of the buffer pointer, since it wants an unqualified one.
Tests/gui/NSBitmapImageRep/pngAllocationOverflow.m already covers this: it feeds a 2148 by 1000000 image that libpng rejects with "Not enough image data".
Run gnustep-tests from Tests/gui. 4353 assertions pass and one is a dashed hope, before and after.
Built with base and gui GNUSTEP_WITH_ASAN=1, that test goes from leaking 8000000 bytes in one allocation to leaking nothing, and no other test program's leak total changes.