GNOME Bugzilla – Bug 737412
h264: playing a certain broken stream causes a crash (stack corruption caused by codec parser)
Last modified: 2015-01-14 11:46:25 UTC
Created attachment 287135 [details] [review] codecparser: h264: smash stack corruption bug Playing back a certain broken stream, causes the following crash on x86_64:
+ Trace 234131
It's OK in frame 2, but not OK in the caller frame 3. This means, that gst_h264_parse_process_nal() (or functions it calls itself) must have corrupted it. One obvious culprit would e.g. be somebody who is overwriting the stack object 'GstH264SliceHdr slice' (from gsth264parse.c gst_h264_parse_process_nal() around line 655). Further investigation shows that GstH264SliceHdr::ref_pic_list_modification_l0 and GstH264SliceHdr::ref_pic_list_modification_l1 are arrays with 32 entries each. The array is written to by slice_parse_ref_pic_list_modification_1() and indeed, it happens a lot that the while(1) in that function can end up writing beyond one of the two above arrays, causing stack corruption, and thus the crash. I have seen it writing into entries > 200, whereas there is space for 32 entries only. I am not sure if this means there is a bug in the h264 parser, where it doesn't behave as the spec mandates, or we have a corrupt stream, but attached is a fix to prevent it from writing into memory that it doesn't own.
The stack traces got a bit garbled - for x68_64, we get:
+ Trace 234132
Using a well placed breakpoint, we can also see that the pointer to nalu becomes corrupted in the calling function:
+ Trace 234133
Could you make a sample available that reproduces this?
Sorry, it took so long. I've put it there: https://drive.google.com/file/d/0BwcM3sY1O09ASXUyeTBnNW9Ddk0/edit?usp=sharing
It happens in several positions in this file, the first is at around 44 seconds.
seems like the bug is a bit older. I can reproduce the problem with a stock gstreamer 1.2.4 from Fedora 20 with Totem (crashes around 1:44, "stack smashing detected")
Ah, yes, I forgot to mention that 1.2 can't play it properly either. And the stack smashing detection does not detect all the cases of stack corruption, some cases are missed (of course). Using e.g. a more verbose version of my patch you should be able to see that corruption can happen silently.
Created attachment 294366 [details] [review] fix stack smashing This patch extends yours to fix the extra accesses you pointed to, and fixes an apparent discrepancy with the spec, which I found out when I tracked it down to see what was the expected way to deal with too many entries (turns out it doesn't say).
Review of attachment 294366 [details] [review]: Looks good except for one thing, please push after fixing that :) ::: gst-libs/gst/codecparsers/gsth264parser.c @@ +659,3 @@ } else { entries = slice->ref_pic_list_modification_l1; + max_entries = G_N_ELEMENTS (slice->ref_pic_list_modification_l0); This should be _l1
Doh. I'll have to think hard to top that one :D commit c73a5e0c545babe50130e7042be8cf92828afa18 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Mon Jan 12 17:24:52 2015 +0000 h264parser: fix stack smashing Ensure that we do not trust the bitstream when filling a table with a fixed max size. Additionally, the code was not quite matching what the spec says: - a value of 3 broke from the loop before adding an entry - an unhandled value did not add an entry The reference algorithm does these things differently (7.3.3.1 in ITU-T Rec. H.264 (05/2003)). This plays (apparently correctly) the original repro file, with no stack smashing. Based on a patch and bug report by André Draszik <git@andred.net>