GNOME Bugzilla – Bug 722081
h265parse: Fix segfault when parsing VPS
Last modified: 2014-01-21 06:42:38 UTC
Created attachment 266131 [details] [review] base with 2e9e8b5 dvb: Use DVB_API_VERSION to know if we have recent enough version I saw the segfault with some test stream. This test stream has no "vps_sub_layer_ordering_info_present_flag". and The gsth265parser.c:1756 and gsth265parser.c:1927 is some problem. ----------------------------------------------------------- for (i = 0; i <= (vps->max_sub_layers_minus1 - 1); i++) { if vps->max_sub_layers_minus1 is zero , then (vps->max_sub_layers_minus1-1) is 0xFFFF. ----------------------------------------------------------- so I fixed this problem. Could you check it?
Could you provide this without all the code style changes? Maybe first do a patch that just has the gst-indent stuff, and then on top of that these changes. Also don't run gst-indent on headers please :) Can you also provide a sample file that reproduces the segfault?
Created attachment 266224 [details] [review] new patch set base with HEAD I fixed indent for gsth265parser.c
Created attachment 266225 [details] Test Stream for reproduce This test stream has no "vps_sub_layer_ordering_info_present_flag". You can reproduce. Actually, I want delivery to you fully. But I can't This demo stream made for LG Electronics.
Review of attachment 266224 [details] [review]: ::: gst-libs/gst/codecparsers/gsth265parser.c @@ -1716,3 @@ /* setting default values if vps->sub_layer_ordering_info_present_flag is zero */ if (!vps->sub_layer_ordering_info_present_flag) { - for (i = 0; i <= (vps->max_sub_layers_minus1 - 1); i++) { This doesn't look right. Should you just set all vps->max_sub_layers_minus1 of these instead of all possible? Maybe just keep the current code and check for vps->max_sub_layers_minus1 == 0 before going into the loop? ::: gst-libs/gst/codecparsers/gsth265parser.h @@ +33,3 @@ G_BEGIN_DECLS +#define GST_H265_MAX_SUB_LAYERS 7 Are you sure 7 is correct? vps->max_sub_layers_minus1 is 3 bits, so can hold 8 values. And the vps->max_sub_layers_minus1 -th element of the array is accessed, so array[8] in the worst case, needing a 9 element large array. Does the spec specify something about this here?
Actually, I confused max sub layers value. and it can hold 8 value. I checked latest HM reference software. There MAX_SUB_LAYER is 8. I think MAX_SUB_LAYER 8 is correct. What about your think ?
max_sub_layers_minus1 can be 8, so I assume MAX_SUB_LAYER should be 9?
I don't think so. according to 7.4.3.1 --------------------------------------------------------------------------- vps_max_sub_layers_minus1 : plus 1 specifies the maximum number of temporal sub-layers that may be present in the CVS. The value of vps_max_sub_layers_minus1 shall be in the range of 0 to 6, inclusive. --------------------------------------------------------------------------- If we are read 3 bit( vps_max_sub_layers_minus1 ) with 6, plus 1 and array will be 8 ( 0~ 7 ). so MAX_SUB_LAYER should be 8. You can find it. ( according to HM1.2 source code ) https://hevc.hhi.fraunhofer.de/trac/hevc/browser/tags/HM-12.1/source/Lib/TLibCommon/TComSlice.h : 403 ~ 405 line https://hevc.hhi.fraunhofer.de/trac/hevc/browser/tags/HM-12.1/source/Lib/TLibDecoder/TDecCAVLC.cpp : 667 line ~ it MAX_TLAYER is 8. I think, this way is right.
So it seems, can you update the patch please? :) Also we should check that we don't read 7 from that field then and error out in that case.
Thanks for your support. You can check new patch.
Created attachment 266263 [details] [review] new patchset based on e8d569f faceblur: set maximum feature size to 0x0 modified MAX_SUB_LAYERS value.
commit a8151d78fc946c142d63682055adad9534083ac6 Author: duhui.lee <duhui.lee@lge.com> Date: Tue Jan 14 23:21:25 2014 +0900 h265parser: Fix segfault when parsing VPS https://bugzilla.gnome.org/show_bug.cgi?id=722081