GNOME Bugzilla – Bug 755161
h264parse, h265parse: initialize picture parameter set structure
Last modified: 2015-10-29 09:28:55 UTC
When calling gst_h264_parse_pps() the function may fail when it calls gst_h264_parser_get_sps(), leaving unitialised the variable slice_group_id. This situation may lead to a segmentation fault when calling gst_h264_pps_clear(). Setting slice_group_id to NULL before calling gst_h264_parser_get_sps() avoids this potential segmentation fault.
Created attachment 311553 [details] [review] h264parse: initialize slice group id first
Is the same change required for h265parse? Do you have a test stream that crashes?
(In reply to Sebastian Dröge (slomo) from comment #2) > Is the same change required for h265parse? NO, hevc doesn't have any dynamic allocation in PPS... Do you have a test stream that > crashes?
(In reply to Sebastian Dröge (slomo) from comment #2) > Do you have a test stream that crashes? I found a case, using gstreamer-vaapi, when testing for bug 754845, with MVCDS-1.264 in gstreamer 1.4
Why exactly is that crashing though, shouldn't the struct be initialized with zeroes or something sensible before passing it there? Is the code that uses this just allocating it on the stack without initialization? I would consider that a bug too
(In reply to Sebastian Dröge (slomo) from comment #5) > Why exactly is that crashing though, shouldn't the struct be initialized > with zeroes or something sensible before passing it there? That's the question, and in bug 754845 we discussed it. But, as the pointer is set to NULL in the function already, it seems to me logical that the pointer should be set to NULL before the function could return, so the a future call to gst_h264_pps_clear() doesn't crash. > Is the code that uses this just allocating it on the stack without > initialization? I would consider that a bug too Yes, and we already integrated the patch for it in gstreamer-vaapi: https://github.com/01org/gstreamer-vaapi/commit/f6ae00a6bb5d55b48fc2074b5abccad0bccee641
(In reply to Víctor Manuel Jáquez Leal from comment #6) > (In reply to Sebastian Dröge (slomo) from comment #5) > > Why exactly is that crashing though, shouldn't the struct be initialized > > with zeroes or something sensible before passing it there? > > That's the question, and in bug 754845 we discussed it. > > But, as the pointer is set to NULL in the function already, it seems to me > logical that the pointer should be set to NULL before the function could > return, so the a future call to gst_h264_pps_clear() doesn't crash. Maybe to prevent such bugs in the future, we should memset() the structs to zero before parsing? For PPS/SPS/etc. Otherwise someone might add another pointer field at some point, and then it crashes in the future again :)
(In reply to Sebastian Dröge (slomo) from comment #7) > (In reply to Víctor Manuel Jáquez Leal from comment #6) > > (In reply to Sebastian Dröge (slomo) from comment #5) > > > Why exactly is that crashing though, shouldn't the struct be initialized > > > with zeroes or something sensible before passing it there? > > > > That's the question, and in bug 754845 we discussed it. > > > > But, as the pointer is set to NULL in the function already, it seems to me > > logical that the pointer should be set to NULL before the function could > > return, so the a future call to gst_h264_pps_clear() doesn't crash. > > Maybe to prevent such bugs in the future, we should memset() the structs to > zero before parsing? For PPS/SPS/etc. yes, do the memset + initialize the non zero default values... > Otherwise someone might add another pointer field at some point, and then it > crashes in the future again :)
Comment on attachment 311553 [details] [review] h264parse: initialize slice group id first Let's go with that then
Created attachment 311577 [details] [review] codecparsers: h26{4,5}: init picture parameter set When calling gst_h26{4,5}_parse_pps() the function may fail when it calls gst_h264_parser_get_sps(), leaving unitialized the PPS structure. This situation may lead to future problems, such as a segmentation fault, in the case of H264, when calling gst_h264_pps_clear(), for example. This patch initializes to zero the PPS structures, either in h264parse and h265parse before filling them.
Review of attachment 311577 [details] [review]: Should do the same for SPS and VPS and possibly other data structures ::: gst-libs/gst/codecparsers/gsth265parser.c @@ -1832,2 @@ pps->uniform_spacing_flag = 1; pps->loop_filter_across_tiles_enabled_flag = 1; And also move these defaults up maybe?
Actually this is not just the case of PPS, we should have the memset() in whole APIs then:parse_sps, parse_slice_hdr etc... :)
Created attachment 311621 [details] [review] codecparser: h264: initialize parsing structures Initialize to 0 these parse structures before filling them: GstH264SEIMessage, GstH264NalUnit, GstH264PPS, GstH264SPS and GstH264SliceHdr. When calling the functions which fill those structures, they may fail, leaving unitialized those structures. This situation may lead to future problems, such as a segmentation fault when freeing, for example. This patch initializes to zero these structures, before filling them.
Created attachment 311622 [details] [review] codecparser: h265: initialize parsing structures Initialize to 0 these parse structures before filling them: GstH265SEIMessage, GstH265NalUnit, GstH265VPS, GstH265PPS, GstH265SPS and GstH265SliceHdr. When calling the functions which fill those structures, they may fail, leaving unitialized those structures. This situation may lead to future problems, such as a segmentation fault when freeing, for example. This patch initializes to zero these structures, before filling them.
Piuf... Done. A lot of assignations were removed. I did some quick playbacks and they ran fine. Sree, could you review that I didn't remove code that I shouldn't??? (In reply to Sebastian Dröge (slomo) from comment #11) > Review of attachment 311577 [details] [review] [review]: > > Should do the same for SPS and VPS and possibly other data structures > > ::: gst-libs/gst/codecparsers/gsth265parser.c > @@ -1832,2 @@ > pps->uniform_spacing_flag = 1; > pps->loop_filter_across_tiles_enabled_flag = 1; > > And also move these defaults up maybe? I'm a bit reluctant to move that code before a possible error return.
Review of attachment 311621 [details] [review]: You have missed this one: gst_h264_parser_identify_nalu_avc() :) otherwise lgtm... Would be good if we can get some testing over a large stream database, or at least the h264 reference streams...
Review of attachment 311622 [details] [review]: lgtm,,
Only thing is that we are adding a slight overhead by doing memset over large structures... Before it was doing memset only for some structures if needed. Even the sub-structures were memset()ing only only when needed (eg: some sub-structures only for B-frames, etc).. Which means, changing more-optimal implementation to sub-optimal, but, I agreed it could be more safer :)
Regarding the test coverage: We will get the full stretched testing over the whole reference streams (both h264 and hevc) once it get integrated to gstreamer-vaapi.... But not soon, I will do this only in the next iteration of codec-parser cherry-pick, after the upcoming bug-fix release of gstreamer-vaapi.
Created attachment 311636 [details] [review] codecparser: h264: initialize parsing structures Initialize to 0 these parse structures before filling them: GstH264SEIMessage, GstH264NalUnit, GstH264PPS, GstH264SPS and GstH264SliceHdr. When calling the functions which fill those structures, they may fail, leaving unitialized those structures. This situation may lead to future problems, such as a segmentation fault when freeing, for example. This patch initializes to zero these structures, before filling them.
If upstream guys are ready to sacrifice with the overhead I mentioned in comment 18, then the patch should be fine.. Each NAL unit in a frame will introduce one memset :)
Yuhu!! \o/
Attachment 311622 [details] pushed as c8b3d84 - codecparser: h265: initialize parsing structures Attachment 311636 [details] pushed as d054a69 - codecparser: h264: initialize parsing structures