diff mbox

[FFmpeg-devel] aiffdec: fix division by zero

Message ID e71a5cd2-13c6-e06f-479f-55d9e20ca9a3@googlemail.com
State Accepted
Commit c143a9c96ff907a8fe4598529664aec7cb156708
Headers show

Commit Message

Andreas Cadhalpun Oct. 16, 2016, 8:38 p.m. UTC 
Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/aiffdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer Oct. 17, 2016, 3:43 a.m. UTC | #1 
On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote:
> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> ---
>  libavformat/aiffdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
> index cd916f9..de82787 100644
> --- a/libavformat/aiffdec.c
> +++ b/libavformat/aiffdec.c
> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s,
>          size = st->codecpar->block_align;
>          break;
>      default:
> -        size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align;
> +        size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE;

how do you reach block_align == 0 ?
aiff_read_header() checks for block_align == 0

[...]
Andreas Cadhalpun Oct. 17, 2016, 2:17 p.m. UTC | #2 
On 17.10.2016 05:43, Michael Niedermayer wrote:
> On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote:
>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> ---
>>  libavformat/aiffdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
>> index cd916f9..de82787 100644
>> --- a/libavformat/aiffdec.c
>> +++ b/libavformat/aiffdec.c
>> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s,
>>          size = st->codecpar->block_align;
>>          break;
>>      default:
>> -        size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align;
>> +        size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE;
> 
> how do you reach block_align == 0 ?
> aiff_read_header() checks for block_align == 0

I'm not aware of a way to reproduce this with the ffmpeg binary, however
an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type
and codecpar->codec_id to force decoding a stream with a particular codec.

However, avcodec_parameters_from_context sets codecpar->block_align to 0
for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash.

It's similar for the other two division by zero patches.

So an alternative approach would be to set all parameter fields from the codec
in avcodec_parameters_from_context, but I prefer making the code more robust.

Best regards,
Andreas
Michael Niedermayer Oct. 17, 2016, 3:13 p.m. UTC | #3 
On Mon, Oct 17, 2016 at 04:17:35PM +0200, Andreas Cadhalpun wrote:
> On 17.10.2016 05:43, Michael Niedermayer wrote:
> > On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote:
> >> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >> ---
> >>  libavformat/aiffdec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
> >> index cd916f9..de82787 100644
> >> --- a/libavformat/aiffdec.c
> >> +++ b/libavformat/aiffdec.c
> >> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s,
> >>          size = st->codecpar->block_align;
> >>          break;
> >>      default:
> >> -        size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align;
> >> +        size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE;
> > 
> > how do you reach block_align == 0 ?
> > aiff_read_header() checks for block_align == 0
> 
> I'm not aware of a way to reproduce this with the ffmpeg binary, however
> an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type
> and codecpar->codec_id to force decoding a stream with a particular codec.
> 
> However, avcodec_parameters_from_context sets codecpar->block_align to 0
> for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash.

hmm, patch is probably ok then

thx

[...]
Andreas Cadhalpun Oct. 17, 2016, 4:27 p.m. UTC | #4 
On 17.10.2016 17:13, Michael Niedermayer wrote:
> On Mon, Oct 17, 2016 at 04:17:35PM +0200, Andreas Cadhalpun wrote:
>> On 17.10.2016 05:43, Michael Niedermayer wrote:
>>> On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote:
>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>> ---
>>>>  libavformat/aiffdec.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
>>>> index cd916f9..de82787 100644
>>>> --- a/libavformat/aiffdec.c
>>>> +++ b/libavformat/aiffdec.c
>>>> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s,
>>>>          size = st->codecpar->block_align;
>>>>          break;
>>>>      default:
>>>> -        size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align;
>>>> +        size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE;
>>>
>>> how do you reach block_align == 0 ?
>>> aiff_read_header() checks for block_align == 0
>>
>> I'm not aware of a way to reproduce this with the ffmpeg binary, however
>> an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type
>> and codecpar->codec_id to force decoding a stream with a particular codec.
>>
>> However, avcodec_parameters_from_context sets codecpar->block_align to 0
>> for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash.
> 
> hmm, patch is probably ok then

Pushed.

