diff mbox series

[FFmpeg-devel] avcodec/vvcdec: ff_vvc_frame_submit, avoid initializing task twice.

Message ID TYSPR06MB6433019CEB35172181653C0DAA132@TYSPR06MB6433.apcprd06.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel] avcodec/vvcdec: ff_vvc_frame_submit, avoid initializing task twice. | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Nuo Mi April 21, 2024, 2:52 p.m. UTC 
For some error bitstreams, a CTU belongs to two slices/entry points.
If the decoder initializes and submmits the CTU task twice, it may crash the program
or cause it to enter an infinite loop.

Reported-by: Frank Plowman <post@frankplowman.com>
---
 libavcodec/vvc/dec.c    |  7 +++++--
 libavcodec/vvc/thread.c | 43 ++++++++++++++++++++++++++++-------------
 libavcodec/vvc/thread.h |  2 +-
 3 files changed, 36 insertions(+), 16 deletions(-)

Comments

Frank Plowman April 21, 2024, 4:34 p.m. UTC | #1 
On 21/04/2024 15:52, Nuo Mi wrote:
> For some error bitstreams, a CTU belongs to two slices/entry points.
> If the decoder initializes and submmits the CTU task twice, it may crash the program
> or cause it to enter an infinite loop.
> 
> Reported-by: Frank Plowman <post@frankplowman.com>
> ---
>  libavcodec/vvc/dec.c    |  7 +++++--
>  libavcodec/vvc/thread.c | 43 ++++++++++++++++++++++++++++-------------
>  libavcodec/vvc/thread.h |  2 +-
>  3 files changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/vvc/dec.c b/libavcodec/vvc/dec.c
> index 6aeec27eaf..4f7d184e43 100644
> --- a/libavcodec/vvc/dec.c
> +++ b/libavcodec/vvc/dec.c
> @@ -893,10 +893,13 @@ static int wait_delayed_frame(VVCContext *s, AVFrame *output, int *got_output)
>  
>  static int submit_frame(VVCContext *s, VVCFrameContext *fc, AVFrame *output, int *got_output)
>  {
> -    int ret;
> +    int ret = ff_vvc_frame_submit(s, fc);
> +    if (ret < 0)
> +        return ret;
> +
>      s->nb_frames++;
>      s->nb_delayed++;
> -    ff_vvc_frame_submit(s, fc);
> +
>      if (s->nb_delayed >= s->nb_fcs) {
>          if ((ret = wait_delayed_frame(s, output, got_output)) < 0)
>              return ret;
> diff --git a/libavcodec/vvc/thread.c b/libavcodec/vvc/thread.c
> index 01c3ff75b1..3b27811db2 100644
> --- a/libavcodec/vvc/thread.c
> +++ b/libavcodec/vvc/thread.c
> @@ -124,11 +124,17 @@ static void task_init(VVCTask *t, VVCTaskStage stage, VVCFrameContext *fc, const
>      atomic_store(&t->target_inter_score, 0);
>  }
>  
> -static void task_init_parse(VVCTask *t, SliceContext *sc, EntryPoint *ep, const int ctu_idx)
> +static int task_init_parse(VVCTask *t, SliceContext *sc, EntryPoint *ep, const int ctu_idx)
>  {
> +    if (t->sc) {
> +        // the task already inited, error bitstream
> +        return AVERROR_INVALIDDATA;
> +    }
>      t->sc      = sc;
>      t->ep      = ep;
>      t->ctu_idx = ctu_idx;
> +
> +    return 0;
>  }
>  
>  static uint8_t task_add_score(VVCTask *t, const VVCTaskStage stage)
> @@ -758,24 +764,35 @@ static void submit_entry_point(VVCContext *s, VVCFrameThread *ft, SliceContext *
>      frame_thread_add_score(s, ft, t->rx, t->ry, VVC_TASK_STAGE_PARSE);
>  }
>  
> -void ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc)
> +int ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc)
>  {
>      VVCFrameThread *ft = fc->ft;
>  
> -    for (int i = 0; i < fc->nb_slices; i++) {
> -        SliceContext *sc = fc->slices[i];
> -        for (int j = 0; j < sc->nb_eps; j++) {
> -            EntryPoint *ep = sc->eps + j;
> -            for (int k = ep->ctu_start; k < ep->ctu_end; k++) {
> -                const int rs = sc->sh.ctb_addr_in_curr_slice[k];
> -                VVCTask *t   = ft->tasks + rs;
> -
> -                task_init_parse(t, sc, ep, k);
> -                check_colocation(s, t);
> +    // We'll handle this in two passes:
> +    // Pass 0 to initialize tasks with parser, this will help detect bit stream error
> +    // Pass 1 to shedule location check and submit the entry point
> +    for (int pass = 0; pass < 2; pass++) {
> +        for (int i = 0; i < fc->nb_slices; i++) {
> +            SliceContext *sc = fc->slices[i];
> +            for (int j = 0; j < sc->nb_eps; j++) {
> +                EntryPoint *ep = sc->eps + j;
> +                for (int k = ep->ctu_start; k < ep->ctu_end; k++) {
> +                    const int rs = sc->sh.ctb_addr_in_curr_slice[k];
> +                    VVCTask *t   = ft->tasks + rs;
> +                    if (pass) {
> +                        check_colocation(s, t);
> +                    } else {
> +                        const int ret = task_init_parse(t, sc, ep, k);
> +                        if (ret < 0)
> +                            return ret;
> +                    }
> +                }
> +                if (pass)
> +                    submit_entry_point(s, ft, sc, ep);
>              }
> -            submit_entry_point(s, ft, sc, ep);
>          }
>      }
> +    return 0;
>  }
>  
>  int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext *fc)
> diff --git a/libavcodec/vvc/thread.h b/libavcodec/vvc/thread.h
> index 55bb4ea244..8ac59b2ecf 100644
> --- a/libavcodec/vvc/thread.h
> +++ b/libavcodec/vvc/thread.h
> @@ -30,7 +30,7 @@ void ff_vvc_executor_free(struct AVExecutor **e);
>  
>  int ff_vvc_frame_thread_init(VVCFrameContext *fc);
>  void ff_vvc_frame_thread_free(VVCFrameContext *fc);
> -void ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc);
> +int ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc);
>  int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext *fc);
>  
>  #endif // AVCODEC_VVC_THREAD_H

