After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 753087 - jpegdec: fix output state memory leak
jpegdec: fix output state memory leak
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-07-31 04:02 UTC by Vineeth
Modified: 2015-11-10 11:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix output state memory leak (4.61 KB, patch)
2015-07-31 04:04 UTC, Vineeth
none Details | Review
fix output state memory leak (1.10 KB, patch)
2015-08-06 03:45 UTC, Vineeth
none Details | Review
fix output state memory leak (1.61 KB, patch)
2015-08-06 05:11 UTC, Vineeth
committed Details | Review

Description Vineeth 2015-07-31 04:02:59 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
Comment 1 Vineeth 2015-07-31 04:04:25 UTC
Created attachment 308507 [details] [review]
fix output state memory leak
Comment 2 Thiago Sousa Santos 2015-07-31 05:53:22 UTC
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
Comment 3 Vineeth 2015-07-31 06:29:21 UTC
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..
Comment 4 Vineeth 2015-07-31 08:22:14 UTC
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
Comment 5 Thiago Sousa Santos 2015-08-05 18:18:32 UTC
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.
Comment 6 Vineeth 2015-08-06 03:45:59 UTC
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.
Comment 7 Thiago Sousa Santos 2015-08-06 04:56:31 UTC
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.
Comment 8 Vineeth 2015-08-06 05:01:06 UTC
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()
Comment 9 Thiago Sousa Santos 2015-08-06 05:04:33 UTC
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.
Comment 10 Vineeth 2015-08-06 05:11:05 UTC
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.
Comment 11 Thiago Sousa Santos 2015-08-06 13:02:05 UTC
<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?
Comment 12 Vineeth 2015-08-07 00:23:18 UTC
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.
Comment 13 Thiago Sousa Santos 2015-11-10 11:48:43 UTC
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