[FFmpeg-devel] [PATCH] avcodec/vvcdec: fix potential deadlock in report_frame_progress

[Nuo Mi]

On Sun, Aug 25, 2024 at 12:36 PM Nuo Mi <nuomi2021 at gmail.com> wrote:

> Fixes:
>
> https://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-tsan&time=20240823175808
>
> Reproduction steps:
> ./configure --enable-memory-poisoning --toolchain=gcc-tsan
> --disable-stripping && make fate-vvc
>
> Root cause:
> We hold the current frame's lock while updating progress for other frames.
> This process also requires acquiring other frame locks. It could lead to a
> deadlock.
>
> Example scenario:
> - Frame F0 holds mutex M0 and then tries to lock M1.
> - Frame F1, meanwhile, holds mutex M1 and attempts to lock M0.
>
Hi Anton and James.
Upon reconsideration, the scenario described in the commit log might be
incorrect, as there's no cyclic reference.
If F0 reports progress to F1, F1 won't report progress back to F0.

Could this be a false positive?
Someone report an issue like this
https://github.com/google/sanitizers/issues/1419
And our PR test will test 200+ clips every time. I did not observe any
deadlock at least for half a year.

> ---
>  libavcodec/vvc/refs.c   | 13 +++++++------
>  libavcodec/vvc/thread.c |  8 ++++++--
>  2 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/libavcodec/vvc/refs.c b/libavcodec/vvc/refs.c
> index c1fc6132c2..bebcef7fd6 100644
> --- a/libavcodec/vvc/refs.c
> +++ b/libavcodec/vvc/refs.c
> @@ -588,12 +588,13 @@ void ff_vvc_report_progress(VVCFrame *frame, const
> VVCProgress vp, const int y)
>      VVCProgressListener *l = NULL;
>
>      ff_mutex_lock(&p->lock);
> -
> -    av_assert0(p->progress[vp] < y || p->progress[vp] == INT_MAX);
> -    p->progress[vp] = y;
> -    l = get_done_listener(p, vp);
> -    ff_cond_signal(&p->cond);
> -
> +    if (p->progress[vp] < y) {
> +        // Due to the nature of thread scheduling, later progress may
> reach this point before earlier progress.
> +        // Therefore, we only update the progress when p->progress[vp] <
> y.
> +        p->progress[vp] = y;
> +        l = get_done_listener(p, vp);
> +        ff_cond_signal(&p->cond);
> +    }
>      ff_mutex_unlock(&p->lock);
>
>      while (l) {
> diff --git a/libavcodec/vvc/thread.c b/libavcodec/vvc/thread.c
> index 74f8e4e9d0..86a7753c6a 100644
> --- a/libavcodec/vvc/thread.c
> +++ b/libavcodec/vvc/thread.c
> @@ -466,12 +466,16 @@ static void report_frame_progress(VVCFrameContext
> *fc,
>          y = old = ft->row_progress[idx];
>          while (y < ft->ctu_height &&
> atomic_load(&ft->rows[y].col_progress[idx]) == ft->ctu_width)
>              y++;
> +        if (old != y)
> +            ft->row_progress[idx] = y;
> +        // ff_vvc_report_progress will acquire other frames' locks, which
> could lead to a deadlock
> +        // We need to unlock ft->lock first
> +        ff_mutex_unlock(&ft->lock);
> +
>          if (old != y) {
>              const int progress = y == ft->ctu_height ? INT_MAX : y *
> ctu_size;
> -            ft->row_progress[idx] = y;
>              ff_vvc_report_progress(fc->ref, idx, progress);
>          }
> -        ff_mutex_unlock(&ft->lock);
>      }
>  }
>
> --
> 2.34.1
>
>