diff mbox series

[FFmpeg-devel] dashenc: check pts to prevent division by zero error

Message ID 21f10d45-db51-62fd-9fdd-3d01b8eecd3e@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] dashenc: check pts to prevent division by zero error
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork fail Failed to apply patch

Commit Message

Alfred E. Heggestad Jan. 30, 2020, 9:58 a.m. UTC
this usecase will cause a division by zero trap:

1. dashenc has received one frame
2. os->max_pts and os->start_pts have same value
3. delta between max_pts and start_pts is 0
4. av_rescale_q(0, x, y) returns 0
5. this value is used as denominator in division
6. Bang! -> segfault

this fix checks that max_pts > start_pts.
the fix has been tested and works.

please review and suggest better fixes.

Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com>
---
  libavformat/dashenc.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

              os->muxer_overhead = ((int64_t) (range_length - 
os->total_pkt_size) *
                                    8 * AV_TIME_BASE) /
                                   av_rescale_q(os->max_pts - os->start_pts,

Comments

Jeyapal, Karthick Jan. 31, 2020, 1:08 p.m. UTC | #1
On 1/30/20 3:28 PM, Alfred E. Heggestad wrote:
> this usecase will cause a division by zero trap:
>
> 1. dashenc has received one frame
> 2. os->max_pts and os->start_pts have same value
> 3. delta between max_pts and start_pts is 0
> 4. av_rescale_q(0, x, y) returns 0
> 5. this value is used as denominator in division
> 6. Bang! -> segfault
>
> this fix checks that max_pts > start_pts.
> the fix has been tested and works.
>
> please review and suggest better fixes.
>
> Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com>
> ---
>   libavformat/dashenc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
> index f555f208a7..3b651b9514 100644
> --- a/libavformat/dashenc.c
> +++ b/libavformat/dashenc.c
> @@ -1883,7 +1883,7 @@ static int dash_flush(AVFormatContext *s, int 
> final, int stream)
>  
> st->time_base,
>  
> AV_TIME_BASE_Q));
>
> -        if (!os->muxer_overhead)
> +        if (!os->muxer_overhead && os->max_pts > os->start_pts)
>               os->muxer_overhead = ((int64_t) (range_length - 
> os->total_pkt_size) *
>                                     8 * AV_TIME_BASE) /
>                                    av_rescale_q(os->max_pts - os->start_pts,
LGTM.

This actually exposes a corner case bug in overhead calculation logic. 
Guess we will need to come up with a better logic for it.
Until then, this fix will atleast make sure things don’t crash.

Thanks,
Karthick
James Almer Jan. 31, 2020, 1:17 p.m. UTC | #2
On 1/31/2020 10:08 AM, Jeyapal, Karthick wrote:
> 
> On 1/30/20 3:28 PM, Alfred E. Heggestad wrote:
>> this usecase will cause a division by zero trap:
>>
>> 1. dashenc has received one frame
>> 2. os->max_pts and os->start_pts have same value
>> 3. delta between max_pts and start_pts is 0
>> 4. av_rescale_q(0, x, y) returns 0
>> 5. this value is used as denominator in division
>> 6. Bang! -> segfault
>>
>> this fix checks that max_pts > start_pts.
>> the fix has been tested and works.
>>
>> please review and suggest better fixes.
>>
>> Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com>
>> ---
>>   libavformat/dashenc.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>> index f555f208a7..3b651b9514 100644
>> --- a/libavformat/dashenc.c
>> +++ b/libavformat/dashenc.c
>> @@ -1883,7 +1883,7 @@ static int dash_flush(AVFormatContext *s, int 
>> final, int stream)
>>  
>> st->time_base,
>>  
>> AV_TIME_BASE_Q));
>>
>> -        if (!os->muxer_overhead)
>> +        if (!os->muxer_overhead && os->max_pts > os->start_pts)
>>               os->muxer_overhead = ((int64_t) (range_length - 
>> os->total_pkt_size) *
>>                                     8 * AV_TIME_BASE) /
>>                                    av_rescale_q(os->max_pts - os->start_pts,
> LGTM.
> 
> This actually exposes a corner case bug in overhead calculation logic. 
> Guess we will need to come up with a better logic for it.
> Until then, this fix will atleast make sure things don’t crash.
> 
> Thanks,
> Karthick

Applied.
Alfred E. Heggestad Jan. 31, 2020, 4:18 p.m. UTC | #3
On 31/01/2020 14:17, James Almer wrote:
> On 1/31/2020 10:08 AM, Jeyapal, Karthick wrote:
>>
>> On 1/30/20 3:28 PM, Alfred E. Heggestad wrote:
>>> this usecase will cause a division by zero trap:
>>>
>>> 1. dashenc has received one frame
>>> 2. os->max_pts and os->start_pts have same value
>>> 3. delta between max_pts and start_pts is 0
>>> 4. av_rescale_q(0, x, y) returns 0
>>> 5. this value is used as denominator in division
>>> 6. Bang! -> segfault
>>>
>>> this fix checks that max_pts > start_pts.
>>> the fix has been tested and works.
>>>
>>> please review and suggest better fixes.
>>>
>>> Signed-off-by: Alfred E. Heggestad <alfred.heggestad@gmail.com>
>>> ---
>>>    libavformat/dashenc.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
>>> index f555f208a7..3b651b9514 100644
>>> --- a/libavformat/dashenc.c
>>> +++ b/libavformat/dashenc.c
>>> @@ -1883,7 +1883,7 @@ static int dash_flush(AVFormatContext *s, int
>>> final, int stream)
>>>   
>>> st->time_base,
>>>   
>>> AV_TIME_BASE_Q));
>>>
>>> -        if (!os->muxer_overhead)
>>> +        if (!os->muxer_overhead && os->max_pts > os->start_pts)
>>>                os->muxer_overhead = ((int64_t) (range_length -
>>> os->total_pkt_size) *
>>>                                      8 * AV_TIME_BASE) /
>>>                                     av_rescale_q(os->max_pts - os->start_pts,
>> LGTM.
>>
>> This actually exposes a corner case bug in overhead calculation logic.
>> Guess we will need to come up with a better logic for it.
>> Until then, this fix will atleast make sure things don’t crash.
>>
>> Thanks,
>> Karthick
> 
> Applied.

Great!

Thanks Jeyapal and James.



/alfred
diff mbox series

Patch

diff --git a/libavformat/dashenc.c b/libavformat/dashenc.c
index f555f208a7..3b651b9514 100644
--- a/libavformat/dashenc.c
+++ b/libavformat/dashenc.c
@@ -1883,7 +1883,7 @@  static int dash_flush(AVFormatContext *s, int 
final, int stream)
 
st->time_base,
 
AV_TIME_BASE_Q));

-        if (!os->muxer_overhead)
+        if (!os->muxer_overhead && os->max_pts > os->start_pts)