What about the similar patches for astdec and westwood_aud?

Best regards,
Andreas
Michael Niedermayer Oct. 17, 2016, 6:11 p.m. UTC | #5 
On Mon, Oct 17, 2016 at 06:27:29PM +0200, Andreas Cadhalpun wrote:
> On 17.10.2016 17:13, Michael Niedermayer wrote:
> > On Mon, Oct 17, 2016 at 04:17:35PM +0200, Andreas Cadhalpun wrote:
> >> On 17.10.2016 05:43, Michael Niedermayer wrote:
> >>> On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote:
> >>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> >>>> ---
> >>>>  libavformat/aiffdec.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
> >>>> index cd916f9..de82787 100644
> >>>> --- a/libavformat/aiffdec.c
> >>>> +++ b/libavformat/aiffdec.c
> >>>> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s,
> >>>>          size = st->codecpar->block_align;
> >>>>          break;
> >>>>      default:
> >>>> -        size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align;
> >>>> +        size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE;
> >>>
> >>> how do you reach block_align == 0 ?
> >>> aiff_read_header() checks for block_align == 0
> >>
> >> I'm not aware of a way to reproduce this with the ffmpeg binary, however
> >> an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type
> >> and codecpar->codec_id to force decoding a stream with a particular codec.
> >>
> >> However, avcodec_parameters_from_context sets codecpar->block_align to 0
> >> for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash.
> > 
> > hmm, patch is probably ok then
> 
> Pushed.
> 
> What about the similar patches for astdec and westwood_aud?

probably ok too

[...]
Andreas Cadhalpun Oct. 17, 2016, 6:47 p.m. UTC | #6 
On 17.10.2016 20:11, Michael Niedermayer wrote:
> On Mon, Oct 17, 2016 at 06:27:29PM +0200, Andreas Cadhalpun wrote:
>> On 17.10.2016 17:13, Michael Niedermayer wrote:
>>> On Mon, Oct 17, 2016 at 04:17:35PM +0200, Andreas Cadhalpun wrote:
>>>> On 17.10.2016 05:43, Michael Niedermayer wrote:
>>>>> On Sun, Oct 16, 2016 at 10:38:42PM +0200, Andreas Cadhalpun wrote:
>>>>>> Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>>>>>> ---
>>>>>>  libavformat/aiffdec.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
>>>>>> index cd916f9..de82787 100644
>>>>>> --- a/libavformat/aiffdec.c
>>>>>> +++ b/libavformat/aiffdec.c
>>>>>> @@ -380,7 +380,7 @@ static int aiff_read_packet(AVFormatContext *s,
>>>>>>          size = st->codecpar->block_align;
>>>>>>          break;
>>>>>>      default:
>>>>>> -        size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align;
>>>>>> +        size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE;
>>>>>
>>>>> how do you reach block_align == 0 ?
>>>>> aiff_read_header() checks for block_align == 0
>>>>
>>>> I'm not aware of a way to reproduce this with the ffmpeg binary, however
>>>> an API user (e.g. my fuzz-testing-program) can change codecpar->codec_type
>>>> and codecpar->codec_id to force decoding a stream with a particular codec.
>>>>
>>>> However, avcodec_parameters_from_context sets codecpar->block_align to 0
>>>> for AVMEDIA_TYPE_VIDEO thus causing the subsequent crash.
>>>
>>> hmm, patch is probably ok then
>>
>> Pushed.
>>
>> What about the similar patches for astdec and westwood_aud?
> 
> probably ok too

Thanks, pushed both.

Best regards,
Andreas
diff mbox

Patch

diff --git a/libavformat/aiffdec.c b/libavformat/aiffdec.c
index cd916f9..de82787 100644
--- a/libavformat/aiffdec.c
+++ b/libavformat/aiffdec.c
@@ -380,7 +380,7 @@  static int aiff_read_packet(AVFormatContext *s,
         size = st->codecpar->block_align;
         break;
     default:
-        size = (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align;
+        size = st->codecpar->block_align ? (MAX_SIZE / st->codecpar->block_align) * st->codecpar->block_align : MAX_SIZE;
     }
     size = FFMIN(max_size, size);
     res = av_get_packet(s->pb, pkt, size);