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 755161 - h264parse, h265parse: initialize picture parameter set structure
h264parse, h265parse: initialize picture parameter set structure
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 1.7.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-17 13:39 UTC by Víctor Manuel Jáquez Leal
Modified: 2015-10-29 09:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
h264parse: initialize slice group id first (1.72 KB, patch)
2015-09-17 13:39 UTC, Víctor Manuel Jáquez Leal
needs-work Details | Review
codecparsers: h26{4,5}: init picture parameter set (3.07 KB, patch)
2015-09-17 17:21 UTC, Víctor Manuel Jáquez Leal
reviewed Details | Review
codecparser: h264: initialize parsing structures (7.02 KB, patch)
2015-09-18 10:40 UTC, Víctor Manuel Jáquez Leal
none Details | Review
codecparser: h265: initialize parsing structures (11.57 KB, patch)
2015-09-18 10:41 UTC, Víctor Manuel Jáquez Leal
committed Details | Review
codecparser: h264: initialize parsing structures (7.32 KB, patch)
2015-09-18 14:11 UTC, Víctor Manuel Jáquez Leal
committed Details | Review

Description Víctor Manuel Jáquez Leal 2015-09-17 13:39:50 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.
Comment 1 Víctor Manuel Jáquez Leal 2015-09-17 13:39:54 UTC
Created attachment 311553 [details] [review]
h264parse: initialize slice group id first
Comment 2 Sebastian Dröge (slomo) 2015-09-17 13:41:56 UTC
Is the same change required for h265parse? Do you have a test stream that crashes?
Comment 3 sreerenj 2015-09-17 13:50:25 UTC
(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?
Comment 4 Víctor Manuel Jáquez Leal 2015-09-17 13:55:12 UTC
(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
Comment 5 Sebastian Dröge (slomo) 2015-09-17 14:37:39 UTC
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
Comment 6 Víctor Manuel Jáquez Leal 2015-09-17 14:53:08 UTC
(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
Comment 7 Sebastian Dröge (slomo) 2015-09-17 15:06:56 UTC
(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 :)
Comment 8 sreerenj 2015-09-17 15:24:23 UTC
(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 9 Sebastian Dröge (slomo) 2015-09-17 16:32:16 UTC
Comment on attachment 311553 [details] [review]
h264parse: initialize slice group id first

Let's go with that then
Comment 10 Víctor Manuel Jáquez Leal 2015-09-17 17:21:56 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2015-09-17 17:25:45 UTC
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?
Comment 12 sreerenj 2015-09-17 17:31:25 UTC
Actually this is not just the case of PPS, we should have the memset() in whole APIs then:parse_sps, parse_slice_hdr etc... :)
Comment 13 Víctor Manuel Jáquez Leal 2015-09-18 10:40:54 UTC
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.
Comment 14 Víctor Manuel Jáquez Leal 2015-09-18 10:41:00 UTC
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.
Comment 15 Víctor Manuel Jáquez Leal 2015-09-18 10:45:45 UTC
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.
Comment 16 sreerenj 2015-09-18 12:32:09 UTC
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...
Comment 17 sreerenj 2015-09-18 12:33:46 UTC
Review of attachment 311622 [details] [review]:

lgtm,,
Comment 18 sreerenj 2015-09-18 12:39:03 UTC
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 :)
Comment 19 sreerenj 2015-09-18 12:44:17 UTC
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.
Comment 20 Víctor Manuel Jáquez Leal 2015-09-18 14:11:42 UTC
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.
Comment 21 sreerenj 2015-10-19 21:20:32 UTC
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 :)
Comment 22 Víctor Manuel Jáquez Leal 2015-10-20 14:20:03 UTC
Yuhu!! \o/
Comment 23 Sebastian Dröge (slomo) 2015-10-29 09:27:11 UTC
Attachment 311622 [details] pushed as c8b3d84 - codecparser: h265: initialize parsing structures
Attachment 311636 [details] pushed as d054a69 - codecparser: h264: initialize parsing structures