| Message ID | 20170824231532.16002-6-michael@niedermayer.cc |
|---|---|
| State | Superseded |
| Headers | show |
2017-08-25 7:15 GMT+08:00 Michael Niedermayer <michael@niedermayer.cc>: > Fixes: loop.m3u > > The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome Why not give a option to user for set the reload_count and limit from 1 - 10000? > > Found-by: Xiaohei and Wangchu from Alibaba Security Team > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/hls.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavformat/hls.c b/libavformat/hls.c > index 01731bd36b..26f4ebd965 100644 > --- a/libavformat/hls.c > +++ b/libavformat/hls.c > @@ -1263,6 +1263,7 @@ static int read_data(void *opaque, uint8_t *buf, int buf_size) > HLSContext *c = v->parent->priv_data; > int ret, i; > int just_opened = 0; > + int reload_count = 0; > > restart: > if (!v->needed) > @@ -1294,6 +1295,9 @@ restart: > reload_interval = default_reload_interval(v); > > reload: > + reload_count++; > + if (reload_count > 10000) > + return AVERROR_EOF; > if (!v->finished && > av_gettime_relative() - v->last_load_time >= reload_interval) { > if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) { > -- > 2.14.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, 25 Aug 2017 01:15:32 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: loop.m3u > > The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome > > Found-by: Xiaohei and Wangchu from Alibaba Security Team > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/hls.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavformat/hls.c b/libavformat/hls.c > index 01731bd36b..26f4ebd965 100644 > --- a/libavformat/hls.c > +++ b/libavformat/hls.c > @@ -1263,6 +1263,7 @@ static int read_data(void *opaque, uint8_t *buf, int buf_size) > HLSContext *c = v->parent->priv_data; > int ret, i; > int just_opened = 0; > + int reload_count = 0; > > restart: > if (!v->needed) > @@ -1294,6 +1295,9 @@ restart: > reload_interval = default_reload_interval(v); > > reload: > + reload_count++; > + if (reload_count > 10000) > + return AVERROR_EOF; > if (!v->finished && > av_gettime_relative() - v->last_load_time >= reload_interval) { > if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) { Why 1000? This patch is unnecessary. The interrupt callback can break out of this loop anyway on user request. Or is this patch again intended for transcode servers with hilariously insecure amateurish configuration, like most of these patches seem to be?
On Fri, Aug 25, 2017 at 10:08:23AM +0200, wm4 wrote: > On Fri, 25 Aug 2017 01:15:32 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > Fixes: loop.m3u > > > > The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome > > > > Found-by: Xiaohei and Wangchu from Alibaba Security Team > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/hls.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/libavformat/hls.c b/libavformat/hls.c > > index 01731bd36b..26f4ebd965 100644 > > --- a/libavformat/hls.c > > +++ b/libavformat/hls.c > > @@ -1263,6 +1263,7 @@ static int read_data(void *opaque, uint8_t *buf, int buf_size) > > HLSContext *c = v->parent->priv_data; > > int ret, i; > > int just_opened = 0; > > + int reload_count = 0; > > > > restart: > > if (!v->needed) > > @@ -1294,6 +1295,9 @@ restart: > > reload_interval = default_reload_interval(v); > > > > reload: > > + reload_count++; > > + if (reload_count > 10000) > > + return AVERROR_EOF; > > if (!v->finished && > > av_gettime_relative() - v->last_load_time >= reload_interval) { > > if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) { > > Why 1000? Please read the commit message at the top of the mail you reply to, that is explained in it > > This patch is unnecessary. The interrupt callback can break out of this > loop anyway on user request. Or is this patch again intended for > transcode servers with hilariously insecure amateurish configuration, > like most of these patches seem to be? Do you object to this issue being fixed ? In which case i will of course respect your veto and leave the issue open. Otherwise i will here note that i disagree with your oppinion. And will resubmit a patch with the changes requested by the maintainer Thanks
On Fri, Aug 25, 2017 at 08:23:32AM +0800, Steven Liu wrote: > 2017-08-25 7:15 GMT+08:00 Michael Niedermayer <michael@niedermayer.cc>: > > Fixes: loop.m3u > > > > The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome > Why not give a option to user for set the reload_count and limit from 1 - 10000? patch submitted thx [...]
On Fri, 25 Aug 2017 11:59:54 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, Aug 25, 2017 at 10:08:23AM +0200, wm4 wrote: > > On Fri, 25 Aug 2017 01:15:32 +0200 > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > Fixes: loop.m3u > > > > > > The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome > > > > > > Found-by: Xiaohei and Wangchu from Alibaba Security Team > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > --- > > > libavformat/hls.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/libavformat/hls.c b/libavformat/hls.c > > > index 01731bd36b..26f4ebd965 100644 > > > --- a/libavformat/hls.c > > > +++ b/libavformat/hls.c > > > @@ -1263,6 +1263,7 @@ static int read_data(void *opaque, uint8_t *buf, int buf_size) > > > HLSContext *c = v->parent->priv_data; > > > int ret, i; > > > int just_opened = 0; > > > + int reload_count = 0; > > > > > > restart: > > > if (!v->needed) > > > @@ -1294,6 +1295,9 @@ restart: > > > reload_interval = default_reload_interval(v); > > > > > > reload: > > > + reload_count++; > > > + if (reload_count > 10000) > > > + return AVERROR_EOF; > > > if (!v->finished && > > > av_gettime_relative() - v->last_load_time >= reload_interval) { > > > if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) { > > > > Why 1000? > > Please read the commit message at the top of the mail you reply to, > that is explained in it > > > > > > This patch is unnecessary. The interrupt callback can break out of this > > loop anyway on user request. Or is this patch again intended for > > transcode servers with hilariously insecure amateurish configuration, > > like most of these patches seem to be? > > Do you object to this issue being fixed ? I content that there is an issue. It was found by a fuzzer, and has no security implications. > In which case i will of course respect your veto and leave the issue > open. > Otherwise i will here note that i disagree with your oppinion. > > And will resubmit a patch with the changes requested by the maintainer A user option is even worse, because it creates API for a thing that doesn't matter.
On Fri, Aug 25, 2017 at 01:03:35PM +0200, wm4 wrote: > On Fri, 25 Aug 2017 11:59:54 +0200 > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > On Fri, Aug 25, 2017 at 10:08:23AM +0200, wm4 wrote: > > > On Fri, 25 Aug 2017 01:15:32 +0200 > > > Michael Niedermayer <michael@niedermayer.cc> wrote: > > > > > > > Fixes: loop.m3u > > > > > > > > The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome > > > > > > > > Found-by: Xiaohei and Wangchu from Alibaba Security Team > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > > > --- > > > > libavformat/hls.c | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/libavformat/hls.c b/libavformat/hls.c > > > > index 01731bd36b..26f4ebd965 100644 > > > > --- a/libavformat/hls.c > > > > +++ b/libavformat/hls.c > > > > @@ -1263,6 +1263,7 @@ static int read_data(void *opaque, uint8_t *buf, int buf_size) > > > > HLSContext *c = v->parent->priv_data; > > > > int ret, i; > > > > int just_opened = 0; > > > > + int reload_count = 0; > > > > > > > > restart: > > > > if (!v->needed) > > > > @@ -1294,6 +1295,9 @@ restart: > > > > reload_interval = default_reload_interval(v); > > > > > > > > reload: > > > > + reload_count++; > > > > + if (reload_count > 10000) > > > > + return AVERROR_EOF; > > > > if (!v->finished && > > > > av_gettime_relative() - v->last_load_time >= reload_interval) { > > > > if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) { > > > > > > Why 1000? > > > > Please read the commit message at the top of the mail you reply to, > > that is explained in it > > > > > > > > > > This patch is unnecessary. The interrupt callback can break out of this > > > loop anyway on user request. Or is this patch again intended for > > > transcode servers with hilariously insecure amateurish configuration, > > > like most of these patches seem to be? > > > > Do you object to this issue being fixed ? > > I content that there is an issue. It was found by a fuzzer, and has no > security implications. I belive you should CC the security researchers who found the issue. Also to repeat, my oppinion is that this is a security issue and requires a fix. > > > In which case i will of course respect your veto and leave the issue > > open. > > Otherwise i will here note that i disagree with your oppinion. > > > > And will resubmit a patch with the changes requested by the maintainer > > A user option is even worse, because it creates API for a thing that > doesn't matter. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/libavformat/hls.c b/libavformat/hls.c index 01731bd36b..26f4ebd965 100644 --- a/libavformat/hls.c +++ b/libavformat/hls.c @@ -1263,6 +1263,7 @@ static int read_data(void *opaque, uint8_t *buf, int buf_size) HLSContext *c = v->parent->priv_data; int ret, i; int just_opened = 0; + int reload_count = 0; restart: if (!v->needed) @@ -1294,6 +1295,9 @@ restart: reload_interval = default_reload_interval(v); reload: + reload_count++; + if (reload_count > 10000) + return AVERROR_EOF; if (!v->finished && av_gettime_relative() - v->last_load_time >= reload_interval) { if ((ret = parse_playlist(c, v->url, v, NULL)) < 0) {
Fixes: loop.m3u The max iteration count of 10000 is arbitrary and ideas for a better solution are welcome Found-by: Xiaohei and Wangchu from Alibaba Security Team Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/hls.c | 4 ++++ 1 file changed, 4 insertions(+)