diff mbox

[FFmpeg-devel] sbgdec: prevent NULL pointer access

Message ID 4e7111c8-6fee-164f-9dd0-5ffcc9cfdbe8@googlemail.com
State Accepted
Commit dbefbb61b785cd77810c032f5cdb499d2a92df07
Headers show

Commit Message

Andreas Cadhalpun Nov. 10, 2016, 9:24 p.m. UTC 
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/sbgdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andreas Cadhalpun Nov. 22, 2016, 10:22 p.m. UTC | #1 
On 10.11.2016 22:24, Andreas Cadhalpun wrote:
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavformat/sbgdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
> index bb020d7..cbedd12 100644
> --- a/libavformat/sbgdec.c
> +++ b/libavformat/sbgdec.c
> @@ -927,7 +927,7 @@ static void expand_timestamps(void *log, struct sbg_script *s)
>          }
>      }
>      if (s->start_ts == AV_NOPTS_VALUE)
> -        s->start_ts = s->opt_start_at_first ? s->tseq[0].ts.t : now;
> +        s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
>      s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
>                  AV_NOPTS_VALUE; /* may be overridden later by -E option */
>      cur_ts = now;
> 

Ping. It would be good to have this fixed in 3.2.1.

Best regards,
Andreas
Josh Dekker Nov. 22, 2016, 11:01 p.m. UTC | #2 
On 22/11/2016 22:22, Andreas Cadhalpun wrote:
> On 10.11.2016 22:24, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavformat/sbgdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
>> index bb020d7..cbedd12 100644
>> --- a/libavformat/sbgdec.c
>> +++ b/libavformat/sbgdec.c
>> @@ -927,7 +927,7 @@ static void expand_timestamps(void *log, struct sbg_script *s)
>>          }
>>      }
>>      if (s->start_ts == AV_NOPTS_VALUE)
>> -        s->start_ts = s->opt_start_at_first ? s->tseq[0].ts.t : now;
>> +        s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
>>      s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
>>                  AV_NOPTS_VALUE; /* may be overridden later by -E option */
>>      cur_ts = now;
>>
>
> Ping. It would be good to have this fixed in 3.2.1.
>

I don't see how s->tseq can be NULL unless the functions are externally 
invoked without a proper state (which they shouldn't be because they're 
static).
Andreas Cadhalpun Nov. 22, 2016, 11:37 p.m. UTC | #3 
On 23.11.2016 00:01, Josh de Kock wrote:
> On 22/11/2016 22:22, Andreas Cadhalpun wrote:
>> On 10.11.2016 22:24, Andreas Cadhalpun wrote:
>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>> ---
>>>  libavformat/sbgdec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
>>> index bb020d7..cbedd12 100644
>>> --- a/libavformat/sbgdec.c
>>> +++ b/libavformat/sbgdec.c
>>> @@ -927,7 +927,7 @@ static void expand_timestamps(void *log, struct sbg_script *s)
>>>          }
>>>      }
>>>      if (s->start_ts == AV_NOPTS_VALUE)
>>> -        s->start_ts = s->opt_start_at_first ? s->tseq[0].ts.t : now;
>>> +        s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
>>>      s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
>>>                  AV_NOPTS_VALUE; /* may be overridden later by -E option */
>>>      cur_ts = now;
>>>
>>
>> Ping. It would be good to have this fixed in 3.2.1.
>>
> 
> I don't see how s->tseq can be NULL unless the functions are externally invoked without
> a proper state (which they shouldn't be because they're static).

It happens with simply using ffprobe on the sample.
The problem is that tseq is only allocated in parse_time_sequence, but
that function is not necessarily called.

