[FFmpeg-devel] [libav-devel] [PATCH] riffdec: error out on negative bit rate

[Andreas Cadhalpun]

On 11.07.2015 18:13, Luca Barbato wrote:
> On 11/07/15 17:22, Andreas Cadhalpun wrote:
>> On 11.07.2015 11:44, Luca Barbato wrote:
>>> On 11/07/15 01:26, Andreas Cadhalpun wrote:
>>>> Note however that the explode mode doesn't necessarily work, because e.g.
>>>> the avi demuxer doesn't set err_recognition for the AVCodecContext.
>>>
>>> It wouldn't work as intended, setting the bit_rate to 0 thought, does
>>> prevent an issue with g726 but probably that chunk of code could be removed.
>>>
>>> Some patches following...
>>
>> After your riff patch, it is possible to the the explode mode working.
>> Updated patch attached.
>>
> 
> @@ -79,6 +79,7 @@ int ff_get_wav_header(AVFormatContext *s, AVIOContext *pb,
>                        AVCodecContext *codec, int size)
>  {
>      int id;
> +    uint64_t bitrate;
> 
>      if (size < 14)
>          return AVERROR_INVALIDDATA;
> @@ -87,7 +88,7 @@ int ff_get_wav_header(AVFormatContext *s, AVIOContext *pb,
>      codec->codec_type  = AVMEDIA_TYPE_AUDIO;
>      codec->channels    = avio_rl16(pb);
>      codec->sample_rate = avio_rl32(pb);
> -    codec->bit_rate    = avio_rl32(pb) * 8;
> +    bitrate            = avio_rl32(pb) * 8;
>      codec->block_align = avio_rl16(pb);
>      if (size == 14) {  /* We're dealing with plain vanilla WAVEFORMAT */
>          codec->bits_per_coded_sample = 8;
> @@ -124,6 +125,23 @@ int ff_get_wav_header(AVFormatContext *s,
> AVIOContext *pb,
>          if (size > 0)
>              avio_skip(pb, size);
>      }
> +
> +    if (bitrate > INT_MAX) {
> +        if (s->error_recognition & AV_EF_EXPLODE) {
> +            av_log(s, AV_LOG_ERROR,
> +                   "The bitrate %"PRIu64" is too large.\n",
> +                    bitrate);
> +            return AVERROR_INVALIDDATA;
> +        } else {
> +            av_log(s, AV_LOG_WARNING,
> +                   "The bitrate %"PRIu64" is too large, resetting to 0.",
> +                   bitrate);
> +            codec->bit_rate = 0;
> +        }
> +    } else {
> +        codec->bit_rate = bitrate;
> +    }
> +
> 
> This is possibly safer, the sample_rate field might enjoy the same
> treatment, bonus point for whoever know which is the correct upper bound
> for those fields =)

That's fine as well (and avoids said undefined integer overflow).

Best regards,
Andreas