[FFmpeg-devel] [PATCH] check for mod by zero (issue 2502)

[Justin Ruggles]

On 01/07/2011 11:33 PM, Daniel Kang wrote:

> On Fri, Jan 7, 2011 at 3:06 PM, Justin Ruggles <justin.ruggles at gmail.com>wrote:
> 
>>  In voc_get_packet(), if codec_id is invalid, don't read the packet.
>> or alternatively, set it only once for the first valid value, then later
>> instead of changing it if the value changes just print a warning and
>> read the packet.
>>
>> In pcm.c if codec_id is invalid, don't try to decode the packet. It will
>> screw up more than just the divide-by-zero.
>>
> 
> I have added a check in voc_get_packet() along with a check in the pcm
> decoder. I have kept the check for mod by zero.


> diff --git a/libavcodec/pcm.c b/libavcodec/pcm.c
> index b6b49dc..4e01ced 100644
> --- a/libavcodec/pcm.c
> +++ b/libavcodec/pcm.c
> @@ -272,6 +272,11 @@ static int pcm_decode_frame(AVCodecContext *avctx,
>      samples = data;
>      src = buf;
> 
> +    if (avctx->codec_id == CODEC_ID_NONE) {
> +        av_log(avctx, AV_LOG_ERROR, "invalid codec_id\n");
> +        return -1;
> +    }

return AVERROR(EINVAL);

>      if (avctx->sample_fmt!=avctx->codec->sample_fmts[0]) {
>          av_log(avctx, AV_LOG_ERROR, "invalid sample_fmt\n");
>          return -1;
> @@ -294,8 +299,8 @@ static int pcm_decode_frame(AVCodecContext *avctx,
> 
>      n = avctx->channels * sample_size;
> 
> -    if(n && buf_size % n){
> -        if (buf_size < n) {
> +    if((n == 0)||(n && buf_size % n)){
> +        if ((n == 0) || (buf_size < n)) {
>              av_log(avctx, AV_LOG_ERROR, "invalid PCM packet\n");
>              return -1;
>          }else


if you just add a check to ensure sample_size is not 0 then you don't
have to worry about the mod by 0 check because n will never be 0.

> diff --git a/libavformat/vocdec.c b/libavformat/vocdec.c
> index 909520c..6c37246 100644
> --- a/libavformat/vocdec.c
> +++ b/libavformat/vocdec.c
> @@ -68,7 +68,7 @@ voc_get_packet(AVFormatContext *s, AVPacket *pkt, AVStream *st, int max_size)
>      AVCodecContext *dec = st->codec;
>      ByteIOContext *pb = s->pb;
>      VocType type;
> -    int size;
> +    int size, tmp;
>      int sample_rate = 0;
>      int channels = 1;
> 
> @@ -90,7 +90,11 @@ voc_get_packet(AVFormatContext *s, AVPacket *pkt, AVStream *st, int max_size)
>              if (sample_rate)
>                  dec->sample_rate = sample_rate;
>              dec->channels = channels;
> -            dec->codec_id = ff_codec_get_id(ff_voc_codec_tags, get_byte(pb));
> +            tmp = ff_codec_get_id(ff_voc_codec_tags, get_byte(pb));
> +            if (dec->codec_id != CODEC_ID_NONE)
> +                dec->codec_id = ff_codec_get_id(ff_voc_codec_tags, get_byte(pb));
> +            else
> +                av_log(s, AV_LOG_ERROR, "Unknown codec ID, continuing to decode\n");
>              dec->bits_per_coded_sample = av_get_bits_per_sample(dec->codec_id);
>              voc->remaining_size -= 2;
>              max_size -= 2;
> @@ -113,7 +117,11 @@ voc_get_packet(AVFormatContext *s, AVPacket *pkt, AVStream *st, int max_size)
>              dec->sample_rate = get_le32(pb);
>              dec->bits_per_coded_sample = get_byte(pb);
>              dec->channels = get_byte(pb);
> -            dec->codec_id = ff_codec_get_id(ff_voc_codec_tags, get_le16(pb));
> +            tmp = ff_codec_get_id(ff_voc_codec_tags, get_byte(pb));
> +            if (dec->codec_id != CODEC_ID_NONE)
> +                dec->codec_id = ff_codec_get_id(ff_voc_codec_tags, get_byte(pb));
> +            else
> +                av_log(s, AV_LOG_ERROR, "Unknown codec ID, continuing to decode\n");
>              url_fskip(pb, 4);
>              voc->remaining_size -= 12;
>              max_size -= 12;


This is not correct.  It is reading the codec code twice. It is not
checking to make sure the final codec_id != CODEC_ID_NONE.  And it will
never set a valid codec_id. (note that in this case codec_id is not set
at read_header() as is normally the case for demuxers. it is set the
first time an audio packet is read)

My suggestion:
switch(){
...
case VOC_TYPE_VOICE_DATA:
    ...
    tmp_codec = ff_codec_get_id(ff_voc_codec_tags, get_byte(pb));
    if (dec->codec_id == CODEC_ID_NONE)
        dec->codec_id = tmp_codec;
    else if (tmp_codec != dec->codec_id)
        print AV_LOG_WARNING message about ignoring changed codec code
    ...
case VOC_TYPE_NEW_VOICE_DATA:
    same as above
    ...
}
if (dec->codec_id == CODEC_ID_NONE) {
    print error message
    return AVERROR(EINVAL);
}

-Justin