Best regards,
Andreas
Josh Dekker Nov. 23, 2016, 12:06 a.m. UTC | #4 
On 22/11/2016 23:37, Andreas Cadhalpun wrote:
> On 23.11.2016 00:01, Josh de Kock wrote:
>> On 22/11/2016 22:22, Andreas Cadhalpun wrote:
>>> On 10.11.2016 22:24, Andreas Cadhalpun wrote:
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>> ---
>>>>  libavformat/sbgdec.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
>>>> index bb020d7..cbedd12 100644
>>>> --- a/libavformat/sbgdec.c
>>>> +++ b/libavformat/sbgdec.c
>>>> @@ -927,7 +927,7 @@ static void expand_timestamps(void *log, struct sbg_script *s)
>>>>          }
>>>>      }
>>>>      if (s->start_ts == AV_NOPTS_VALUE)
>>>> -        s->start_ts = s->opt_start_at_first ? s->tseq[0].ts.t : now;
>>>> +        s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
>>>>      s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
>>>>                  AV_NOPTS_VALUE; /* may be overridden later by -E option */
>>>>      cur_ts = now;
>>>>
>>>
>>> Ping. It would be good to have this fixed in 3.2.1.
>>>
>>
>> I don't see how s->tseq can be NULL unless the functions are externally invoked without
>> a proper state (which they shouldn't be because they're static).
> 
> It happens with simply using ffprobe on the sample.
> The problem is that tseq is only allocated in parse_time_sequence, but
> that function is not necessarily called.
>  

Ok. I see that now, at the very least this patch shouldn't have any
adverse effects--LGTM.
Andreas Cadhalpun Nov. 23, 2016, 12:23 a.m. UTC | #5 
On 23.11.2016 01:06, Josh de Kock wrote:
> On 22/11/2016 23:37, Andreas Cadhalpun wrote:
>> On 23.11.2016 00:01, Josh de Kock wrote:
>>> On 22/11/2016 22:22, Andreas Cadhalpun wrote:
>>>> On 10.11.2016 22:24, Andreas Cadhalpun wrote:
>>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>>> ---
>>>>>  libavformat/sbgdec.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
>>>>> index bb020d7..cbedd12 100644
>>>>> --- a/libavformat/sbgdec.c
>>>>> +++ b/libavformat/sbgdec.c
>>>>> @@ -927,7 +927,7 @@ static void expand_timestamps(void *log, struct sbg_script *s)
>>>>>          }
>>>>>      }
>>>>>      if (s->start_ts == AV_NOPTS_VALUE)
>>>>> -        s->start_ts = s->opt_start_at_first ? s->tseq[0].ts.t : now;
>>>>> +        s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
>>>>>      s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
>>>>>                  AV_NOPTS_VALUE; /* may be overridden later by -E option */
>>>>>      cur_ts = now;
>>>>>
>>>>
>>>> Ping. It would be good to have this fixed in 3.2.1.
>>>>
>>>
>>> I don't see how s->tseq can be NULL unless the functions are externally invoked without
>>> a proper state (which they shouldn't be because they're static).
>>
>> It happens with simply using ffprobe on the sample.
>> The problem is that tseq is only allocated in parse_time_sequence, but
>> that function is not necessarily called.
>>  
> 
> Ok. I see that now, at the very least this patch shouldn't have any
> adverse effects--LGTM.

Pushed.

Best regards,
Andreas
diff mbox

Patch

diff --git a/libavformat/sbgdec.c b/libavformat/sbgdec.c
index bb020d7..cbedd12 100644
--- a/libavformat/sbgdec.c
+++ b/libavformat/sbgdec.c
@@ -927,7 +927,7 @@  static void expand_timestamps(void *log, struct sbg_script *s)
         }
     }
     if (s->start_ts == AV_NOPTS_VALUE)
-        s->start_ts = s->opt_start_at_first ? s->tseq[0].ts.t : now;
+        s->start_ts = (s->opt_start_at_first && s->tseq) ? s->tseq[0].ts.t : now;
     s->end_ts = s->opt_duration ? s->start_ts + s->opt_duration :
                 AV_NOPTS_VALUE; /* may be overridden later by -E option */
     cur_ts = now;