[FFmpeg-devel] Bug in libavformat/mov.c?

[Michael Niedermayer]

On Wed, Jul 09, 2008 at 01:24:45PM +0100, M?ns Rullg?rd wrote:
> 
> Ivan Zezyulya wrote:
> > Hi all,
> >
> > please anyone can tell me the meaning of the following lines in
> > libavformat/mov.c:1043 in function mov_read_stsz:
> >
> >   if(entries >= UINT_MAX / sizeof(int))
> >         return -1;
> >
> >
> > here is more context, this fucntion reads the 'stsz' atom in a quicktime
> > movie file:
> >
> > static int mov_read_stsz(MOVContext *c, ByteIOContext *pb, MOV_atom_t atom)
> > {
> >     AVStream *st = c->fc->streams[c->fc->nb_streams-1];
> >     MOVStreamContext *sc = st->priv_data;
> >     unsigned int i, entries, sample_size;
> >
> >     get_byte(pb); /* version */
> >     get_be24(pb); /* flags */
> >
> >     sample_size = get_be32(pb);
> >     if (!sc->sample_size) /* do not overwrite value computed in stsd */
> >         sc->sample_size = sample_size;
> >     entries = get_be32(pb);
> >     if(entries >= UINT_MAX / sizeof(int))
> 
> If this condition is false, ...
> 
> >         return -1;
> >
> >     sc->sample_count = entries;
> >     if (sample_size)
> >         return 0;
> >
> >     dprintf(c->fc, "sample_size = %d sample_count = %d\n",
> > sc->sample_size, sc->sample_count);
> >
> >     sc->sample_sizes = av_malloc(entries * sizeof(int));
> 
> ... then this multiplication will overflow, ...
> 
> >     if (!sc->sample_sizes)
> >         return -1;
> >     for(i=0; i<entries; i++)
> >         sc->sample_sizes[i] = get_be32(pb);
> 
> ... and this loop will overflow the allocated buffer.
> 
> >     return 0;
> > }
> >
> > The problem is that I have a .mov file, VERY large file (101GB), and it
> > has more than UINT_MAX / sizeof(int) entries in one of its sample size
> > tables. After reaching the above lines with check of "entries" variable,
> > ffmpeg recursively quits reading .mov file and reports "error reading
> > header" (in mov_read_header function in libavormat/mov.c:1743) and then
> > quits. The file itself is correct, the QuickTime player can successfully
> > play it.
> >
> > According to the QuickTime specification
> > (http://developer.apple.com/documentation/QuickTime/QTFF/qtff.pdf), the
> > "Number of entries" field is "A 32-bit integer containing the count of
> > entries in the sample size table.", without any upper limits.
> >
> > I've just commented out this check and after it ffmpeg was able to read
> > all the tracks and the whole .mov file and successfully process it
> > without errors. (Of course, it was not so fast, it took for me not an
> > one hour to identify this problem in deep debugging ;)
> > But this line of code looks a bit strange for me, and I'll be happy if
> > someone can tell me why it is needed so I can comment it out without
> > suspicion that I'm breaking something in ffmpeg.
> 
> I'm surprised that removing the check didn't cause a crash.
> 
> The proper way to handle such files is, IMHO, to load the sample sizes
> in smaller chunks as needed, based on the current playback position.
> Loading them all would require an insane amount of memory.  This could
> be done either by seeking and reading, or (if virtual address space
> allows), by mmap()ing that section of the file, and let the OS take
> care of the caching.

theres an additional complication with that, mov allows these headers to
be zlib compressed IIRC, that would likely complicate random access
considerably ...
and i suspect that files where these are gb sized likely would have them
compressed for obvious reasons.

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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080709/cf389f13/attachment.pgp>