GNOME Bugzilla – Bug 753087
jpegdec: fix output state memory leak
Last modified: 2015-11-10 11:48:57 UTC
output state was being created locally and the references were not getting cleared during decoding error cases, leading to leak. Hence creating a state variable in dec structure and freeing it on finalize
Created attachment 308507 [details] [review] fix output state memory leak
Review of attachment 308507 [details] [review]: Wouldn't it be easier just to unref the object in the error cases instead of keeping a ref? Seems to add complexity without any benefit
the reason it was not working like that was in gst_jpeg_dec_handle_frame we are doing state = gst_video_decoder_get_output_state (bdec); which increases the refcount... and as soon as it reaches jpeg_finish_decompress (&dec->cinfo); the library is throwing error which makes it jump to if (setjmp (dec->jerr.setjmp_buffer)) { code = dec->jerr.pub.msg_code; .... .... } at this point of time, the local variable state is NULL, but the reference is still there..
probably this is because it is happening longjmp() restores the environment saved by the last call of setjmp(3) with the corresponding env argument. before setjmp is called, state is still NULL, so once the error happens, it comes back to setjmp and at this time it is restored to state.. Have to read a bit more about longjmp, setjmp
How about using another call to setjmp after having the output_state reference? In that case I assume the state will restored with the variable value and we can also do proper frame unmapping cleanup.
Created attachment 308828 [details] [review] fix output state memory leak As per review, instead of changing the whole logic to make state as a variable in structure, adding another setjmp after state gets allocated to make sure memory is not leaked.
What if one of the gst_jpeg_dec_decode_* has the same error handling? I think it might also call the error routines. So yet another setjmp is needed there. This one is correct because it will need to unref the output_state but one more is needed to when it needs to unref the output state but also unmap the video frame.
instead of adding one more setjmp, how does this sound if (setjmp (dec->jerr.setjmp_buffer)) { code = dec->jerr.pub.msg_code; gst_video_frame_unmap (&vframe); goto decode_error; } just after gst_video_frame_map()
If the error happens in jpeg_finish_decompress then the frame has already been unmapped and shouldn't be unmapped again. The error handling is different for those 2 parts.
Created attachment 308834 [details] [review] fix output state memory leak My bad! overlooked that. adding one more setjmp to make sure frame is unmapped if error happens before finish_decompress.
<ystreet00> thiagoss: http://patrakov.blogspot.com/2009/07/dangers-of-setjmplongjmp.html ? <ystreet00> ie, the making the variable volatile This was just raised on IRC as a more readable alternative. Would you mind trying that and check if it helps so we don't have to set that many setjmp()s?
hmm i just tried that... It doesnt seem to work. I declared state as a volatile variable and let it error out. And in setjmp function, state is still NULL.
commit 7ef9dd37610d1b8e94c7144e546be28674fce148 Author: Vineeth TM <vineeth.tm@samsung.com> Date: Thu Aug 6 12:44:20 2015 +0900 jpegdec: fix output state memory leak When jpeg_finish_decompress is called, output state reference is being created. But if there is any failures in finishing decompress, it jumps to setjmp, and at that point state was not referenced. Resulting in leak of output state. Hence adding another setjmp after output state is referenced. Similarly adding another setjmp to unmap the frame in case error happens before finish_decompress https://bugzilla.gnome.org/show_bug.cgi?id=753087