diff mbox series

[FFmpeg-devel] avcodec/hevcdec: slice decoder, fix crash for thread_number > 16

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

Checks

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

Commit Message

Nuo Mi Nov. 28, 2020, 3:55 p.m. UTC 
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(-)

Comments

Marton Balint Nov. 28, 2020, 5:54 p.m. UTC | #1 
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
Nuo Mi Nov. 29, 2020, 1:39 a.m. UTC | #2 
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".
Marton Balint Nov. 29, 2020, 11:34 a.m. UTC | #3 
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".
Nuo Mi Nov. 29, 2020, 2:21 p.m. UTC | #4 
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 mbox series

Patch

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;
 }