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

[Marton Balint]

On Sun, 29 Nov 2020, Nuo Mi wrote:

> On Sun, Nov 29, 2020 at 1:54 AM Marton Balint <cus at 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 at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".