diff mbox series

[FFmpeg-devel] avformat/dashdec: only limit DASH manifest size according to AVBprint limits

Message ID 20200903172901.44199-1-jeebjp@gmail.com
State Accepted
Commit 3249c757aed678780e22e99a1a49f4672851bca9
Headers show
Series [FFmpeg-devel] avformat/dashdec: only limit DASH manifest size according to AVBprint limits
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. 3, 2020, 5:29 p.m. UTC
Currently the API is internally limited to unsigned integers, so if we
limit the file size as well as the amount to read to UINT_MAX - 1, we
do not require additional limiting to be performed on the values.

This change is based on the fact that initially the 8*1024 value added
in 96d70694aea64616c68db8be306c159c73fb3980 was only for the case where
the file size was not known. It was not a maximum file size limit.

In 29121188983932f79aef8501652630d322a9974c this was reworked to be
a maximum manifest file size limit, while its commit message appears
to only note that it added support for larger manifest file sizes.

This should enable various unfortunately large MPEG-DASH manifests,
such as Youtube's multi-megabyte live stream archives to load up
as well as bring back the original intent of the logic.
---
 libavformat/dashdec.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Jan Ekström Sept. 4, 2020, 8:40 p.m. UTC | #1
On Thu, Sep 3, 2020 at 8:29 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> Currently the API is internally limited to unsigned integers, so if we
> limit the file size as well as the amount to read to UINT_MAX - 1, we
> do not require additional limiting to be performed on the values.
>
> This change is based on the fact that initially the 8*1024 value added
> in 96d70694aea64616c68db8be306c159c73fb3980 was only for the case where
> the file size was not known. It was not a maximum file size limit.
>
> In 29121188983932f79aef8501652630d322a9974c this was reworked to be
> a maximum manifest file size limit, while its commit message appears
> to only note that it added support for larger manifest file sizes.
>
> This should enable various unfortunately large MPEG-DASH manifests,
> such as Youtube's multi-megabyte live stream archives to load up
> as well as bring back the original intent of the logic.
> ---

Got some reviews regarding the commit message on IRC, and have updated
it to the following:

avformat/dashdec: drop arbitrary DASH manifest size limit

Currently the utilized AVBPrint API is internally limited to unsigned
integers, so if we limit the file size as well as the amount to read
to UINT_MAX - 1, we do not require additional limiting to be performed
on the values.

This change is based on the fact that initially the 8*1024 value added
in 96d70694aea64616c68db8be306c159c73fb3980 was only for the case where
the file size was not known. It was not a maximum file size limit.

In 29121188983932f79aef8501652630d322a9974c this was reworked to be
a maximum manifest file size limit, while its commit message appears
to only note that it added support for larger manifest file sizes.

This should enable various unfortunately large MPEG-DASH manifests,
such as Youtube's multi-megabyte live stream archives to load up
as well as bring back the original intent of the logic.
---

