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

[Daniel Kang]

On Sat, Jan 8, 2011 at 9:33 AM, Justin Ruggles <justin.ruggles at gmail.com>wrote:

>  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
>

I have updated the patch with your comments. I am not sure if
sample_size can be negative or not, so I have just checked if it is not
zero.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcm_sanity_check.diff
Type: application/octet-stream
Size: 3634 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110108/20ea14ea/attachment.obj>