[FFmpeg-devel] [PATCH 2/2] avformat/mpc8: fix hang with fuzzed file

[wm4]

On Tue, 3 Feb 2015 21:47:57 +0100
Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:

> On Tue, Feb 03, 2015 at 07:04:12PM +0100, wm4 wrote:
> > This can lead to an endless loop by seeking back a few bytes after each
> > attempted chunk read. Assuming negative sizes are always invalid, this
> > is easy to fix. Other code in this demuxer treats negative sizes as
> > invalid as well.
> > 
> > Fixes ticket #4262.
> > ---
> >  libavformat/mpc8.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/libavformat/mpc8.c b/libavformat/mpc8.c
> > index d6ca338..6524c7e 100644
> > --- a/libavformat/mpc8.c
> > +++ b/libavformat/mpc8.c
> > @@ -223,6 +223,10 @@ static int mpc8_read_header(AVFormatContext *s)
> >      while(!avio_feof(pb)){
> >          pos = avio_tell(pb);
> >          mpc8_get_chunk_header(pb, &tag, &size);
> > +        if (size < 0) {
> 
> Isn't the only way for this to become negative for a too
> large uint64_t to be assigned to a int64_t?
> I.e. undefined behaviour.
> In that case this isn't quite the right way in the strictest sense,
> though it is likely to work "normally".

This is what mpc8_get_chunk_header() does:

    pos = avio_tell(pb);
    *tag = avio_rl16(pb);
    *size = ffio_read_varlen(pb);
    *size -= avio_tell(pb) - pos;

ffio_read_varlen() returns uint64_t, so I don't think it's supposed to
read negative values. But the last line could make it negative anyway
if the chunk size field read here is inconsistent.