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

Michael Niedermayer michaelni
Tue Jan 11 13:29:10 CET 2011


On Mon, Jan 10, 2011 at 09:43:54PM -0500, Daniel Kang wrote:
> On Mon, Jan 10, 2011 at 8:11 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> >  On Mon, Jan 10, 2011 at 07:01:02PM -0500, Daniel Kang wrote:
> > > On Mon, Jan 10, 2011 at 3:51 PM, Michael Niedermayer <michaelni at gmx.at
> > >wrote:
> > >
> > > >  On Sat, Jan 08, 2011 at 04:39:35PM -0500, Daniel Kang wrote:
> > > > > On Sat, Jan 8, 2011 at 3:45 PM, Justin Ruggles <
> > justin.ruggles at gmail.com
> > > > >wrote:
> > > > >
> > > > > > On 01/08/2011 01:52 PM, Daniel Kang wrote:
> > > > > >
> > > > > > > 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:
> > > > > >  >>> 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.
> > > > > >
> > > > > >
> > > > > > > +            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 (dec->codec_id != tmp_codec)
> > > > > > > +                av_log(s, AV_LOG_WARNING, "Ignoring change in
> > codec,
> > > > > > continuing to decode\n");
> > > > > >
> > > > > >
> > > > > > Technically, the demuxer does not decode, it demuxes.  I would just
> > say
> > > > > > "Ignoring mid-stream change in audio codec".
> > > > > >
> > > > > > -Justin
> > > > > >
> > > > >
> > > > > I have updated the message.
> > > >
> > > > >  libavcodec/pcm.c     |   10 ++++++++++
> > > > >  libavformat/vocdec.c |   18 +++++++++++++++---
> > > > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > > > 65c7d12fe75a01827377e1adb4dfa2a7b7015dd6  pcm_sanity_check.diff
> > > > > From f9905f8b8c077d079951344c7d25646589c4c5aa Mon Sep 17 00:00:00
> > 2001
> > > > > From: Daniel Kang <daniel.d.kang at gmail.com>
> > > > > Date: Thu, 6 Jan 2011 21:03:27 -0500
> > > > > Subject: [PATCH] Add check for pcm files.
> > > > >
> > > > > ---
> > > > >  libavcodec/pcm.c     |   10 ++++++++++
> > > > >  libavformat/vocdec.c |   18 +++++++++++++++---
> > > > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/libavcodec/pcm.c b/libavcodec/pcm.c
> > > > > index b6b49dc..e321a4d 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 AVERROR(EINVAL);
> > > > > +    }
> > > > > +
> > > > >      if (avctx->sample_fmt!=avctx->codec->sample_fmts[0]) {
> > > > >          av_log(avctx, AV_LOG_ERROR, "invalid sample_fmt\n");
> > > > >          return -1;
> > > > > @@ -292,6 +297,11 @@ static int pcm_decode_frame(AVCodecContext
> > *avctx,
> > > > >          /* we process 40-bit blocks per channel for LXF */
> > > > >          sample_size = 5;
> > > > >
> > > > > +    if (sample_size == 0) {
> > > > > +        av_log(avctx, AV_LOG_ERROR, "Invalid sample_size\n");
> > > > > +        return AVERROR(EINVAL);
> > > > > +    }
> > > >
> > > > the codec_id test looks unneeded
> > >
> > >
> > > I have removed the codec_id test.
> >
> > >  libavcodec/pcm.c     |    5 +++++
> > >  libavformat/vocdec.c |   18 +++++++++++++++---
> > >  2 files changed, 20 insertions(+), 3 deletions(-)
> > > c93b9039d5fd6aa892aa6b0fc2e876a1edd250a8  pcm_sanity_check.diff
> > > From 3d4fc3a6e0740cb4bcea52515e76f7e7e425d9cd Mon Sep 17 00:00:00 2001
> > > From: Daniel Kang <daniel.d.kang at gmail.com>
> > > Date: Thu, 6 Jan 2011 21:03:27 -0500
> > > Subject: [PATCH] Add check for pcm files.
> > >
> > > ---
> > >  libavcodec/pcm.c     |    5 +++++
> > >  libavformat/vocdec.c |   18 +++++++++++++++---
> > >  2 files changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/pcm.c b/libavcodec/pcm.c
> > > index b6b49dc..533e834 100644
> > > --- a/libavcodec/pcm.c
> > > +++ b/libavcodec/pcm.c
> > > @@ -292,6 +292,11 @@ static int pcm_decode_frame(AVCodecContext *avctx,
> > >          /* we process 40-bit blocks per channel for LXF */
> > >          sample_size = 5;
> > >
> > > +    if (sample_size == 0) {
> > > +        av_log(avctx, AV_LOG_ERROR, "Invalid sample_size\n");
> > > +        return AVERROR(EINVAL);
> > > +    }
> > > +
> > >      n = avctx->channels * sample_size;
> > >
> > >      if(n && buf_size % n){
> > > diff --git a/libavformat/vocdec.c b/libavformat/vocdec.c
> > > index 909520c..835417e 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_codec;
> > >      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_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 (dec->codec_id != tmp_codec)
> > > +                av_log(s, AV_LOG_WARNING, "Ignoring mid-stream change in
> > audio codec\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_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 (dec->codec_id != tmp_codec)
> > > +                av_log(s, AV_LOG_WARNING, "Ignoring mid-stream change in
> > audio codec\n");
> > >              url_fskip(pb, 4);
> > >              voc->remaining_size -= 12;
> > >              max_size -= 12;
> >
> >
> > > @@ -125,6 +133,10 @@ voc_get_packet(AVFormatContext *s, AVPacket *pkt,
> > AVStream *st, int max_size)
> > >              voc->remaining_size = 0;
> > >              break;
> > >          }
> > > +        if (dec->codec_id == CODEC_ID_NONE) {
> > > +            av_log(s, AV_LOG_ERROR, "Invalid codec_id\n");
> > > +            return AVERROR(EINVAL);
> > > +        }
> > >      }
> >
> > Its possible to override the codec id see AVFormatContext.audio_codec_id
> > so i think dec->codec_id == CODEC_ID_NONE is not neccesary fatal though
> > surely
> > not normal either, that is the av_log() is ok but i think we should
> > continue
> > (at least if AVFormatContext.audio_codec_id is set)
> 
> 
> That check will only be triggered if dec->codec_id == CODEC_ID_NONE and
> then the codec_id that is read in is invalid. In this case, I do not
> think AVFormatContext.audio_codec_id could be set to anything valid.
> I have updated the patch so that the error returns only if
> s->audio_codec_id == CODEC_ID_NONE.

>  libavcodec/pcm.c     |    5 +++++
>  libavformat/vocdec.c |   18 +++++++++++++++---
>  2 files changed, 20 insertions(+), 3 deletions(-)
> ae83bafc08d82d28b96d75010dff678785c7b6a6  pcm_sanity_check.diff
> From 583e4955f3f87ff10bc88eab3a2db76156f9ed7c Mon Sep 17 00:00:00 2001
> From: Daniel Kang <daniel.d.kang at gmail.com>
> Date: Thu, 6 Jan 2011 21:03:27 -0500
> Subject: [PATCH] Add check for pcm files.

lgtm if tested


[...]
--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20110111/642bfbe3/attachment.pgp>



More information about the ffmpeg-devel mailing list