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

[Andreas Rheinhardt]

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.
> 
Ok, if the file size might be wrong, your code makes sense.
Yet there is a catch in your check: filesize > c->maximum_manifest_size.
filesize will be promoted to uint64_t here.

- Andreas