[07/17] riffdec: Avoid a division by zero

Message ID 1380406879-6174-7-git-send-email-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Sept. 28, 2013, 10:21 p.m.
Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC: libav-stable@libav.org
---
 libavformat/riffdec.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Kostya Shishkov Sept. 29, 2013, 6:01 a.m. | #1
On Sun, Sep 29, 2013 at 01:21:09AM +0300, Martin Storsjö wrote:
> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC: libav-stable@libav.org
> ---
>  libavformat/riffdec.c |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/riffdec.c b/libavformat/riffdec.c
> index 447a686..1927b82 100644
> --- a/libavformat/riffdec.c
> +++ b/libavformat/riffdec.c
> @@ -127,8 +127,14 @@ int ff_get_wav_header(AVIOContext *pb, AVCodecContext *codec, int size)
>          codec->sample_rate = 0;
>      }
>      /* override bits_per_coded_sample for G.726 */
> -    if (codec->codec_id == AV_CODEC_ID_ADPCM_G726)
> +    if (codec->codec_id == AV_CODEC_ID_ADPCM_G726) {
> +        if (codec->sample_rate <= 0) {
> +            av_log(NULL, AV_LOG_ERROR,
> +                   "Invalid sample rate for G726: %d\n", codec->sample_rate);
> +            return AVERROR_INVALIDDATA;
> +        }
>          codec->bits_per_coded_sample = codec->bit_rate / codec->sample_rate;
> +    }
>  
>      return 0;
>  }
> -- 

this makes me wonder why we should allow such sample rates for other codecs
Martin Storsjö Sept. 29, 2013, 9:49 a.m. | #2
On Sun, 29 Sep 2013, Kostya Shishkov wrote:

> On Sun, Sep 29, 2013 at 01:21:09AM +0300, Martin Storsjö wrote:
>> Reported-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
>> CC: libav-stable@libav.org
>> ---
>>  libavformat/riffdec.c |    8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavformat/riffdec.c b/libavformat/riffdec.c
>> index 447a686..1927b82 100644
>> --- a/libavformat/riffdec.c
>> +++ b/libavformat/riffdec.c
>> @@ -127,8 +127,14 @@ int ff_get_wav_header(AVIOContext *pb, AVCodecContext *codec, int size)
>>          codec->sample_rate = 0;
>>      }
>>      /* override bits_per_coded_sample for G.726 */
>> -    if (codec->codec_id == AV_CODEC_ID_ADPCM_G726)
>> +    if (codec->codec_id == AV_CODEC_ID_ADPCM_G726) {
>> +        if (codec->sample_rate <= 0) {
>> +            av_log(NULL, AV_LOG_ERROR,
>> +                   "Invalid sample rate for G726: %d\n", codec->sample_rate);
>> +            return AVERROR_INVALIDDATA;
>> +        }
>>          codec->bits_per_coded_sample = codec->bit_rate / codec->sample_rate;
>> +    }
>>
>>      return 0;
>>  }
>> --
>
> this makes me wonder why we should allow such sample rates for other codecs

I thought about this as well, and I guess we could try to check it for all 
cases. There's a special case for AAC_LATM where we intentionally set it 
to zero, but I guess we could place the check before that (and only allow 
it to be left at zero in that case). Or would this break any other known 
case where it's ok to have it set to zero in the wav header? (It passes 
all fate tests but that doesn't tell half the story of how it's used in 
practice...)

// Martin

Patch

diff --git a/libavformat/riffdec.c b/libavformat/riffdec.c
index 447a686..1927b82 100644
--- a/libavformat/riffdec.c
+++ b/libavformat/riffdec.c
@@ -127,8 +127,14 @@  int ff_get_wav_header(AVIOContext *pb, AVCodecContext *codec, int size)
         codec->sample_rate = 0;
     }
     /* override bits_per_coded_sample for G.726 */
-    if (codec->codec_id == AV_CODEC_ID_ADPCM_G726)
+    if (codec->codec_id == AV_CODEC_ID_ADPCM_G726) {
+        if (codec->sample_rate <= 0) {
+            av_log(NULL, AV_LOG_ERROR,
+                   "Invalid sample rate for G726: %d\n", codec->sample_rate);
+            return AVERROR_INVALIDDATA;
+        }
         codec->bits_per_coded_sample = codec->bit_rate / codec->sample_rate;
+    }
 
     return 0;
 }