GNOME Bugzilla – Bug 788759
qtdemux: fix 'stsd' table leak and nested caps leak
Last modified: 2017-12-04 09:27:55 UTC
typically in DASH/HLS adaptive streaming cases, 'moov' box will be handled in qtdemux multiple times. An existing QtDemuxStream's 'stsd' will be overwritten without freed. if an existing QtDemuxStream is found, its 'stsd' tables and related caps need to be freed.
Created attachment 361221 [details] [review] fix leak patch added. please review
please kindly review the patch.
Comment on attachment 361221 [details] [review] fix leak gst_qtdemux_stream_clear() also unrefs the rgb8_palette field but does not free anything else, and it also clears various other fields (are any of the others ones needed here too? The protection ones maybe?). Maybe refactor the above into a separate function that is also called from stream_clear()?
(In reply to Sebastian Dröge (slomo) from comment #3) > Comment on attachment 361221 [details] [review] [review] > fix leak > > gst_qtdemux_stream_clear() also unrefs the rgb8_palette field but does not > free anything else, and it also clears various other fields (are any of the > others ones needed here too? The protection ones maybe?). Maybe refactor the > above into a separate function that is also called from stream_clear()? yes, great suggestion. I will dig more, and sort out what's more need to be freed. a separate function will be created.
Created attachment 361713 [details] [review] reset reused QtDemuxStream
after digging more, IMHO, QtDemuxStream needs to be reset if reused. 'gst_qtdemux_stream_reset' is newly added, it does: 1. clear stream 2. free stsd entries, and its nested caps please review.
Review of attachment 361713 [details] [review]: Don't change the permissions of files. ::: gst/isomp4/qtdemux.c @@ +2543,3 @@ } } + g_free (stream->stsd_entries); This will crash when called another time, might want to set it to 0 and the number of entries too.
(In reply to Sebastian Dröge (slomo) from comment #7) > Review of attachment 361713 [details] [review] [review]: > > Don't change the permissions of files. > > ::: gst/isomp4/qtdemux.c > @@ +2543,3 @@ > } > } > + g_free (stream->stsd_entries); > > This will crash when called another time, might want to set it to 0 and the > number of entries too. thanks for reviewing, please review the new patch again.
Created attachment 362044 [details] [review] reset reused QtDemuxStream_v2
hi, slomo, please review the newly added patch. thanks.
*** Bug 790110 has been marked as a duplicate of this bug. ***
commit 7c8aeff262a58d62ce3ad44ef7a5aec4ae8bacd7 (HEAD -> master) Author: Jun Xie <jun.xie@samsung.com> Date: Sun Oct 22 18:26:12 2017 +0800 qtdemux: reset reused QtDemuxStream while parsing a new 'trak' if QtDemuxStream is reused, then we need to reset it. https://bugzilla.gnome.org/show_bug.cgi?id=788759
*** Bug 790472 has been marked as a duplicate of this bug. ***