[FFmpeg-devel] [PATCH] Vorbis-in-Ogg: Do not set timebase to invalid values

Michael Niedermayer michaelni
Sat Jan 29 16:39:50 CET 2011


On Sat, Jan 29, 2011 at 06:05:11AM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> On Sat, Jan 29, 2011 at 5:52 AM, Reimar D?ffinger
> <Reimar.Doeffinger at gmx.de> wrote:
> > On Sat, Jan 29, 2011 at 01:46:31AM -0800, Jason Garrett-Glaser wrote:
> >> On Fri, Jan 28, 2011 at 10:22 PM, Reimar D?ffinger
> >> <Reimar.Doeffinger at gmx.de> wrote:
> >> > On 29 Jan 2011, at 05:42, M?ns Rullg?rd <mans at mansr.com> wrote:
> >> >> Janne Grunau <janne-ffmpeg at jannau.net> writes:
> >> >>
> >> >>> From: Reimar D?ffinger <Reimar.Doeffinger at gmx.de>
> >> >>>
> >> >>> cherry picked from git.videolan.org repo
> >> >>>
> >> >>> Janne
> >> >>> ---8<---
> >> >>> Avoids an assert when the sample rate is invalid and the timebase
> >> >>> is thus set to e.g. 1/0.
> >> >>> Sample file is http://samples.mplayerhq.hu/ogg/fuzzed-srate-crash.ogg
> >> >>> ---
> >> >>> libavformat/oggparsevorbis.c | ? 10 +++++++---
> >> >>> 1 files changed, 7 insertions(+), 3 deletions(-)
> >> >>>
> >> >>> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> >> >>> index cdb0266..d743d25 100644
> >> >>> --- a/libavformat/oggparsevorbis.c
> >> >>> +++ b/libavformat/oggparsevorbis.c
> >> >>> @@ -221,6 +221,7 @@ vorbis_header (AVFormatContext * s, int idx)
> >> >>> ? ? if (os->buf[os->pstart] == 1) {
> >> >>> ? ? ? ? const uint8_t *p = os->buf + os->pstart + 7; /* skip "\001vorbis" tag */
> >> >>> ? ? ? ? unsigned blocksize, bs0, bs1;
> >> >>> + ? ? ? ?int srate;
> >> >>>
> >> >>> ? ? ? ? if (os->psize != 30)
> >> >>> ? ? ? ? ? ? return -1;
> >> >>> @@ -229,7 +230,7 @@ vorbis_header (AVFormatContext * s, int idx)
> >> >>> ? ? ? ? ? ? return -1;
> >> >>>
> >> >>> ? ? ? ? st->codec->channels = bytestream_get_byte(&p);
> >> >>> - ? ? ? ?st->codec->sample_rate = bytestream_get_le32(&p);
> >> >>> + ? ? ? ?srate = bytestream_get_le32(&p);
> >> >>> ? ? ? ? p += 4; // skip maximum bitrate
> >> >>> ? ? ? ? st->codec->bit_rate = bytestream_get_le32(&p); // nominal bitrate
> >> >>> ? ? ? ? p += 4; // skip minimum bitrate
> >> >>> @@ -249,8 +250,11 @@ vorbis_header (AVFormatContext * s, int idx)
> >> >>> ? ? ? ? st->codec->codec_type = AVMEDIA_TYPE_AUDIO;
> >> >>> ? ? ? ? st->codec->codec_id = CODEC_ID_VORBIS;
> >> >>>
> >> >>> - ? ? ? ?st->time_base.num = 1;
> >> >>> - ? ? ? ?st->time_base.den = st->codec->sample_rate;
> >> >>> + ? ? ? ?if (srate > 0) {
> >> >>> + ? ? ? ? ? ?st->codec->sample_rate = srate;
> >> >>> + ? ? ? ? ? ?st->time_base.num = 1;
> >> >>> + ? ? ? ? ? ?st->time_base.den = srate;
> >> >>> + ? ? ? ?}
> >> >>> ? ? } else if (os->buf[os->pstart] == 3) {
> >> >>> ? ? ? ? if (os->psize > 8)
> >> >>> ? ? ? ? ? ? ff_vorbis_comment (s, &st->metadata, os->buf + os->pstart + 7, os->psize - 8);
> >> >>> --
> >> >>> 1.7.4.rc2
> >> >>
> >> >> I still want to know why common code doesn't catch this. ?Replicating
> >> >> this check in each and every demuxer is insane.
> >> >
> >> > I already explained it and nobody cared.
> >> > I also gave a reason why, even if the common code would catch it this is of advantage.
> >> > In addition relying on common code is risky, streams can be added at any time, demuxers might parse headers even if they are in the middle of the stream, unless data is validated immediately (i.e. in the demuxer) there is a significant risk that it will end up outside FFmpeg, in the application.
> >> > However common code could set the timebase properly/better if the decoder figures out the sample rate, but I don't feel like investigating where to do this.
> >>
> >> Maybe it could be done in the wrapper functions, e.g. the utility
> >> functions, not per-demuxer? ?So it's still done in libav*, but not in
> >> each codec repeatedly.
> >
> > I was going to say that there is no place to do this that does not include
> > a lot of issues, but there actually is one:
> > av_set_pts_info
> > With the little issue that this code incorrectly bypasses that function.
> > A lot of code does that, so a first patch is to make everyone actually
> > use that.
> > I probably missed an few.
> > And WTF is up with everyone setting both st->codec->time_base and
> > st->time_base? This is just insanity!
> 
> If they are the same, let's remove one of them (probably st->timebase).

coup     project: success
maintain code   : fail

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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- 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/20110129/5b0f8652/attachment.pgp>



More information about the ffmpeg-devel mailing list