[FFmpeg-devel] [PATCH] matroska: Ensure sample rate is always >= 1.

[Michael Niedermayer]

On Thu, Apr 19, 2012 at 02:22:28PM -0700, Dale Curtis wrote:
> On Thu, Apr 19, 2012 at 1:31 PM, Michael Niedermayer <michaelni at gmx.at>wrote:
> 
> > On Mon, Apr 16, 2012 at 06:05:27PM -0700, dalecurtis at chromium.org wrote:
> > > From: Dale Curtis <dalecurtis at chromium.org>
> > >
> > > May no longer be necessary, but seems like a valid
> > > enforcement.
> > >
> > > Signed-off-by: Dale Curtis <dalecurtis at chromium.org>
> > > ---
> > >  libavformat/matroskadec.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> > > index a484c50..cdaeff7 100644
> > > --- a/libavformat/matroskadec.c
> > > +++ b/libavformat/matroskadec.c
> > > @@ -1680,7 +1680,7 @@ static int matroska_read_header(AVFormatContext *s)
> > >              }
> > >          } else if (track->type == MATROSKA_TRACK_TYPE_AUDIO) {
> > >              st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
> > > -            st->codec->sample_rate = track->audio.out_samplerate;
> > > +            st->codec->sample_rate = FFMAX(1,
> > track->audio.out_samplerate);
> >
> > What does or did this fix ?
> >
> >
> The change was made after an audit by our security team at the dawn of
> Chrome a couple years ago. The test cases associated with that audit don't
> reproduce any visible issues anymore:
> 
> http://commondatastorage.googleapis.com/dalecurtis-shared/old-test-cases.tar.bz2
> 
> 
> Before submitting, I reviewed the current code and didn't see anything
> questionable, most consumers of the sample_rate are guarded by a <= check.
> However, instead of just tossing the patch I figured I'd see if anyone on
> here had some thoughts first.

well my thoughts would be:
theres not much code in matroskadec that reads the sample rate
so a hypothetical issue should be outside matroskadec and for these
any fix should probably also be outside matroskadec.
If <= 0 values really posed a problem that could be handled in generic
code and not be dependant on every demuxer having such a check.

also turning a invalid value silently to a valid one that is with
near certainity not correct feels non ideal

In the end the patch looks unneeded to me but i cannot say with
certainity that it is unneeded and theres no corner case in no
piece of code that might be better of without negative or even or
prime numbers ...


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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20120420/ceedca10/attachment.asc>