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

[Måns Rullgård]

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.

I'll leave an implementation to our resident quicktime experts.

-- 
M?ns Rullg?rd
mans at mansr.com