Message ID | 20210210175214.8217-1-jamrial@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avcodec/hevcdec: check that the local context list exists before dereferencing it | 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 |
James Almer: > Since the decoder is not flagged as init cleanup capable, hevc_decode_free() > is being called manually if the hevc_decode_extradata() call fails at the end > of hevc_decode_init(). > In a frame threading scenario, however, if AVCodec->init() returns an error, > ff_frame_thread_free() will be called regardless of the above flag being set > or not, resulting in hevc_decode_free() being called a second time for the > same context. > > Solve this by ensuring pointers are not dereferenced if they are NULL, and > setting the decoder as init cleanup capable. > > Fixes ticket #9099. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Maybe ff_frame_thread_free() should not call AVCodec->close() for thread contexts > where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set? > Fixing this has been on my to-do list. (The situation is even worse than you describe it: It is possible that AVCodec->close is called on an AVCodecContext whose private_data couldn't be allocated.) > libavcodec/hevcdec.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > index 92eb888033..898dac8cbb 100644 > --- a/libavcodec/hevcdec.c > +++ b/libavcodec/hevcdec.c > @@ -3417,6 +3417,7 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) > av_freep(&s->sh.offset); > av_freep(&s->sh.size); > > + if (s->HEVClcList && s->sList) { > for (i = 1; i < s->threads_number; i++) { > HEVCLocalContext *lc = s->HEVClcList[i]; > if (lc) { > @@ -3424,9 +3425,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) > av_freep(&s->sList[i]); > } > } > - if (s->HEVClc == s->HEVClcList[0]) > - s->HEVClc = NULL; > - av_freep(&s->HEVClcList[0]); > + } > + av_freep(&s->HEVClc); > av_freep(&s->HEVClcList); > av_freep(&s->sList); > > @@ -3622,7 +3622,6 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) > if (avctx->extradata_size > 0 && avctx->extradata) { > ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); > if (ret < 0) { > - hevc_decode_free(avctx); > return ret; > } > } > @@ -3673,7 +3672,7 @@ AVCodec ff_hevc_decoder = { > .capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | > AV_CODEC_CAP_SLICE_THREADS | AV_CODEC_CAP_FRAME_THREADS, > .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_EXPORTS_CROPPING | > - FF_CODEC_CAP_ALLOCATE_PROGRESS, > + FF_CODEC_CAP_ALLOCATE_PROGRESS | FF_CODEC_CAP_INIT_CLEANUP, > .profiles = NULL_IF_CONFIG_SMALL(ff_hevc_profiles), > .hw_configs = (const AVCodecHWConfigInternal *const []) { > #if CONFIG_HEVC_DXVA2_HWACCEL >
On Wed, Feb 10, 2021 at 6:57 PM Andreas Rheinhardt < andreas.rheinhardt@gmail.com> wrote: > James Almer: > > Since the decoder is not flagged as init cleanup capable, > hevc_decode_free() > > is being called manually if the hevc_decode_extradata() call fails at > the end > > of hevc_decode_init(). > > In a frame threading scenario, however, if AVCodec->init() returns an > error, > > ff_frame_thread_free() will be called regardless of the above flag being > set > > or not, resulting in hevc_decode_free() being called a second time for > the > > same context. > > > > Solve this by ensuring pointers are not dereferenced if they are NULL, > and > > setting the decoder as init cleanup capable. > > > > Fixes ticket #9099. > > > > Signed-off-by: James Almer <jamrial@gmail.com> > > --- > > Maybe ff_frame_thread_free() should not call AVCodec->close() for thread > contexts > > where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set? > > > > Fixing this has been on my to-do list. (The situation is even worse than > you describe it: It is possible that AVCodec->close is called on an > AVCodecContext whose private_data couldn't be allocated.) > So how should proceed? Apply this patch and fix other issues after it? > > > libavcodec/hevcdec.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c > > index 92eb888033..898dac8cbb 100644 > > --- a/libavcodec/hevcdec.c > > +++ b/libavcodec/hevcdec.c > > @@ -3417,6 +3417,7 @@ static av_cold int hevc_decode_free(AVCodecContext > *avctx) > > av_freep(&s->sh.offset); > > av_freep(&s->sh.size); > > > > + if (s->HEVClcList && s->sList) { > > for (i = 1; i < s->threads_number; i++) { > > HEVCLocalContext *lc = s->HEVClcList[i]; > > if (lc) { > > @@ -3424,9 +3425,8 @@ static av_cold int hevc_decode_free(AVCodecContext > *avctx) > > av_freep(&s->sList[i]); > > } > > } > > - if (s->HEVClc == s->HEVClcList[0]) > > - s->HEVClc = NULL; > > - av_freep(&s->HEVClcList[0]); > > + } > > + av_freep(&s->HEVClc); > > av_freep(&s->HEVClcList); > > av_freep(&s->sList); > > > > @@ -3622,7 +3622,6 @@ static av_cold int hevc_decode_init(AVCodecContext > *avctx) > > if (avctx->extradata_size > 0 && avctx->extradata) { > > ret = hevc_decode_extradata(s, avctx->extradata, > avctx->extradata_size, 1); > > if (ret < 0) { > > - hevc_decode_free(avctx); > > return ret; > > } > > } > > @@ -3673,7 +3672,7 @@ AVCodec ff_hevc_decoder = { > > .capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | > > AV_CODEC_CAP_SLICE_THREADS | > AV_CODEC_CAP_FRAME_THREADS, > > .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | > FF_CODEC_CAP_EXPORTS_CROPPING | > > - FF_CODEC_CAP_ALLOCATE_PROGRESS, > > + FF_CODEC_CAP_ALLOCATE_PROGRESS | > FF_CODEC_CAP_INIT_CLEANUP, > > .profiles = NULL_IF_CONFIG_SMALL(ff_hevc_profiles), > > .hw_configs = (const AVCodecHWConfigInternal *const []) { > > #if CONFIG_HEVC_DXVA2_HWACCEL > > > > _______________________________________________ > 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".
hevcdec and pthread_slice has some memory manage issues. 1. hls_slice_data_wpp did not check the return value of ff_alloc_entries and av_malloc. 2. ff_alloc_entries did not check the return value of pthread_cond_init and pthread_mutex_init 2. Even hls_slice_data_wpp return some error for memory allocation failed, decode_nal_unit may ignore the return value at the end, We need carefully handle the half created arrays/memory.
On 2/10/2021 8:59 PM, Paul B Mahol wrote: > On Wed, Feb 10, 2021 at 6:57 PM Andreas Rheinhardt < > andreas.rheinhardt@gmail.com> wrote: > >> James Almer: >>> Since the decoder is not flagged as init cleanup capable, >> hevc_decode_free() >>> is being called manually if the hevc_decode_extradata() call fails at >> the end >>> of hevc_decode_init(). >>> In a frame threading scenario, however, if AVCodec->init() returns an >> error, >>> ff_frame_thread_free() will be called regardless of the above flag being >> set >>> or not, resulting in hevc_decode_free() being called a second time for >> the >>> same context. >>> >>> Solve this by ensuring pointers are not dereferenced if they are NULL, >> and >>> setting the decoder as init cleanup capable. >>> >>> Fixes ticket #9099. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> Maybe ff_frame_thread_free() should not call AVCodec->close() for thread >> contexts >>> where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set? >>> >> >> Fixing this has been on my to-do list. (The situation is even worse than >> you describe it: It is possible that AVCodec->close is called on an >> AVCodecContext whose private_data couldn't be allocated.) >> > > So how should proceed? Apply this patch and fix other issues after it? Applied this patch. The other unchecked allocs are handled in another patch, and the ff_frame_thread_init() issues should be fixed by Andreas' patch.
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c index 92eb888033..898dac8cbb 100644 --- a/libavcodec/hevcdec.c +++ b/libavcodec/hevcdec.c @@ -3417,6 +3417,7 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) av_freep(&s->sh.offset); av_freep(&s->sh.size); + if (s->HEVClcList && s->sList) { for (i = 1; i < s->threads_number; i++) { HEVCLocalContext *lc = s->HEVClcList[i]; if (lc) { @@ -3424,9 +3425,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx) av_freep(&s->sList[i]); } } - if (s->HEVClc == s->HEVClcList[0]) - s->HEVClc = NULL; - av_freep(&s->HEVClcList[0]); + } + av_freep(&s->HEVClc); av_freep(&s->HEVClcList); av_freep(&s->sList); @@ -3622,7 +3622,6 @@ static av_cold int hevc_decode_init(AVCodecContext *avctx) if (avctx->extradata_size > 0 && avctx->extradata) { ret = hevc_decode_extradata(s, avctx->extradata, avctx->extradata_size, 1); if (ret < 0) { - hevc_decode_free(avctx); return ret; } } @@ -3673,7 +3672,7 @@ AVCodec ff_hevc_decoder = { .capabilities = AV_CODEC_CAP_DR1 | AV_CODEC_CAP_DELAY | AV_CODEC_CAP_SLICE_THREADS | AV_CODEC_CAP_FRAME_THREADS, .caps_internal = FF_CODEC_CAP_INIT_THREADSAFE | FF_CODEC_CAP_EXPORTS_CROPPING | - FF_CODEC_CAP_ALLOCATE_PROGRESS, + FF_CODEC_CAP_ALLOCATE_PROGRESS | FF_CODEC_CAP_INIT_CLEANUP, .profiles = NULL_IF_CONFIG_SMALL(ff_hevc_profiles), .hw_configs = (const AVCodecHWConfigInternal *const []) { #if CONFIG_HEVC_DXVA2_HWACCEL
Since the decoder is not flagged as init cleanup capable, hevc_decode_free() is being called manually if the hevc_decode_extradata() call fails at the end of hevc_decode_init(). In a frame threading scenario, however, if AVCodec->init() returns an error, ff_frame_thread_free() will be called regardless of the above flag being set or not, resulting in hevc_decode_free() being called a second time for the same context. Solve this by ensuring pointers are not dereferenced if they are NULL, and setting the decoder as init cleanup capable. Fixes ticket #9099. Signed-off-by: James Almer <jamrial@gmail.com> --- Maybe ff_frame_thread_free() should not call AVCodec->close() for thread contexts where AVCodec->init() failed and FF_CODEC_CAP_INIT_CLEANUP is not set? libavcodec/hevcdec.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)