[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:56 PM Andreas Rheinhardt
<andreas.rheinhardt at gmail.com> wrote:
>
> 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 :) .
> >
>
> My thinking was this: If filesize is >= 0 and if it passed all checks
> already, then just use avio_read_to_bprint(in, &buf, filesize). After
> all, we know the filesize or is there some reason to believe it to be wrong?
>

That would work with filesize > 0, as avio_read_to_bprint would just
return 0 without any data read in case it was zero.

As for HTTP and such, I'm actually not sure how libavformat handles
file sizes. I would expect it to trust the HTTP header value if
available, but what happens when it's either not available, or if
there is compression being applied over the wire (gzip compression
etc, although I have not checked if the libavformat HTTP
implementation implements this).

Thus an unset (either zero or negative) file size could be a reality,
which is why I would expect the current code to have utilized the
maximum manifest size as the size to read, as opposed to the file size
itself.

Jan