Jan Ekström:
> On Tue, Sep 1, 2020 at 9:56 PM Andreas Rheinhardt
> <andreas.rheinha...@gmail.com> wrote:
>>
>> Jan Ekström:
>>> On Tue, Sep 1, 2020 at 9:31 PM Andreas Rheinhardt
>>> <andreas.rheinha...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
