| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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} > }; > >
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
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". >
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
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 --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} };