diff mbox

[FFmpeg-devel] dashdec: Fix segfault on decoding segment timeline

Message ID 1516432032-4465-1-git-send-email-redmcg@redmandi.dyndns.org
State Accepted
Commit 4e3e8980b58fc22eb41c0e3cd3392bb4e6ca0184
Headers show

Commit Message

Brendan McGrath Jan. 20, 2018, 7:07 a.m. UTC
If first_seq_no is not within the bounds of timelines then a segfault
will occur.

This patch removes the use of first_seq_no within the timelines array

It also adds first_seq_no to the value returned by calc_next_seg_no_from_timelines
(which allows for different values of 'startNumber')

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---

This can be reproduced by creating a dash file with the following command:
ffmpeg -i in.ts -f dash -window_size 2 out.mpd

and then after about 10 seconds run ffprobe against it. 

The SegementTemplatei of out.mpd will look something like this:
  <SegmentTemplate timescale="48000" initialization="init-stream$RepresentationID$.m4s" media="chunk-stream$RepresentationID$-$Number%05d$.m4s" startNumber="7">
    <SegmentTimeline>
      <S t="2196480" d="480256" r="1" />
    </SegmentTimeline>
  </SegmentTemplate>

So the code was trying to access the 7th element within the timeline
array (which only had one element).

The patch doesn't worry about checking for a negative value after deducting the 60
seconds as calc_next_seg_no_from_timelines accepts a signed int and works correctly with
negative values.

 libavformat/dashdec.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Brendan McGrath Jan. 28, 2018, 1:09 a.m. UTC | #1
Just making sure this patch hasn't been overlooked.

To better explain the existing issue - the value of 'pls->first_seq_no' 
comes from the 'startNumber' attribute of the 'SegmentTemplate' element.

The 'pls->timelines' array represents the elements within the 
'SegmentTimeline' element.

So for this example:
   <SegmentTemplate timescale="48000" 
initialization="init-stream$RepresentationID$.m4s" 
media="chunk-stream$RepresentationID$-$Number%05d$.m4s" startNumber="7">
     <SegmentTimeline>
       <S t="2196480" d="480256" r="1" />
     </SegmentTimeline>
   </SegmentTemplate>

'pls->first_seq_no' will be 7 but the 'pls->timelines' array will only 
have the one element - hence:
pls->timelines[pls->first_seq_no]

causes a segfault.

This patch removes the use of 'pls->first_seq_no' within the 
'pls->timelines' array