This patch fixes most of the fuzz bitstreams I have which enter infinite
loops, but also introduces a regression, turning some other bitstreams
which were okay before into infinite loops.
Nuo Mi April 25, 2024, 2:05 p.m. UTC | #2 
Thank you,  Frank
Fixed by v2


On Mon, Apr 22, 2024 at 12:34 AM Frank Plowman <post@frankplowman.com>
wrote:

> On 21/04/2024 15:52, Nuo Mi wrote:
> > For some error bitstreams, a CTU belongs to two slices/entry points.
> > If the decoder initializes and submmits the CTU task twice, it may crash
> the program
> > or cause it to enter an infinite loop.
> >
> > Reported-by: Frank Plowman <post@frankplowman.com>
> > ---
> >  libavcodec/vvc/dec.c    |  7 +++++--
> >  libavcodec/vvc/thread.c | 43 ++++++++++++++++++++++++++++-------------
> >  libavcodec/vvc/thread.h |  2 +-
> >  3 files changed, 36 insertions(+), 16 deletions(-)
> >
> > diff --git a/libavcodec/vvc/dec.c b/libavcodec/vvc/dec.c
> > index 6aeec27eaf..4f7d184e43 100644
> > --- a/libavcodec/vvc/dec.c
> > +++ b/libavcodec/vvc/dec.c
> > @@ -893,10 +893,13 @@ static int wait_delayed_frame(VVCContext *s,
> AVFrame *output, int *got_output)
> >
> >  static int submit_frame(VVCContext *s, VVCFrameContext *fc, AVFrame
> *output, int *got_output)
> >  {
> > -    int ret;
> > +    int ret = ff_vvc_frame_submit(s, fc);
> > +    if (ret < 0)
> > +        return ret;
> > +
> >      s->nb_frames++;
> >      s->nb_delayed++;
> > -    ff_vvc_frame_submit(s, fc);
> > +
> >      if (s->nb_delayed >= s->nb_fcs) {
> >          if ((ret = wait_delayed_frame(s, output, got_output)) < 0)
> >              return ret;
> > diff --git a/libavcodec/vvc/thread.c b/libavcodec/vvc/thread.c
> > index 01c3ff75b1..3b27811db2 100644
> > --- a/libavcodec/vvc/thread.c
> > +++ b/libavcodec/vvc/thread.c
> > @@ -124,11 +124,17 @@ static void task_init(VVCTask *t, VVCTaskStage
> stage, VVCFrameContext *fc, const
> >      atomic_store(&t->target_inter_score, 0);
> >  }
> >
> > -static void task_init_parse(VVCTask *t, SliceContext *sc, EntryPoint
> *ep, const int ctu_idx)
> > +static int task_init_parse(VVCTask *t, SliceContext *sc, EntryPoint
> *ep, const int ctu_idx)
> >  {
> > +    if (t->sc) {
> > +        // the task already inited, error bitstream
> > +        return AVERROR_INVALIDDATA;
> > +    }
> >      t->sc      = sc;
> >      t->ep      = ep;
> >      t->ctu_idx = ctu_idx;
> > +
> > +    return 0;
> >  }
> >
> >  static uint8_t task_add_score(VVCTask *t, const VVCTaskStage stage)
> > @@ -758,24 +764,35 @@ static void submit_entry_point(VVCContext *s,
> VVCFrameThread *ft, SliceContext *
> >      frame_thread_add_score(s, ft, t->rx, t->ry, VVC_TASK_STAGE_PARSE);
> >  }
> >
> > -void ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc)
> > +int ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc)
> >  {
> >      VVCFrameThread *ft = fc->ft;
> >
> > -    for (int i = 0; i < fc->nb_slices; i++) {
> > -        SliceContext *sc = fc->slices[i];
> > -        for (int j = 0; j < sc->nb_eps; j++) {
> > -            EntryPoint *ep = sc->eps + j;
> > -            for (int k = ep->ctu_start; k < ep->ctu_end; k++) {
> > -                const int rs = sc->sh.ctb_addr_in_curr_slice[k];
> > -                VVCTask *t   = ft->tasks + rs;
> > -
> > -                task_init_parse(t, sc, ep, k);
> > -                check_colocation(s, t);
> > +    // We'll handle this in two passes:
> > +    // Pass 0 to initialize tasks with parser, this will help detect
> bit stream error
> > +    // Pass 1 to shedule location check and submit the entry point
> > +    for (int pass = 0; pass < 2; pass++) {
> > +        for (int i = 0; i < fc->nb_slices; i++) {
> > +            SliceContext *sc = fc->slices[i];
> > +            for (int j = 0; j < sc->nb_eps; j++) {
> > +                EntryPoint *ep = sc->eps + j;
> > +                for (int k = ep->ctu_start; k < ep->ctu_end; k++) {
> > +                    const int rs = sc->sh.ctb_addr_in_curr_slice[k];
> > +                    VVCTask *t   = ft->tasks + rs;
> > +                    if (pass) {
> > +                        check_colocation(s, t);
> > +                    } else {
> > +                        const int ret = task_init_parse(t, sc, ep, k);
> > +                        if (ret < 0)
> > +                            return ret;
> > +                    }
> > +                }
> > +                if (pass)
> > +                    submit_entry_point(s, ft, sc, ep);
> >              }
> > -            submit_entry_point(s, ft, sc, ep);
> >          }
> >      }
> > +    return 0;
> >  }
> >
> >  int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext *fc)
> > diff --git a/libavcodec/vvc/thread.h b/libavcodec/vvc/thread.h
> > index 55bb4ea244..8ac59b2ecf 100644
> > --- a/libavcodec/vvc/thread.h
> > +++ b/libavcodec/vvc/thread.h
> > @@ -30,7 +30,7 @@ void ff_vvc_executor_free(struct AVExecutor **e);
> >
> >  int ff_vvc_frame_thread_init(VVCFrameContext *fc);
> >  void ff_vvc_frame_thread_free(VVCFrameContext *fc);
> > -void ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc);
> > +int ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc);
> >  int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext *fc);
> >
> >  #endif // AVCODEC_VVC_THREAD_H
>
> This patch fixes most of the fuzz bitstreams I have which enter infinite
> loops, but also introduces a regression, turning some other bitstreams
> which were okay before into infinite loops.
>
> --
> Frank
> _______________________________________________
> 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/vvc/dec.c b/libavcodec/vvc/dec.c
index 6aeec27eaf..4f7d184e43 100644
--- a/libavcodec/vvc/dec.c
+++ b/libavcodec/vvc/dec.c
@@ -893,10 +893,13 @@  static int wait_delayed_frame(VVCContext *s, AVFrame *output, int *got_output)
 
 static int submit_frame(VVCContext *s, VVCFrameContext *fc, AVFrame *output, int *got_output)
 {
-    int ret;
+    int ret = ff_vvc_frame_submit(s, fc);
+    if (ret < 0)
+        return ret;
+
     s->nb_frames++;
     s->nb_delayed++;
-    ff_vvc_frame_submit(s, fc);
+
     if (s->nb_delayed >= s->nb_fcs) {
         if ((ret = wait_delayed_frame(s, output, got_output)) < 0)
             return ret;
diff --git a/libavcodec/vvc/thread.c b/libavcodec/vvc/thread.c
index 01c3ff75b1..3b27811db2 100644
--- a/libavcodec/vvc/thread.c
+++ b/libavcodec/vvc/thread.c
@@ -124,11 +124,17 @@  static void task_init(VVCTask *t, VVCTaskStage stage, VVCFrameContext *fc, const
     atomic_store(&t->target_inter_score, 0);
 }
 
-static void task_init_parse(VVCTask *t, SliceContext *sc, EntryPoint *ep, const int ctu_idx)
+static int task_init_parse(VVCTask *t, SliceContext *sc, EntryPoint *ep, const int ctu_idx)
 {
+    if (t->sc) {
+        // the task already inited, error bitstream
+        return AVERROR_INVALIDDATA;
+    }
     t->sc      = sc;
     t->ep      = ep;
     t->ctu_idx = ctu_idx;
+
+    return 0;
 }
 
 static uint8_t task_add_score(VVCTask *t, const VVCTaskStage stage)
@@ -758,24 +764,35 @@  static void submit_entry_point(VVCContext *s, VVCFrameThread *ft, SliceContext *
     frame_thread_add_score(s, ft, t->rx, t->ry, VVC_TASK_STAGE_PARSE);
 }
 
-void ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc)
+int ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc)
 {
     VVCFrameThread *ft = fc->ft;
 
-    for (int i = 0; i < fc->nb_slices; i++) {
-        SliceContext *sc = fc->slices[i];
-        for (int j = 0; j < sc->nb_eps; j++) {
-            EntryPoint *ep = sc->eps + j;
-            for (int k = ep->ctu_start; k < ep->ctu_end; k++) {
-                const int rs = sc->sh.ctb_addr_in_curr_slice[k];
-                VVCTask *t   = ft->tasks + rs;
-
-                task_init_parse(t, sc, ep, k);
-                check_colocation(s, t);
+    // We'll handle this in two passes:
+    // Pass 0 to initialize tasks with parser, this will help detect bit stream error
+    // Pass 1 to shedule location check and submit the entry point
+    for (int pass = 0; pass < 2; pass++) {
+        for (int i = 0; i < fc->nb_slices; i++) {
+            SliceContext *sc = fc->slices[i];
+            for (int j = 0; j < sc->nb_eps; j++) {
+                EntryPoint *ep = sc->eps + j;
+                for (int k = ep->ctu_start; k < ep->ctu_end; k++) {
+                    const int rs = sc->sh.ctb_addr_in_curr_slice[k];
+                    VVCTask *t   = ft->tasks + rs;
+                    if (pass) {
+                        check_colocation(s, t);
+                    } else {
+                        const int ret = task_init_parse(t, sc, ep, k);
+                        if (ret < 0)
+                            return ret;
+                    }
+                }
+                if (pass)
+                    submit_entry_point(s, ft, sc, ep);
             }
-            submit_entry_point(s, ft, sc, ep);
         }
     }
+    return 0;
 }
 
 int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext *fc)
diff --git a/libavcodec/vvc/thread.h b/libavcodec/vvc/thread.h
index 55bb4ea244..8ac59b2ecf 100644
--- a/libavcodec/vvc/thread.h
+++ b/libavcodec/vvc/thread.h
@@ -30,7 +30,7 @@  void ff_vvc_executor_free(struct AVExecutor **e);
 
 int ff_vvc_frame_thread_init(VVCFrameContext *fc);
 void ff_vvc_frame_thread_free(VVCFrameContext *fc);
-void ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc);
+int ff_vvc_frame_submit(VVCContext *s, VVCFrameContext *fc);
 int ff_vvc_frame_wait(VVCContext *s, VVCFrameContext *fc);
 
 #endif // AVCODEC_VVC_THREAD_H