[FFmpeg-devel] Security issues?
Reimar Döffinger
Reimar.Doeffinger
Wed Sep 23 12:31:51 CEST 2009
On Tue, Sep 22, 2009 at 08:09:08PM +0200, Michael Niedermayer wrote:
> Hi
>
> lars has mailed me the following 2 links
> http://www.heise.de/newsticker/Sicherheitsluecken-in-VLC-und-FFmpeg--/meldung/145655
> http://secunia.com/advisories/36805/
Ok, here is the stuff from
http://src.chromium.org/viewvc/chrome/trunk/deps/third_party/ffmpeg/patches/to_upstream/?pathrev=26428
Could someone find a contact from chromium and forward this to them?
Because I already commented on some of these things, and I don't want to
end up reviewing everything n times.
09_mov_stsz_int_oflow.patch:
--- libavformat/mov.c.orig 2009-08-19 20:37:50.000000000 -0700
+++ libavformat/mov.c 2009-08-19 20:46:37.000000000 -0700
@@ -1169,7 +1169,12 @@
if (!sc->sample_sizes)
return AVERROR(ENOMEM);
- num_bytes = (entries*field_size+4)>>3;
+ num_bytes = entries*field_size;
+ if (num_bytes / field_size != entries || num_bytes + 4 < num_bytes) {
+ av_freep(&sc->sample_sizes);
+ return -1;
+ }
+ num_bytes = (num_bytes+4)>>3;
buf = av_malloc(num_bytes+FF_INPUT_BUFFER_PADDING_SIZE);
if (!buf) {
Fixed IMO in a better way in r19793
=======================================================
10_vorbis_submap_indexes.patch:
--- libavcodec/vorbis_dec.c.orig 2009-08-20 16:47:16.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-20 19:33:13.000000000 -0700
@@ -723,9 +723,20 @@
}
for(j=0;j<mapping_setup->submaps;++j) {
+ int bits;
skip_bits(gb, 8); // FIXME check?
- mapping_setup->submap_floor[j]=get_bits(gb, 8);
- mapping_setup->submap_residue[j]=get_bits(gb, 8);
+ bits=get_bits(gb, 8);
+ if (bits>=vc->floor_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "submap floor value %d out of range. \n", bits);
+ return 1;
+ }
+ mapping_setup->submap_floor[j]=bits;
+ bits=get_bits(gb, 8);
+ if (bits>=vc->residue_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "submap residue value %d out of range. \n", bits);
+ return 1;
+ }
+ mapping_setup->submap_residue[j]=bits;
AV_DEBUG(" %d mapping %d submap : floor %d, residue %d \n", i, j, mapping_setup->submap_floor[j], mapping_setup->submap_residue[j]);
}
Should either just use FFMIN and print no message at all, or check both
after reading with a single if to keep code bloat low.
FFMIN can not fix the floor_count == 0 or residue_count == 0 case,
though if they are not valid that should be caught when reading them.
I think each of these patches should add a comment in the struct
declarations that specifies the valid values for each field.
==========================================
11_vorbis_residue_book_index:
--- libavcodec/vorbis_dec.c.orig3 2009-08-21 08:21:21.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-21 08:42:17.000000000 -0700
@@ -636,6 +636,10 @@
res_setup->partition_size=get_bits(gb, 24)+1;
res_setup->classifications=get_bits(gb, 6)+1;
res_setup->classbook=get_bits(gb, 8);
+ if (res_setup->classbook>=vc->codebook_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "classbook value %d out of range. \n", res_setup->classbook);
+ return 1;
+ }
AV_DEBUG(" begin %d end %d part.size %d classif.s %d classbook %d \n", res_setup->begin, res_setup->end, res_setup->partition_size,
res_setup->classifications, res_setup->classbook);
Ok IMO, though FFMIN instead of return 1 might be better, except it
would not work for codebook_count == 0. Someone who
knows the code should decide if the output would be useful for
debugging, otherwise just using FFMIN without any av_log would be
better.
====================================================
12_vorbis_mode_indexes:
--- libavcodec/vorbis_dec.c.orig4 2009-08-21 12:37:02.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-21 13:09:27.000000000 -0700
@@ -803,7 +803,11 @@
mode_setup->blockflag=get_bits1(gb);
mode_setup->windowtype=get_bits(gb, 16); //FIXME check
mode_setup->transformtype=get_bits(gb, 16); //FIXME check
- mode_setup->mapping=get_bits(gb, 8); //FIXME check
+ mode_setup->mapping=get_bits(gb, 8);
+ if (mode_setup->mapping>=vc->mapping_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "mode mapping value %d out of range. \n", mode_setup->mapping);
+ return 1;
+ }
AV_DEBUG(" %d mode: blockflag %d, windowtype %d, transformtype %d, mapping %d \n", i, mode_setup->blockflag, mode_setup->windowtype, mode_setup->transformtype, mode_setup->mapping);
}
@@ -1466,6 +1470,10 @@
} else {
mode_number=get_bits(gb, ilog(vc->mode_count-1));
}
+ if (mode_number>=vc->mode_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "mode number %d out of range.\n", mode_number);
+ return -1;
+ }
vc->mode_number=mode_number;
mapping=&vc->mappings[vc->modes[mode_number].mapping];
Same comment, except that it needs careful checking that the return
1/return -1 really and completely aborts decoding. Or just use FFMIN in
addition to make sure the value stored in the context is valid.
Note that wherever the fuzzed sample file can be played when just using
FFMIN instead of return ..., that would be preferable - does not work
for mapping_count == 0 or mode_count == 0 though.
Keeping the error messages (though they should be warning instead of
error then) is good except when it bloats the code too much.
Note that it might be a good idea to just verify all values in one place
after all parsing, something like vorbis_check_context after
vorbis_parse_setup_hdr (as far as that can fix the issues, maybe some of
those read values are already used within vorbis_parse_setup_hdr).
================================================
13_ogg_comment_parsing
--- libavformat/oggparsevorbis.c.orig 2009-08-20 20:23:55.000000000 -0700
+++ libavformat/oggparsevorbis.c 2009-08-24 21:56:22.000000000 -0700
@@ -47,9 +47,12 @@
p += s;
+ if (end - p < 4)
+ return -1;
+
n = bytestream_get_le32(&p);
- while (p < end && n > 0) {
+ while ((end - p) >= 4 && n > 0) {
const char *t, *v;
int tl, vl;
@@ -97,7 +100,7 @@
}
if (p != end)
- av_log(as, AV_LOG_INFO, "%ti bytes of comment header remain\n", p-end);
+ av_log(as, AV_LOG_INFO, "%ti bytes of comment header remain\n", end-p);
if (n > 0)
av_log(as, AV_LOG_INFO,
"truncated comment header, %i comments not found\n", n);
The second hunk is unrelated and has been in my local tree for ages, so
I just applied it.
Part of what caused the issue here is also that s is unsigned.
So I'd propose this instead:
Index: libavformat/oggparsevorbis.c
===================================================================
--- libavformat/oggparsevorbis.c (revision 19978)
+++ libavformat/oggparsevorbis.c (working copy)
@@ -50,27 +50,28 @@
{
const uint8_t *p = buf;
const uint8_t *end = buf + size;
- unsigned s, n, j;
+ unsigned n, j;
+ int s;
if (size < 8) /* must have vendor_length and user_comment_list_length */
return -1;
s = bytestream_get_le32(&p);
- if (end - p < s)
+ if (end - p - 4 < s || s < 0)
return -1;
p += s;
n = bytestream_get_le32(&p);
- while (p < end && n > 0) {
+ while (end - p >= 4 && n > 0) {
const char *t, *v;
int tl, vl;
s = bytestream_get_le32(&p);
- if (end - p < s)
+ if (end - p < s || s < 0)
break;
t = p;
Since p is padded, making s signed and checking for s < 0 would be
enough.
=========================================
14_floor_masterbook_index:
--- libavcodec/vorbis_dec.c.orig6 2009-08-24 19:25:26.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-24 23:49:25.000000000 -0700
@@ -492,13 +492,23 @@
AV_DEBUG(" %d floor %d class dim: %d subclasses %d \n", i, j, floor_setup->data.t1.class_dimensions[j], floor_setup->data.t1.class_subclasses[j]);
if (floor_setup->data.t1.class_subclasses[j]) {
- floor_setup->data.t1.class_masterbook[j]=get_bits(gb, 8);
+ int bits=get_bits(gb, 8);
+ if (bits>=vc->codebook_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "Masterbook index %d is out of range.\n", bits);
+ return 1;
+ }
+ floor_setup->data.t1.class_masterbook[j]=bits;
AV_DEBUG(" masterbook: %d \n", floor_setup->data.t1.class_masterbook[j]);
}
for(k=0;k<(1<<floor_setup->data.t1.class_subclasses[j]);++k) {
- floor_setup->data.t1.subclass_books[j][k]=(int16_t)get_bits(gb, 8)-1;
+ int16_t bits=get_bits(gb, 8)-1;
+ if (bits!=-1 && bits>=vc->codebook_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "Subclass book index %d is out of range.\n", bits);
+ return 1;
+ }
+ floor_setup->data.t1.subclass_books[j][k]=bits;
AV_DEBUG(" book %d. : %d \n", k, floor_setup->data.t1.subclass_books[j][k]);
}
Same comments as with all other vorbis_dec changes, though I also notice
that there might be a bit of an issue if codebook_count is 0, in which
case my suggestion of just using FFMIN might not work...
Maybe one more argument for validating everything as much as possible in
one place, that might make it possible to fix things up properly instead
of vorbis files becoming unplayable just because of a single bit error
anywhere in the header.
==============================================
15_more_residue_book_indexes:
--- libavcodec/vorbis_dec.c.orig7 2009-08-25 07:59:12.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-25 08:27:02.000000000 -0700
@@ -669,7 +669,12 @@
for(j=0;j<res_setup->classifications;++j) {
for(k=0;k<8;++k) {
if (cascade[j]&(1<<k)) {
- res_setup->books[j][k]=get_bits(gb, 8);
+ int bits=get_bits(gb, 8);
+ if (bits>=vc->codebook_count) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "book value %d out of range. \n", bits);
+ return 1;
+ }
+ res_setup->books[j][k]=bits;
AV_DEBUG(" %d class casscade depth %d book: %d \n", j, k, res_setup->books[j][k]);
Well, more of the same...
==========================================
18_fix_theora_header_bit_len:
--- libavcodec/vp3.c.orig2 2009-08-21 13:39:25.000000000 -0700
+++ libavcodec/vp3.c 2009-08-27 16:58:06.000000000 -0700
@@ -2307,7 +2307,7 @@
}
for(i=0;i<3;i++) {
- init_get_bits(&gb, header_start[i], header_len[i]);
+ init_get_bits(&gb, header_start[i], header_len[i] * 8);
ptype = get_bits(&gb, 8);
Seems obvious, IMO apply, though << 3 instead of *8 might be more
consistent (not actually sure about it).
========================================
22_fix_theora_frag_fencepost:
--- libavcodec/vp3.c.orig4 2009-08-27 21:00:20.000000000 -0700
+++ libavcodec/vp3.c 2009-08-27 21:08:41.000000000 -0700
@@ -993,7 +993,7 @@
num_blocks_at_qpi += run_length;
for (j = 0; j < run_length; i++) {
- if (i > s->coded_fragment_list_index)
+ if (i >= s->coded_fragment_list_index)
return -1;
if (s->all_fragments[s->coded_fragment_list[i]].qpi == qpi) {
Seems right, though I have a suspicion that unpack_superblocks also
lacks a check for coded_fragment_list_index < fragment_count is missing,
also the code for the MODE_INTER_NO_MV case is duplicated in that
function...
===================================
23_vorbis_sane_partition:
--- libavcodec/vorbis_dec.c.orig 2009-09-03 20:36:13 -0700
+++ libavcodec/vorbis_dec.c 2009-09-03 20:37:04 -0700
@@ -37,6 +37,7 @@
#define V_NB_BITS 8
#define V_NB_BITS2 11
#define V_MAX_VLCS (1<<16)
+#define V_MAX_PARTITIONS (1<<20)
#ifndef V_DEBUG
#define AV_DEBUG(...)
@@ -644,6 +645,14 @@
res_setup->begin=get_bits(gb, 24);
res_setup->end=get_bits(gb, 24);
res_setup->partition_size=get_bits(gb, 24)+1;
+ /* Validations to prevent a buffer overflow later. */
+ if (res_setup->begin>res_setup->end
+ || res_setup->end>vc->blocksize[1]/(res_setup->type==2?1:2)
+ || (res_setup->end-res_setup->begin)/res_setup->partition_size>V_MAX_PARTITIONS) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "partition out of bounds: type, begin, end, size, blocksize: %d, %d, %d, %d, %d\n", res_setup->type, res_setup->begin, res_setup->end, res_setup->partition_size, vc->blocksize[1]/2);
+ return 1;
+ }
+
res_setup->classifications=get_bits(gb, 6)+1;
res_setup->classbook=get_bits(gb, 8);
if (res_setup->classbook>=vc->codebook_count) {
Oh god, more of the same vorbis stuff, just more ugly. This one
definitely needs to be documented much better, e.g. mentioning
the places where it would crash otherwise and also documenting the
assumptions on begin and end where they are declared.
======================================================
24_vorbis_zero_channels:
--- libavcodec/vorbis_dec.c.orig12 2009-08-28 07:52:50.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-28 08:11:24.000000000 -0700
@@ -896,7 +896,11 @@
}
vc->version=get_bits_long(gb, 32); //FIXME check 0
- vc->audio_channels=get_bits(gb, 8); //FIXME check >0
+ vc->audio_channels=get_bits(gb, 8);
+ if (vc->audio_channels==0) {
+ av_log(vc->avccontext, AV_LOG_ERROR, " Vorbis id header packet corrupt (zero channels). \n");
+ return 1;
+ }
vc->audio_samplerate=get_bits_long(gb, 32); //FIXME check >0
vc->bitrate_maximum=get_bits_long(gb, 32);
vc->bitrate_nominal=get_bits_long(gb, 32);
Applied in r19975, except using -1 as return value which is a bit
inconsistent with the remaining code.
=====================================================
25_vorbis_floor0_index:
--- libavcodec/vorbis_dec.c.orig13 2009-08-28 08:43:23.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-28 08:38:27.000000000 -0700
@@ -569,12 +569,11 @@
uint_fast8_t book_idx;
for (idx=0;idx<floor_setup->data.t0.num_books;++idx) {
book_idx=get_bits(gb, 8);
+ if (book_idx>=vc->codebook_count)
+ return 1;
floor_setup->data.t0.book_list[idx]=book_idx;
if (vc->codebooks[book_idx].dimensions > max_codebook_dim)
max_codebook_dim=vc->codebooks[book_idx].dimensions;
-
- if (floor_setup->data.t0.book_list[idx]>vc->codebook_count)
- return 1;
}
}
Wow, double stupid, validating data after using it and doing the
validation off by one. Obviously ok IMO.
================================================
26_vorbis_mag_angle_index:
--- libavcodec/vorbis_dec.c.orig14 2009-08-28 11:07:19.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-28 11:28:40.000000000 -0700
@@ -729,7 +729,14 @@
for(j=0;j<mapping_setup->coupling_steps;++j) {
mapping_setup->magnitude[j]=get_bits(gb, ilog(vc->audio_channels-1));
mapping_setup->angle[j]=get_bits(gb, ilog(vc->audio_channels-1));
- // FIXME: sanity checks
+ if (mapping_setup->magnitude[j]>=vc->audio_channels) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "magnitude channel %d out of range. \n", mapping_setup->magnitude[j]);
+ return 1;
+ }
+ if (mapping_setup->angle[j]>=vc->audio_channels) {
+ av_log(vc->avccontext, AV_LOG_ERROR, "angle channel %d out of range. \n", mapping_setup->angle[j]);
+ return 1;
+ }
}
} else {
mapping_setup->coupling_steps=0;
IMO only use one if/message for both.
======================================================
27_vorbis_residue_loop_error:
--- libavcodec/vorbis_dec.c.orig15 2009-08-28 11:30:39.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-08-28 14:06:20.000000000 -0700
@@ -1554,7 +1554,7 @@
uint_fast8_t ch=0;
for(j=0;j<vc->audio_channels;++j) {
- if ((mapping->submaps==1) || (i=mapping->mux[j])) {
+ if ((mapping->submaps==1) || (i==mapping->mux[j])) {
res_chan[j]=res_num;
if (no_residue[j]) {
do_not_decode[ch]=1;
Was discussed elsewhere. _Seems_ obviously right, but one of the
comments was if the mapping->submaps==1 check is really supposed to be
there.
==================================================
28_theora_malloc_checks:
--- libavcodec/vp3.c.orig5 2009-08-31 10:45:06.000000000 -0700
+++ libavcodec/vp3.c 2009-08-31 12:09:55.000000000 -0700
@@ -43,6 +43,8 @@
#define FRAGMENT_PIXELS 8
+static av_cold int vp3_decode_end(AVCodecContext *avctx);
+
typedef struct Coeff {
struct Coeff *next;
DCTELEM coeff;
@@ -1740,6 +1742,11 @@
s->coeffs = av_malloc(s->fragment_count * sizeof(Coeff) * 65);
s->coded_fragment_list = av_malloc(s->fragment_count * sizeof(int));
s->pixel_addresses_initialized = 0;
+ if (!s->all_fragments || !s->coeff_counts || !s->coeffs ||
+ !s->coded_fragment_list) {
+ vp3_decode_end(avctx);
+ return -1;
+ }
if (!s->theora_tables)
{
@@ -1845,6 +1852,11 @@
s->superblock_macroblocks = av_malloc(s->superblock_count * 4 * sizeof(int));
s->macroblock_fragments = av_malloc(s->macroblock_count * 6 * sizeof(int));
s->macroblock_coding = av_malloc(s->macroblock_count + 1);
+ if (!s->superblock_fragments || !s->superblock_macroblocks ||
+ !s->macroblock_fragments || !s->macroblock_coding) {
+ vp3_decode_end(avctx);
+ return -1;
+ }
init_block_mapping(s);
for (i = 0; i < 3; i++) {
Seems ok, but the vlc_fail path should probably call vp3_decode_end,
too. And it should be AVERROR(ENOMEM), not -1.
===================================================
29_mov_dref_looping:
--- libavformat/mov.c.orig2 2009-08-31 15:41:36.000000000 -0700
+++ libavformat/mov.c 2009-08-31 15:55:36.000000000 -0700
@@ -261,6 +261,8 @@
MOVDref *dref = &sc->drefs[i];
uint32_t size = get_be32(pb);
int64_t next = url_ftell(pb) + size - 4;
+ if (size < 8)
+ return -1;
dref->type = get_le32(pb);
get_be32(pb); // version + flags
I do not like failing hard, using FFMAX to make sure size is at least
e.g. 4 would be better IMO (no idea what the minimum size actually is).
=======================================================
31_mp3_outlen:
--- libavcodec/mpegaudiodec.c.orig 2009-08-31 23:19:15.000000000 -0700
+++ libavcodec/mpegaudiodec.c 2009-08-31 23:20:05.000000000 -0700
@@ -2294,8 +2294,10 @@
*data_size = out_size;
avctx->sample_rate = s->sample_rate;
//FIXME maybe move the other codec info stuff from above here too
- }else
+ }else{
av_log(avctx, AV_LOG_DEBUG, "Error while decoding MPEG audio frame.\n"); //FIXME return -1 / but also return the number of bytes consumed
+ *data_size = 0;
+ }
s->frame_size = 0;
return buf_size + skipped;
}
*data_size = 0 IMO should be done right at the top of the decode_frame
function - assuming the case decode failed but not returning -1 is
indeed as intended.
========================================================
32_mov_stream_index:
--- libavformat/mov.c.orig3 2009-08-31 23:43:53.000000000 -0700
+++ libavformat/mov.c 2009-09-01 08:32:49.000000000 -0700
@@ -244,10 +244,15 @@
static int mov_read_dref(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
int entries, i, j;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
get_be32(pb); // version + flags
entries = get_be32(pb);
if (entries >= UINT_MAX / sizeof(*sc->drefs))
@@ -392,9 +397,13 @@
static int mov_read_esds(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
+ AVStream *st;
int tag, len;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
get_be32(pb); /* version + flags */
len = mp4_read_descr(c, pb, &tag);
if (tag == MP4ESDescrTag) {
@@ -451,7 +460,10 @@
{
const int num = get_be32(pb);
const int den = get_be32(pb);
- AVStream * const st = c->fc->streams[c->fc->nb_streams-1];
+ AVStream *st;
+ if (c->fc->nb_streams < 1)
+ return 0;
+ st = c->fc->streams[c->fc->nb_streams-1];
if (den != 0) {
if ((st->sample_aspect_ratio.den != 1 || st->sample_aspect_ratio.num) && // default
(den != st->sample_aspect_ratio.den || num != st->sample_aspect_ratio.num))
@@ -505,12 +517,18 @@
static int mov_read_mdhd(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
int version = get_byte(pb);
char language[4] = {0};
unsigned lang;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
if (version > 1)
return -1; /* unsupported */
@@ -572,7 +590,12 @@
static int mov_read_smi(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
+ AVStream *st;
+
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
if((uint64_t)atom.size > (1<<30))
return -1;
@@ -592,9 +615,13 @@
static int mov_read_enda(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
+ AVStream *st;
int little_endian = get_be16(pb);
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
dprintf(c->fc, "enda %d\n", little_endian);
if (little_endian == 1) {
switch (st->codec->codec_id) {
@@ -644,7 +671,12 @@
static int mov_read_wave(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
+ AVStream *st;
+
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
if((uint64_t)atom.size > (1<<30))
return -1;
@@ -671,7 +703,12 @@
*/
static int mov_read_glbl(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
+ AVStream *st;
+
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
if((uint64_t)atom.size > (1<<30))
return -1;
@@ -687,10 +724,16 @@
static int mov_read_stco(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
unsigned int i, entries;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
@@ -753,10 +796,16 @@
static int mov_read_stsd(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
int j, entries, pseudo_stream_id;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
@@ -1078,10 +1127,16 @@
static int mov_read_stsc(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
unsigned int i, entries;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
@@ -1106,10 +1161,16 @@
static int mov_read_stss(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
unsigned int i, entries;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
@@ -1133,12 +1194,18 @@
static int mov_read_stsz(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
unsigned int i, entries, sample_size, field_size, num_bytes;
GetBitContext gb;
unsigned char* buf;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
@@ -1201,12 +1268,18 @@
static int mov_read_stts(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
unsigned int i, entries;
int64_t duration=0;
int64_t total_sample_count=0;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
entries = get_be32(pb);
@@ -1245,10 +1318,16 @@
static int mov_read_ctts(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
unsigned int i, entries;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
entries = get_be32(pb);
@@ -1509,10 +1588,16 @@
int height;
int64_t disp_transform[2];
int display_matrix[3][2];
- AVStream *st = c->fc->streams[c->fc->nb_streams-1];
- MOVStreamContext *sc = st->priv_data;
+ AVStream *st;
+ MOVStreamContext *sc;
int version = get_byte(pb);
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ st = c->fc->streams[c->fc->nb_streams-1];
+ sc = st->priv_data;
+
get_be24(pb); /* flags */
/*
MOV_TRACK_ENABLED 0x0001
@@ -1787,9 +1872,14 @@
/* edit list atom */
static int mov_read_elst(MOVContext *c, ByteIOContext *pb, MOVAtom atom)
{
- MOVStreamContext *sc = c->fc->streams[c->fc->nb_streams-1]->priv_data;
+ MOVStreamContext *sc;
int i, edit_count;
+ if (c->fc->nb_streams < 1)
+ return 0;
+
+ sc = c->fc->streams[c->fc->nb_streams-1]->priv_data;
+
get_byte(pb); /* version */
get_be24(pb); /* flags */
edit_count = get_be32(pb); /* entries */
Seems not acceptable to me. I fear that the mov demuxer in quite a few
places assumes a specific ordering of the atoms without having a good
reason a leading to catastrophic failure, but this is just too much
code.
If there is no better idea (e.g. extending the atom parsing table to put
constraints on the atom order) I'd propose to have the mov demuxer just
create one dummy stream at the very beginning that will either be
replaced by the first real stream or removed at the end of parsing if no
stream was found.
I admit I can't promise this will get less ugly, but I suspect so.
==================================================
34_h264_bad_timings:
--- libavcodec/h264.c.orig2 2009-09-01 12:48:10.000000000 -0700
+++ libavcodec/h264.c 2009-09-01 12:53:14.000000000 -0700
@@ -7503,7 +7503,11 @@
sps->timing_info_present_flag = get_bits1(&s->gb);
if(sps->timing_info_present_flag){
sps->num_units_in_tick = get_bits_long(&s->gb, 32);
+ if (sps->num_units_in_tick <= 0)
+ sps->num_units_in_tick = 1;
sps->time_scale = get_bits_long(&s->gb, 32);
+ if (sps->time_scale <= 0)
+ sps->time_scale = 1;
sps->fixed_frame_rate_flag = get_bits1(&s->gb);
}
I guess it's the best that can be done, except that the fallback
time_scale should be something sensible like e.g. 25, not 1.
====================================================
35_mov_bad_timings:
--- libavformat/mov.c.orig4 2009-09-01 11:50:59.000000000 -0700
+++ libavformat/mov.c 2009-09-01 15:13:48.000000000 -0700
@@ -1502,9 +1502,11 @@
return 0;
}
- if (!sc->time_rate)
+ if (sc->time_rate <= 0)
sc->time_rate = 1;
- if (!sc->time_scale)
+ if (c->time_scale <= 0)
+ c->time_scale = 1;
+ if (sc->time_scale <= 0)
sc->time_scale = c->time_scale;
av_set_pts_info(st, 64, sc->time_rate, sc->time_scale);
Looks ok to me.
=============================================
39_vorbis_zero_dims:
--- libavcodec/vorbis_dec.c.orig16 2009-09-01 22:31:46.000000000 -0700
+++ libavcodec/vorbis_dec.c 2009-09-01 22:34:19.000000000 -0700
@@ -250,8 +250,8 @@
}
codebook_setup->dimensions=get_bits(gb, 16);
- if (codebook_setup->dimensions>16) {
- av_log(vc->avccontext, AV_LOG_ERROR, " %"PRIdFAST16". Codebook's dimension is too large (%d). \n", cb, codebook_setup->dimensions);
+ if (codebook_setup->dimensions>16||codebook_setup->dimensions==0) {
+ av_log(vc->avccontext, AV_LOG_ERROR, " %"PRIdFAST16". Codebook's dimension is invalid (%d). \n", cb, codebook_setup->dimensions);
goto error;
}
entries=get_bits(gb, 24);
Cosmetically nicer:
> if (!codebook_setup->dimensions || codebook_setup->dimensions>16) {
Otherwise probably ok except that as with all these patches it really
needs to be documented properly (i.e. in the declaration).
===========================================
40_ogg_missing_header:
--- libavformat/oggdec.c.orig 2009-09-01 22:06:21.000000000 -0700
+++ libavformat/oggdec.c 2009-09-01 22:21:42.000000000 -0700
@@ -478,11 +478,16 @@
{
struct ogg *ogg = s->priv_data;
ogg->curidx = -1;
+ int i;
//linear headers seek from start
if (ogg_get_headers (s) < 0){
return -1;
}
+ for (i = 0; i < ogg->nstreams; ++i)
+ if (ogg->streams[i].codec && !ogg->streams[i].private)
+ return -1;
+
//linear granulepos seek from end
ogg_get_length (s);
Completely failing just because one stream is broken is not acceptable,
also the parsers do not necessarily have to have a private field.
There are several possible solutions, something like this (untested)
might work for example:
Index: oggdec.c
===================================================================
--- oggdec.c (revision 19976)
+++ oggdec.c (working copy)
@@ -478,11 +478,16 @@
{
struct ogg *ogg = s->priv_data;
ogg->curidx = -1;
+ int i;
//linear headers seek from start
if (ogg_get_headers (s) < 0){
return -1;
}
+ for (i = 0; i < ogg->nstreams; ++i)
+ if (ogg->streams[i].header < 0)
+ ogg->streams[i].codec = NULL;
+
//linear granulepos seek from end
ogg_get_length (s);
=========================================================
41_vorbis_zero_samplerate:
Applied slightly differently in r19975
=================================================
42_aac_zero_bands:
--- libavcodec/aac.c.orig2 2009-09-02 13:27:42.000000000 -0700
+++ libavcodec/aac.c 2009-09-02 14:46:13.000000000 -0700
@@ -641,9 +641,9 @@
while ((sect_len_incr = get_bits(gb, bits)) == (1 << bits)-1)
sect_len += sect_len_incr;
sect_len += sect_len_incr;
- if (sect_len > ics->max_sfb) {
+ if (sect_len > ics->max_sfb || sect_len == 0) {
av_log(ac->avccontext, AV_LOG_ERROR,
- "Number of bands (%d) exceeds limit (%d).\n",
+ "Number of bands (%d) is invalid, limit (%d).\n",
sect_len, ics->max_sfb);
return -1;
}
Going by the explanation I think this is maybe the wrong approach.
As I understand it, the issue is that an all-0 band_type_run_end will
lead to an endless loop in decode_scalefactors.
I think it would be more reliable to avoid this in decode_scalefactors.
===========================================
43_codec_type_mismatch:
--- libavcodec/utils.c.orig1 2009-09-03 12:27:19.000000000 -0700
+++ libavcodec/utils.c 2009-09-03 12:28:16.000000000 -0700
@@ -476,6 +476,7 @@
avctx->codec = codec;
avctx->codec_id = codec->id;
+ avctx->codec_type = codec->type;
avctx->frame_number = 0;
if (HAVE_THREADS && avctx->thread_count>1 && !avctx->thread_opaque) {
SVN errors out in this case now, while that is a good idea, this really
needs to be fixed in the mov (and wherever else this can happen)
demuxers.
========================================
44_respect_flac_configure:
diff -u libavcodec/Makefile libavcodec/Makefile
--- libavcodec/Makefile 2009-09-04 02:14:54.000000000 +0000
+++ libavcodec/Makefile 2009-09-03 20:14:09.000000000 +0000
@@ -368,7 +368,6 @@
OBJS-$(CONFIG_MOV_DEMUXER) += mpeg4audio.o mpegaudiodata.o
OBJS-$(CONFIG_MPEGTS_MUXER) += mpegvideo.o
OBJS-$(CONFIG_NUT_MUXER) += mpegaudiodata.o
-OBJS-$(CONFIG_OGG_DEMUXER) += flacdec.o flacdata.o flac.o
OBJS-$(CONFIG_OGG_MUXER) += xiph.o flacdec.o flacdata.o flac.o
OBJS-$(CONFIG_RTP_MUXER) += mpegvideo.o
diff -u libavformat/Makefile libavformat/Makefile
--- libavformat/Makefile 2009-09-04 02:14:54.000000000 +0000
+++ libavformat/Makefile 2009-09-04 02:42:02.000000000 +0000
@@ -127,12 +127,12 @@
OBJS-$(CONFIG_NUT_MUXER) += nutenc.o nut.o riff.o
OBJS-$(CONFIG_NUV_DEMUXER) += nuv.o riff.o
OBJS-$(CONFIG_OGG_DEMUXER) += oggdec.o \
- oggparseflac.o \
oggparseogm.o \
- oggparsespeex.o \
oggparsetheora.o \
oggparsevorbis.o \
riff.o
+OBJS-$(CONFIG_FLAC_DECODER) += oggparseflac.o
+OBJS-$(CONFIG_LIBSPEEX) += oggparsespeex.o
OBJS-$(CONFIG_OGG_MUXER) += oggenc.o
OBJS-$(CONFIG_OMA_DEMUXER) += oma.o raw.o
OBJS-$(CONFIG_PCM_ALAW_DEMUXER) += raw.o
diff -u libavformat/oggdec.c libavformat/oggdec.c
--- libavformat/oggdec.c 2009-09-04 02:14:54.000000000 +0000
+++ libavformat/oggdec.c 2009-09-04 03:02:16.000000000 +0000
@@ -38,11 +38,15 @@
#define DECODER_BUFFER_SIZE MAX_PAGE_SIZE
static const struct ogg_codec * const ogg_codecs[] = {
+#if CONFIG_LIBSPEEX
&ff_speex_codec,
+#endif
&ff_vorbis_codec,
&ff_theora_codec,
+#if CONFIG_FLAC_DECODER
&ff_flac_codec,
&ff_old_flac_codec,
+#endif
&ff_ogm_video_codec,
&ff_ogm_audio_codec,
&ff_ogm_text_codec,
This is not ok, disabling the FLAC decode may not disable FLAC-in-Ogg
demuxing. This is one of the unfortunate cases where libavformat is tied
more closely to libavcodec than it should be, but I consider it the
fault of Oggs extreme case of misdesign.
It would be nice if the ogg demuxer only enabled compiling in of
ff_flac_parse_streaminfo instead of the whole FLAC decoder though!
More information about the ffmpeg-devel
mailing list