Jan
Steven Liu Sept. 4, 2020, 10:05 p.m. UTC | #2
Jan Ekström <jeebjp@gmail.com> 于2020年9月4日周五 上午2:33写道:
>
> Currently the API is internally limited to unsigned integers, so if we
> limit the file size as well as the amount to read to UINT_MAX - 1, we
> do not require additional limiting to be performed on the values.
>
> This change is based on the fact that initially the 8*1024 value added
> in 96d70694aea64616c68db8be306c159c73fb3980 was only for the case where
> the file size was not known. It was not a maximum file size limit.
>
> In 29121188983932f79aef8501652630d322a9974c this was reworked to be
> a maximum manifest file size limit, while its commit message appears
> to only note that it added support for larger manifest file sizes.
>
> This should enable various unfortunately large MPEG-DASH manifests,
> such as Youtube's multi-megabyte live stream archives to load up
> as well as bring back the original intent of the logic.
> ---
>  libavformat/dashdec.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> index c5a5ff607b..1e9985f32c 100644
> --- a/libavformat/dashdec.c
> +++ b/libavformat/dashdec.c
> @@ -29,7 +29,7 @@
>  #include "dash.h"
>
>  #define INITIAL_BUFFER_SIZE 32768
> -#define MAX_MANIFEST_SIZE 50 * 1024
> +#define MAX_BPRINT_READ_SIZE (UINT_MAX - 1)
>  #define DEFAULT_MANIFEST_SIZE 8 * 1024
>
>  struct fragment {
> @@ -1256,14 +1256,16 @@ static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in)
>      }
>
>      filesize = avio_size(in);
> -    if (filesize > MAX_MANIFEST_SIZE) {
> +    filesize = filesize > 0 ? filesize : DEFAULT_MANIFEST_SIZE;
> +
> +    if (filesize > MAX_BPRINT_READ_SIZE) {
>          av_log(s, AV_LOG_ERROR, "Manifest too large: %"PRId64"\n", filesize);
>          return AVERROR_INVALIDDATA;
>      }
>
> -    av_bprint_init(&buf, (filesize > 0) ? filesize + 1 : DEFAULT_MANIFEST_SIZE, AV_BPRINT_SIZE_UNLIMITED);
> +    av_bprint_init(&buf, filesize + 1, AV_BPRINT_SIZE_UNLIMITED);
>
> -    if ((ret = avio_read_to_bprint(in, &buf, MAX_MANIFEST_SIZE)) < 0 ||
> +    if ((ret = avio_read_to_bprint(in, &buf, MAX_BPRINT_READ_SIZE)) < 0 ||
>          !avio_feof(in) ||
>          (filesize = buf.len) == 0) {
>          av_log(s, AV_LOG_ERROR, "Unable to read to manifest '%s'\n", url);
> --
> 2.26.2
>
> _______________________________________________
> 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".

Not proficient in English, but code looks ok. :D


Thanks jeeb


Steven
Jan Ekström Sept. 5, 2020, 8:58 a.m. UTC | #3
On Sat, Sep 5, 2020 at 1:12 AM Steven Liu <lingjiujianke@gmail.com> wrote:
>
> Jan Ekström <jeebjp@gmail.com> 于2020年9月4日周五 上午2:33写道:
> >
> > Currently the API is internally limited to unsigned integers, so if we
> > limit the file size as well as the amount to read to UINT_MAX - 1, we
> > do not require additional limiting to be performed on the values.
> >
> > This change is based on the fact that initially the 8*1024 value added
> > in 96d70694aea64616c68db8be306c159c73fb3980 was only for the case where
> > the file size was not known. It was not a maximum file size limit.
> >
> > In 29121188983932f79aef8501652630d322a9974c this was reworked to be
> > a maximum manifest file size limit, while its commit message appears
> > to only note that it added support for larger manifest file sizes.
> >
> > This should enable various unfortunately large MPEG-DASH manifests,
> > such as Youtube's multi-megabyte live stream archives to load up
> > as well as bring back the original intent of the logic.
> > ---
> >  libavformat/dashdec.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > index c5a5ff607b..1e9985f32c 100644
> > --- a/libavformat/dashdec.c
> > +++ b/libavformat/dashdec.c
> > @@ -29,7 +29,7 @@
> >  #include "dash.h"
> >
> >  #define INITIAL_BUFFER_SIZE 32768
> > -#define MAX_MANIFEST_SIZE 50 * 1024
> > +#define MAX_BPRINT_READ_SIZE (UINT_MAX - 1)
> >  #define DEFAULT_MANIFEST_SIZE 8 * 1024
> >
> >  struct fragment {
> > @@ -1256,14 +1256,16 @@ static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in)
> >      }
> >
> >      filesize = avio_size(in);
> > -    if (filesize > MAX_MANIFEST_SIZE) {
> > +    filesize = filesize > 0 ? filesize : DEFAULT_MANIFEST_SIZE;
> > +
> > +    if (filesize > MAX_BPRINT_READ_SIZE) {
> >          av_log(s, AV_LOG_ERROR, "Manifest too large: %"PRId64"\n", filesize);
> >          return AVERROR_INVALIDDATA;
> >      }
> >
> > -    av_bprint_init(&buf, (filesize > 0) ? filesize + 1 : DEFAULT_MANIFEST_SIZE, AV_BPRINT_SIZE_UNLIMITED);
> > +    av_bprint_init(&buf, filesize + 1, AV_BPRINT_SIZE_UNLIMITED);
> >
> > -    if ((ret = avio_read_to_bprint(in, &buf, MAX_MANIFEST_SIZE)) < 0 ||
> > +    if ((ret = avio_read_to_bprint(in, &buf, MAX_BPRINT_READ_SIZE)) < 0 ||
> >          !avio_feof(in) ||
> >          (filesize = buf.len) == 0) {
> >          av_log(s, AV_LOG_ERROR, "Unable to read to manifest '%s'\n", url);
> > --
> > 2.26.2
> >
> > _______________________________________________
> > 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".
>
> Not proficient in English, but code looks ok. :D
>
>
> Thanks jeeb
>
>
> Steven

Thanks, applied as 3249c757aed678780e22e99a1a49f4672851bca9 with the
updated commit message.

Jan
diff mbox series

Patch

diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
index c5a5ff607b..1e9985f32c 100644
--- a/libavformat/dashdec.c
+++ b/libavformat/dashdec.c
@@ -29,7 +29,7 @@ 
 #include "dash.h"
 
 #define INITIAL_BUFFER_SIZE 32768
-#define MAX_MANIFEST_SIZE 50 * 1024
+#define MAX_BPRINT_READ_SIZE (UINT_MAX - 1)
 #define DEFAULT_MANIFEST_SIZE 8 * 1024
 
 struct fragment {
@@ -1256,14 +1256,16 @@  static int parse_manifest(AVFormatContext *s, const char *url, AVIOContext *in)
     }
 
     filesize = avio_size(in);
-    if (filesize > MAX_MANIFEST_SIZE) {
+    filesize = filesize > 0 ? filesize : DEFAULT_MANIFEST_SIZE;
+
+    if (filesize > MAX_BPRINT_READ_SIZE) {
         av_log(s, AV_LOG_ERROR, "Manifest too large: %"PRId64"\n", filesize);
         return AVERROR_INVALIDDATA;
     }
 
-    av_bprint_init(&buf, (filesize > 0) ? filesize + 1 : DEFAULT_MANIFEST_SIZE, AV_BPRINT_SIZE_UNLIMITED);
+    av_bprint_init(&buf, filesize + 1, AV_BPRINT_SIZE_UNLIMITED);
 
-    if ((ret = avio_read_to_bprint(in, &buf, MAX_MANIFEST_SIZE)) < 0 ||
+    if ((ret = avio_read_to_bprint(in, &buf, MAX_BPRINT_READ_SIZE)) < 0 ||
         !avio_feof(in) ||
         (filesize = buf.len) == 0) {
         av_log(s, AV_LOG_ERROR, "Unable to read to manifest '%s'\n", url);