On 20/01/18 18:07, Brendan McGrath wrote:
> If first_seq_no is not within the bounds of timelines then a segfault
> will occur.
>
> This patch removes the use of first_seq_no within the timelines array
>
> It also adds first_seq_no to the value returned by calc_next_seg_no_from_timelines
> (which allows for different values of 'startNumber')
>
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
>
> This can be reproduced by creating a dash file with the following command:
> ffmpeg -i in.ts -f dash -window_size 2 out.mpd
>
> and then after about 10 seconds run ffprobe against it.
>
> The SegementTemplatei of out.mpd will look something like this:
>    <SegmentTemplate timescale="48000" initialization="init-stream$RepresentationID$.m4s" media="chunk-stream$RepresentationID$-$Number%05d$.m4s" startNumber="7">
>      <SegmentTimeline>
>        <S t="2196480" d="480256" r="1" />
>      </SegmentTimeline>
>    </SegmentTemplate>
>
> So the code was trying to access the 7th element within the timeline
> array (which only had one element).
>
> The patch doesn't worry about checking for a negative value after deducting the 60
> seconds as calc_next_seg_no_from_timelines accepts a signed int and works correctly with
> negative values.
>
>   libavformat/dashdec.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index 2492f1d..cdb7ba5 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -1083,15 +1083,12 @@ static int64_t calc_cur_seg_no(AVFormatContext *s, struct representation *pls)
>           if (pls->n_fragments) {
>               num = pls->first_seq_no;
>           } else if (pls->n_timelines) {
> -            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - pls->timelines[pls->first_seq_no]->starttime; // total duration of playlist
> -            if (start_time_offset < 60 * pls->fragment_timescale)
> -                start_time_offset = 0;
> -            else
> -                start_time_offset = start_time_offset - 60 * pls->fragment_timescale;
> -
> -            num = calc_next_seg_no_from_timelines(pls, pls->timelines[pls->first_seq_no]->starttime + start_time_offset);
> +            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - 60 * pls->fragment_timescale; // 60 seconds before end
> +            num = calc_next_seg_no_from_timelines(pls, start_time_offset);
>               if (num == -1)
>                   num = pls->first_seq_no;
> +            else
> +                num += pls->first_seq_no;
>           } else if (pls->fragment_duration){
>               if (pls->presentation_timeoffset) {
>                   num = pls->presentation_timeoffset * pls->fragment_timescale / pls->fragment_duration;
Steven Liu Jan. 28, 2018, 11:29 a.m. UTC | #2
> 在 2018年1月28日,上午9:09,Brendan McGrath <redmcg@redmandi.dyndns.org> 写道:
> 
> Just making sure this patch hasn't been overlooked.
> 
> To better explain the existing issue - the value of 'pls->first_seq_no' comes from the 'startNumber' attribute of the 'SegmentTemplate' element.
> 
> The 'pls->timelines' array represents the elements within the 'SegmentTimeline' element.
> 
> So for this example:
>   <SegmentTemplate timescale="48000" initialization="init-stream$RepresentationID$.m4s" media="chunk-stream$RepresentationID$-$Number%05d$.m4s" startNumber="7">
>     <SegmentTimeline>
>       <S t="2196480" d="480256" r="1" />
>     </SegmentTimeline>
>   </SegmentTemplate>
> 
> 'pls->first_seq_no' will be 7 but the 'pls->timelines' array will only have the one element - hence:
> pls->timelines[pls->first_seq_no]
> 
will apply it
> causes a segfault.
> 
> This patch removes the use of 'pls->first_seq_no' within the 'pls->timelines' array
> 
>> On 20/01/18 18:07, Brendan McGrath wrote:
>> If first_seq_no is not within the bounds of timelines then a segfault
>> will occur.
>> 
>> This patch removes the use of first_seq_no within the timelines array
>> 
>> It also adds first_seq_no to the value returned by calc_next_seg_no_from_timelines
>> (which allows for different values of 'startNumber')
>> 
>> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
>> ---
>> 
>> This can be reproduced by creating a dash file with the following command:
>> ffmpeg -i in.ts -f dash -window_size 2 out.mpd
>> 
>> and then after about 10 seconds run ffprobe against it.
>> 
>> The SegementTemplatei of out.mpd will look something like this:
>>   <SegmentTemplate timescale="48000" initialization="init-stream$RepresentationID$.m4s" media="chunk-stream$RepresentationID$-$Number%05d$.m4s" startNumber="7">
>>     <SegmentTimeline>
>>       <S t="2196480" d="480256" r="1" />
>>     </SegmentTimeline>
>>   </SegmentTemplate>
>> 
>> So the code was trying to access the 7th element within the timeline
>> array (which only had one element).
>> 
>> The patch doesn't worry about checking for a negative value after deducting the 60
>> seconds as calc_next_seg_no_from_timelines accepts a signed int and works correctly with
>> negative values.
>> 
>>  libavformat/dashdec.c | 11 ++++-------
>>  1 file changed, 4 insertions(+), 7 deletions(-)
>> 
>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>> index 2492f1d..cdb7ba5 100644
>> --- a/libavformat/dashdec.c
>> +++ b/libavformat/dashdec.c
>> @@ -1083,15 +1083,12 @@ static int64_t calc_cur_seg_no(AVFormatContext *s, struct representation *pls)
>>          if (pls->n_fragments) {
>>              num = pls->first_seq_no;
>>          } else if (pls->n_timelines) {
>> -            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - pls->timelines[pls->first_seq_no]->starttime; // total duration of playlist
>> -            if (start_time_offset < 60 * pls->fragment_timescale)
>> -                start_time_offset = 0;
>> -            else
>> -                start_time_offset = start_time_offset - 60 * pls->fragment_timescale;
>> -
>> -            num = calc_next_seg_no_from_timelines(pls, pls->timelines[pls->first_seq_no]->starttime + start_time_offset);
>> +            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - 60 * pls->fragment_timescale; // 60 seconds before end
>> +            num = calc_next_seg_no_from_timelines(pls, start_time_offset);
>>              if (num == -1)
>>                  num = pls->first_seq_no;
>> +            else
>> +                num += pls->first_seq_no;
>>          } else if (pls->fragment_duration){
>>              if (pls->presentation_timeoffset) {
>>                  num = pls->presentation_timeoffset * pls->fragment_timescale / pls->fragment_duration;
>
Steven Liu Jan. 29, 2018, 2:48 a.m. UTC | #3
>>> 
>>> libavformat/dashdec.c | 11 ++++-------
>>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
>>> index 2492f1d..cdb7ba5 100644
>>> --- a/libavformat/dashdec.c
>>> +++ b/libavformat/dashdec.c
>>> @@ -1083,15 +1083,12 @@ static int64_t calc_cur_seg_no(AVFormatContext *s, struct representation *pls)
>>>         if (pls->n_fragments) {
>>>             num = pls->first_seq_no;
>>>         } else if (pls->n_timelines) {
>>> -            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - pls->timelines[pls->first_seq_no]->starttime; // total duration of playlist
>>> -            if (start_time_offset < 60 * pls->fragment_timescale)
>>> -                start_time_offset = 0;
>>> -            else
>>> -                start_time_offset = start_time_offset - 60 * pls->fragment_timescale;
>>> -
>>> -            num = calc_next_seg_no_from_timelines(pls, pls->timelines[pls->first_seq_no]->starttime + start_time_offset);
>>> +            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - 60 * pls->fragment_timescale; // 60 seconds before end
>>> +            num = calc_next_seg_no_from_timelines(pls, start_time_offset);
>>>             if (num == -1)
>>>                 num = pls->first_seq_no;
>>> +            else
>>> +                num += pls->first_seq_no;
>>>         } else if (pls->fragment_duration){
>>>             if (pls->presentation_timeoffset) {
>>>                 num = pls->presentation_timeoffset * pls->fragment_timescale / pls->fragment_duration;
>> 
> 
> 
> 
Pushed

Thanks
Steven
diff mbox

Patch

diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
index 2492f1d..cdb7ba5 100644
--- a/libavformat/dashdec.c
+++ b/libavformat/dashdec.c
@@ -1083,15 +1083,12 @@  static int64_t calc_cur_seg_no(AVFormatContext *s, struct representation *pls)
         if (pls->n_fragments) {
             num = pls->first_seq_no;
         } else if (pls->n_timelines) {
-            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - pls->timelines[pls->first_seq_no]->starttime; // total duration of playlist
-            if (start_time_offset < 60 * pls->fragment_timescale)
-                start_time_offset = 0;
-            else
-                start_time_offset = start_time_offset - 60 * pls->fragment_timescale;
-
-            num = calc_next_seg_no_from_timelines(pls, pls->timelines[pls->first_seq_no]->starttime + start_time_offset);
+            start_time_offset = get_segment_start_time_based_on_timeline(pls, 0xFFFFFFFF) - 60 * pls->fragment_timescale; // 60 seconds before end
+            num = calc_next_seg_no_from_timelines(pls, start_time_offset);
             if (num == -1)
                 num = pls->first_seq_no;
+            else
+                num += pls->first_seq_no;
         } else if (pls->fragment_duration){
             if (pls->presentation_timeoffset) {
                 num = pls->presentation_timeoffset * pls->fragment_timescale / pls->fragment_duration;