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

This enables people to override the sanity check without compiling
a new binary.
---
 libavformat/dashdec.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

Jan Ekström:
> This enables people to override the sanity check without compiling
> a new binary.
> ---
>  libavformat/dashdec.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index c5a5ff607b..bea9616f4a 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,17 @@ 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 ||

avio_read_to_bprint() uses size_t for its size parameter; the underlying
AVBPrint API uses unsigned. So using an uint64_t (or rather: allowing
something beyond UINT_MAX) seems wrong.
(Even with maximum_manifest_size and filesize <= UINT_MAX, filesize + 1
can already be outside the range of unsigned.)

Btw: Why not use a shorter name for the variable like max_manifest_size?

>          !avio_feof(in) ||
>          (filesize = buf.len) == 0) {
>          av_log(s, AV_LOG_ERROR, "Unable to read to manifest '%s'\n", url);
> @@ -2409,6 +2413,9 @@ static const AVOption dash_options[] = {
>          OFFSET(allowed_extensions), AV_OPT_TYPE_STRING,
>          {.str = "aac,m4a,m4s,m4v,mov,mp4,webm,ts"},
>          INT_MIN, INT_MAX, FLAGS},
> +    {"maximum_manifest_size", "Maximum allowed size of the MPEG-DASH manifest to read in bytes",
> +     OFFSET(maximum_manifest_size), AV_OPT_TYPE_UINT64, {.i64 = MAX_MANIFEST_SIZE},
> +     0, UINT64_MAX, FLAGS},
>      {NULL}
>  };
>  
>

On Tue, Sep 1, 2020 at 9:19 PM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Jan Ekström:
> > This enables people to override the sanity check without compiling
> > a new binary.
> > ---
> >  libavformat/dashdec.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > index c5a5ff607b..bea9616f4a 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,17 @@ 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 ||
>
> avio_read_to_bprint() uses size_t for its size parameter; the underlying
> AVBPrint API uses unsigned. So using an uint64_t (or rather: allowing
> something beyond UINT_MAX) seems wrong.
> (Even with maximum_manifest_size and filesize <= UINT_MAX, filesize + 1
> can already be outside the range of unsigned.)
>

Alright. Will remove the option to not limit with zero, as well as
putting the maximum to UINT_MAX - 1. Would that sound good?

> Btw: Why not use a shorter name for the variable like max_manifest_size?
>

Just what initially popped into my mind like this. Can rename to make
it shorter.

Jan