[FFmpeg-devel] [PATCH 1/2 v2] avformat/dashdec: enable overriding of the maximum manifest size

[Jan Ekström]

On Tue, Sep 1, 2020 at 9:31 PM Andreas Rheinhardt
<andreas.rheinhardt at gmail.com> wrote:
>
> Jan Ekström:
> > This enables people to override the sanity check without compiling
> > a new binary.
> > ---
> >  libavformat/dashdec.c | 17 ++++++++++++++---
> >  1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > index c5a5ff607b..4080b9b650 100644
> > --- a/libavformat/dashdec.c
> > +++ b/libavformat/dashdec.c
> > @@ -160,6 +160,7 @@ typedef struct DASHContext {
> >      int is_init_section_common_video;
> >      int is_init_section_common_audio;
> >
> > +    uint64_t maximum_manifest_size;
> >  } DASHContext;
> >
> >  static int ishttp(char *url)
> > @@ -1256,14 +1257,21 @@ static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in)
> >      }
> >
> >      filesize = avio_size(in);
> > -    if (filesize > MAX_MANIFEST_SIZE) {
> > -        av_log(s, AV_LOG_ERROR, "Manifest too large: %"PRId64"\n", filesize);
> > +    if (c->maximum_manifest_size && filesize > c->maximum_manifest_size) {
> > +        av_log(s, AV_LOG_ERROR,
> > +               "Manifest size too large: %"PRId64" (this sanity check can be "
> > +               "adjusted by using the option 'maximum_manifest_size')\n",
> > +               filesize);
> >          return AVERROR_INVALIDDATA;
> >      }
> >
> >      av_bprint_init(&buf, (filesize > 0) ? filesize + 1 : DEFAULT_MANIFEST_SIZE, AV_BPRINT_SIZE_UNLIMITED);
> >
> > -    if ((ret = avio_read_to_bprint(in, &buf, MAX_MANIFEST_SIZE)) < 0 ||
> > +    if ((ret = avio_read_to_bprint(in, &buf,
> > +                                   c->maximum_manifest_size > 0 ?
> > +                                   c->maximum_manifest_size :
>
> You are treating zero as "no limit", despite this not being documented.
>

Yes. I just wanted to see how people would respond to the idea. But it
seems like due to the underlying bprint api I would in any case have
to limit it to UINT_MAX, thus making an option of "don't limit the
size" not really feasible.

> > +                                   (filesize > MAX_MANIFEST_SIZE ?
> > +                                    filesize : MAX_MANIFEST_SIZE))) < 0 ||
>
> Would be clearer as FFMAX(filesize, MAX_MANIFEST_SIZE). But honestly I
> have trouble understanding why you are not just using filesize here
> (presuming it is >= 0, which is not checked here or anywhere).

True, that would be clearer. I think I just utilized it this way
because if file size is larger than MAX_MANIFEST_SIZE, it should
definitely be larger than zero :) .

Jan