Message ID | 20201128155539.125-1-nuomi2021@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | [FFmpeg-devel] avcodec/hevcdec: slice decoder, fix crash for thread_number > 16 | expand |
Context | Check | Description |
---|---|---|
andriy/x86_make | success | Make finished |
andriy/x86_make_fate | success | Make fate finished |
andriy/PPC64_make | success | Make finished |
andriy/PPC64_make_fate | success | Make fate finished |
On Sat, 28 Nov 2020, Nuo Mi wrote: > following comandline will crash the ffmpeg > ffmpeg -threads 17 -thread_type slice -i WPP_A_ericsson_MAIN_2.bit out.yuv -y > > the HEVCContext->sList size is MAX_NB_THREADS(16), any > 16 thread number will crash the application > --- > libavcodec/hevcdec.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > index 699c13bbcc..e1dae150d5 100644 > --- a/libavcodec/hevcdec.c > +++ b/libavcodec/hevcdec.c > @@ -3406,7 +3406,7 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) > av_freep(&s->sh.offset); > av_freep(&s->sh.size); > > - for (i = 1; i < s->threads_number; i++) { > + for (i = 1; i < FFMIN(s->threads_number, MAX_NB_THREADS); i++) { This should not be needed, if you check the threads_number is hevc_decode_init. > HEVCLocalContext *lc = s->HEVClcList[i]; > if (lc) { > av_freep(&s->HEVClcList[i]); > @@ -3608,6 +3608,11 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > s->threads_type = FF_THREAD_FRAME; > else > s->threads_type = FF_THREAD_SLICE; > + if (s->threads_type == FF_THREAD_SLICE && s->threads_number > MAX_NB_THREADS) { > + av_log(s->avctx, AV_LOG_ERROR, "thread number > %d is not supported.\n", MAX_NB_THREADS); > + hevc_decode_free(avctx); > + return AVERROR(EINVAL); > + } Is it possible to warn the user but gracefully continue with reduced number of threads? Mpeg2 decoder seems to do this. Regards, Marton
On Sun, Nov 29, 2020 at 1:54 AM Marton Balint <cus@passwd.hu> wrote: > > > On Sat, 28 Nov 2020, Nuo Mi wrote: > > > following comandline will crash the ffmpeg > > ffmpeg -threads 17 -thread_type slice -i WPP_A_ericsson_MAIN_2.bit > out.yuv -y > > > > the HEVCContext->sList size is MAX_NB_THREADS(16), any > 16 thread > number will crash the application > > --- > > libavcodec/hevcdec.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > index 699c13bbcc..e1dae150d5 100644 > > --- a/libavcodec/hevcdec.c > > +++ b/libavcodec/hevcdec.c > > @@ -3406,7 +3406,7 @@ static av_cold int hevc_decode_free(AVCodecContext > *avctx) > > av_freep(&s->sh.offset); > > av_freep(&s->sh.size); > > > > - for (i = 1; i < s->threads_number; i++) { > > + for (i = 1; i < FFMIN(s->threads_number, MAX_NB_THREADS); i++) { > > This should not be needed, if you check the threads_number is > hevc_decode_init. > Yes, It's needed. After the patch, we have two hevc_decode_free in this function. Both need this. > > > HEVCLocalContext *lc = s->HEVClcList[i]; > > if (lc) { > > av_freep(&s->HEVClcList[i]); > > @@ -3608,6 +3608,11 @@ static av_cold int > hevc_decode_init(AVCodecContext *avctx) > > s->threads_type = FF_THREAD_FRAME; > > else > > s->threads_type = FF_THREAD_SLICE; > > + if (s->threads_type == FF_THREAD_SLICE && s->threads_number > > MAX_NB_THREADS) { > > + av_log(s->avctx, AV_LOG_ERROR, "thread number > %d is not > supported.\n", MAX_NB_THREADS); > > + hevc_decode_free(avctx); > > + return AVERROR(EINVAL); > > + } > > Is it possible to warn the user but gracefully continue with reduced > number of threads? Mpeg2 decoder seems to do this. > Sure, thanks for the suggestion. After the change, the FFMIN(s->threads_number, MAX_NB_THREADS) still needed by https://github.com/FFmpeg/FFmpeg/blob/394e8bb385a351091cb1ba0be986f3bbb15039fd/libavcodec/hevcdec.c#L3601 > Regards, > Marton > _______________________________________________ > 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 Sun, 29 Nov 2020, Nuo Mi wrote: > On Sun, Nov 29, 2020 at 1:54 AM Marton Balint <cus@passwd.hu> wrote: > >> >> >> On Sat, 28 Nov 2020, Nuo Mi wrote: >> >> > following comandline will crash the ffmpeg >> > ffmpeg -threads 17 -thread_type slice -i WPP_A_ericsson_MAIN_2.bit >> out.yuv -y >> > >> > the HEVCContext->sList size is MAX_NB_THREADS(16), any > 16 thread >> number will crash the application >> > --- >> > libavcodec/hevcdec.c | 7 ++++++- >> > 1 file changed, 6 insertions(+), 1 deletion(-) >> > >> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c >> > index 699c13bbcc..e1dae150d5 100644 >> > --- a/libavcodec/hevcdec.c >> > +++ b/libavcodec/hevcdec.c >> > @@ -3406,7 +3406,7 @@ static av_cold int hevc_decode_free(AVCodecContext >> *avctx) >> > av_freep(&s->sh.offset); >> > av_freep(&s->sh.size); >> > >> > - for (i = 1; i < s->threads_number; i++) { >> > + for (i = 1; i < FFMIN(s->threads_number, MAX_NB_THREADS); i++) { >> >> This should not be needed, if you check the threads_number is >> hevc_decode_init. >> > Yes, It's needed. After the patch, we have two hevc_decode_free in this > function. > Both need this. Then I think it is better to reorder the initializations in hevc_decode_init, so that hevc_decode_free is only called after the number of threads is determined. Regards, Marton > >> >> > HEVCLocalContext *lc = s->HEVClcList[i]; >> > if (lc) { >> > av_freep(&s->HEVClcList[i]); >> > @@ -3608,6 +3608,11 @@ static av_cold int >> hevc_decode_init(AVCodecContext *avctx) >> > s->threads_type = FF_THREAD_FRAME; >> > else >> > s->threads_type = FF_THREAD_SLICE; >> > + if (s->threads_type == FF_THREAD_SLICE && s->threads_number > >> MAX_NB_THREADS) { >> > + av_log(s->avctx, AV_LOG_ERROR, "thread number > %d is not >> supported.\n", MAX_NB_THREADS); >> > + hevc_decode_free(avctx); >> > + return AVERROR(EINVAL); >> > + } >> >> Is it possible to warn the user but gracefully continue with reduced >> number of threads? Mpeg2 decoder seems to do this. >> > Sure, thanks for the suggestion. > After the change, the FFMIN(s->threads_number, MAX_NB_THREADS) still needed > by > https://github.com/FFmpeg/FFmpeg/blob/394e8bb385a351091cb1ba0be986f3bbb15039fd/libavcodec/hevcdec.c#L3601 > > > >> Regards, >> Marton >> _______________________________________________ >> 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". > _______________________________________________ > 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".
Hi Marton, thanks for the suggstion, I take a deep look for this, semes it's not as simple as I thought. thread_number is just one part of the problem, another problem is the wpp thread count. In avcodec_open2, we call ff_thread_init before the avctx->codec->init( avct). So, no matter how we change the thread_number in HEVCContext, we still have 17 wpp threads. Thread 17 will read/write beyond the boundaries. Do you think it's ok if I change sList and HEVClcList to a dynamic allocation pointer? Regards, Nuo On Sun, Nov 29, 2020 at 7:34 PM Marton Balint <cus@passwd.hu> wrote: > > > On Sun, 29 Nov 2020, Nuo Mi wrote: > > > On Sun, Nov 29, 2020 at 1:54 AM Marton Balint <cus@passwd.hu> wrote: > > > >> > >> > >> On Sat, 28 Nov 2020, Nuo Mi wrote: > >> > >> > following comandline will crash the ffmpeg > >> > ffmpeg -threads 17 -thread_type slice -i WPP_A_ericsson_MAIN_2.bit > >> out.yuv -y > >> > > >> > the HEVCContext->sList size is MAX_NB_THREADS(16), any > 16 thread > >> number will crash the application > >> > --- > >> > libavcodec/hevcdec.c | 7 ++++++- > >> > 1 file changed, 6 insertions(+), 1 deletion(-) > >> > > >> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > >> > index 699c13bbcc..e1dae150d5 100644 > >> > --- a/libavcodec/hevcdec.c > >> > +++ b/libavcodec/hevcdec.c > >> > @@ -3406,7 +3406,7 @@ static av_cold int > hevc_decode_free(AVCodecContext > >> *avctx) > >> > av_freep(&s->sh.offset); > >> > av_freep(&s->sh.size); > >> > > >> > - for (i = 1; i < s->threads_number; i++) { > >> > + for (i = 1; i < FFMIN(s->threads_number, MAX_NB_THREADS); i++) { > >> > >> This should not be needed, if you check the threads_number is > >> hevc_decode_init. > >> > > Yes, It's needed. After the patch, we have two hevc_decode_free in this > > function. > > Both need this. > > Then I think it is better to reorder the initializations in > hevc_decode_init, so that hevc_decode_free is only called after the number > of threads is determined. > > Regards, > Marton > > > > >> > >> > HEVCLocalContext *lc = s->HEVClcList[i]; > >> > if (lc) { > >> > av_freep(&s->HEVClcList[i]); > >> > @@ -3608,6 +3608,11 @@ static av_cold int > >> hevc_decode_init(AVCodecContext *avctx) > >> > s->threads_type = FF_THREAD_FRAME; > >> > else > >> > s->threads_type = FF_THREAD_SLICE; > >> > + if (s->threads_type == FF_THREAD_SLICE && s->threads_number > > >> MAX_NB_THREADS) { > >> > + av_log(s->avctx, AV_LOG_ERROR, "thread number > %d is not > >> supported.\n", MAX_NB_THREADS); > >> > + hevc_decode_free(avctx); > >> > + return AVERROR(EINVAL); > >> > + } > >> > >> Is it possible to warn the user but gracefully continue with reduced > >> number of threads? Mpeg2 decoder seems to do this. > >> > > Sure, thanks for the suggestion. > > After the change, the FFMIN(s->threads_number, MAX_NB_THREADS) still > needed > > by > > > https://github.com/FFmpeg/FFmpeg/blob/394e8bb385a351091cb1ba0be986f3bbb15039fd/libavcodec/hevcdec.c#L3601 > > > > > > > >> Regards, > >> Marton > >> _______________________________________________ > >> 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". > > _______________________________________________ > > 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". > _______________________________________________ > 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".
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index 699c13bbcc..e1dae150d5 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -3406,7 +3406,7 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) av_freep(&s->sh.offset); av_freep(&s->sh.size); - for (i = 1; i < s->threads_number; i++) { + for (i = 1; i < FFMIN(s->threads_number, MAX_NB_THREADS); i++) { HEVCLocalContext *lc = s->HEVClcList[i]; if (lc) { av_freep(&s->HEVClcList[i]); @@ -3608,6 +3608,11 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) s->threads_type = FF_THREAD_FRAME; else s->threads_type = FF_THREAD_SLICE; + if (s->threads_type == FF_THREAD_SLICE && s->threads_number > MAX_NB_THREADS) { + av_log(s->avctx, AV_LOG_ERROR, "thread number > %d is not supported.\n", MAX_NB_THREADS); + hevc_decode_free(avctx); + return AVERROR(EINVAL); + } return 0; }