diff mbox series

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

Message ID 20200901180254.26542-1-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2,v2] avformat/dashdec: enable overriding of the maximum manifest size
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jan Ekström Sept. 1, 2020, 6:02 p.m. UTC
This enables people to override the sanity check without compiling
a new binary.
---
 libavformat/dashdec.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Andreas Rheinhardt Sept. 1, 2020, 6:24 p.m. UTC | #1
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.

> +                                   (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).

>          !avio_feof(in) ||
>          (filesize = buf.len) == 0) {
>          av_log(s, AV_LOG_ERROR, "Unable to read to manifest '%s'\n", url);
> @@ -2409,6 +2417,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}
>  };
>  
>
Jan Ekström Sept. 1, 2020, 6:41 p.m. UTC | #2
On Tue, Sep 1, 2020 at 9:31 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 | 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
Andreas Rheinhardt Sept. 1, 2020, 6:56 p.m. UTC | #3
Jan Ekström:
> On Tue, Sep 1, 2020 at 9:31 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 | 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?

> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Jan Ekström Sept. 1, 2020, 7:07 p.m. UTC | #4
On Tue, Sep 1, 2020 at 9:56 PM Andreas Rheinhardt
<andreas.rheinhardt@gmail.com> wrote:
>
> Jan Ekström:
> > On Tue, Sep 1, 2020 at 9:31 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 | 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
Andreas Rheinhardt Sept. 1, 2020, 7:14 p.m. UTC | #5
Jan Ekström:
> On Tue, Sep 1, 2020 at 9:56 PM Andreas Rheinhardt
> <andreas.rheinhardt@gmail.com> wrote:
>>
>> Jan Ekström:
>>> On Tue, Sep 1, 2020 at 9:31 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 | 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
diff mbox series

Patch

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 :
+                                   (filesize > MAX_MANIFEST_SIZE ?
+                                    filesize : MAX_MANIFEST_SIZE))) < 0 ||
         !avio_feof(in) ||
         (filesize = buf.len) == 0) {
         av_log(s, AV_LOG_ERROR, "Unable to read to manifest '%s'\n", url);
@@ -2409,6 +2417,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}
 };