| Message ID | 20210902090228.13132-1-robux4@ycbcr.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/vp9: avoid using uninitialized mutex/condition | expand |
| Context | Check | Description |
|---|---|---|
| andriy/commit_msg_x86 | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/commit_msg_ppc | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
Steve Lhomme: > When using slice decoding vp9_free_entries is called before vp9_alloc_entries > is ever called. It should destroy properly initialized variables (or check it > was never called before). > > It usually works undetected as pthread implementations allows NULL as a special > value (and should return EINVAL but doesn't). But pthreadGC2 doesn't allow NULL > in pthread_mutex_destroy() and crashes when that's the case. > --- > libavcodec/vp9.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c > index 874005a5ae..d40e90b70e 100644 > --- a/libavcodec/vp9.c > +++ b/libavcodec/vp9.c > @@ -1796,6 +1796,10 @@ static av_cold int vp9_decode_init(AVCodecContext *avctx) > > s->last_bpp = 0; > s->s.h.filter.sharpness = -1; > + if (avctx->active_thread_type & FF_THREAD_SLICE) { > + pthread_mutex_init(&s->progress_mutex, NULL); > + pthread_cond_init(&s->progress_cond, NULL); > + } > > for (int i = 0; i < 3; i++) { > s->s.frames[i].tf.f = av_frame_alloc(); > 1. progress_mutex and progress_cond don't exist if compiled without threading support, so your patch will lead to compilation failures in this case. 2. I don't see a reason why these mutexes need to be initialized and destroyed when the dimensions/number of tile cols change at all. They should be initialized once during init (if slice threading is used) and freed during close (if they have been successfully initialized). 3. Initializing this condition and mutex is currently not checked. I can fix this if you want. - Andreas
On 2021-09-02 11:27, Andreas Rheinhardt wrote: > Steve Lhomme: >> When using slice decoding vp9_free_entries is called before vp9_alloc_entries >> is ever called. It should destroy properly initialized variables (or check it >> was never called before). >> >> It usually works undetected as pthread implementations allows NULL as a special >> value (and should return EINVAL but doesn't). But pthreadGC2 doesn't allow NULL >> in pthread_mutex_destroy() and crashes when that's the case. >> --- >> libavcodec/vp9.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c >> index 874005a5ae..d40e90b70e 100644 >> --- a/libavcodec/vp9.c >> +++ b/libavcodec/vp9.c >> @@ -1796,6 +1796,10 @@ static av_cold int vp9_decode_init(AVCodecContext *avctx) >> >> s->last_bpp = 0; >> s->s.h.filter.sharpness = -1; >> + if (avctx->active_thread_type & FF_THREAD_SLICE) { >> + pthread_mutex_init(&s->progress_mutex, NULL); >> + pthread_cond_init(&s->progress_cond, NULL); >> + } >> >> for (int i = 0; i < 3; i++) { >> s->s.frames[i].tf.f = av_frame_alloc(); >> > 1. progress_mutex and progress_cond don't exist if compiled without > threading support, so your patch will lead to compilation failures in > this case. Ah yes, I missed that the 2 functions are conditionally compiled. > 2. I don't see a reason why these mutexes need to be initialized and > destroyed when the dimensions/number of tile cols change at all. They > should be initialized once during init (if slice threading is used) and > freed during close (if they have been successfully initialized). Yes, it makes total sense. > 3. Initializing this condition and mutex is currently not checked. > > I can fix this if you want. Sure. This is currently leading to a crash in VLC 3.0 because we use a pthread implementation that doesn't like this case (the only lib compatible with XP). https://code.videolan.org/videolan/vlc/-/issues/26017
diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c index 874005a5ae..d40e90b70e 100644 --- a/libavcodec/vp9.c +++ b/libavcodec/vp9.c @@ -1796,6 +1796,10 @@ static av_cold int vp9_decode_init(AVCodecContext *avctx) s->last_bpp = 0; s->s.h.filter.sharpness = -1; + if (avctx->active_thread_type & FF_THREAD_SLICE) { + pthread_mutex_init(&s->progress_mutex, NULL); + pthread_cond_init(&s->progress_cond, NULL); + } for (int i = 0; i < 3; i++) { s->s.frames[i].tf.f = av_frame_alloc();