[FFmpeg-devel] [PATCH] lavc/vvc: Add max parameter to kth_order_egk_decode

[Nuo Mi]

Hi Frank,
thank you for the patch
On Sat, Jul 12, 2025 at 6:41 PM Frank Plowman <post at frankplowman.com> wrote:

> Prior to this patch, kth_order_egk_decode could read arbitrarily
> large values which then overflowed and caused various issues.
> Patch fixes this by making kth_order_egk_decode falliable,
> requiring the caller to specify an upper bound and returning an
> error if the read value would exceed that bound.
>
> This patch resolves the same issue as
> eb52251c0ab025b6b40b28994bc9dc616813b190, but I think this is the proper
> fix as it also addresses issues with syntax elements besides
> ff_vvc_num_signalled_palette_entries.
>
> Patch also includes a minor fix in hls_palette_coding, where the
> error code returned by palette_subblock_data was previously unchecked.
>
It's better to provide a separate patch for this, it will help with
bisecting issues more easily.

>
> Signed-off-by: Frank Plowman <post at frankplowman.com>
> ---
> I would appreciate a second pair of eyes on my changes to
> kth_order_egk_decode, particularly wrt the behaviour concerning
> potential overflows.  My understanding when writing this was that
> overflows can only potentially occur when reading the prefix, hence the
> per-prefix bit check there.  Then for the suffix no overflow can occur
> so we just check the final value.
>
The suffix involves a shift operation, so overflow is theoretically
possible. However, since we provide a max, overflow will not occur.

> ---
>  libavcodec/vvc/cabac.c | 27 ++++++++++++++++-----------
>  libavcodec/vvc/cabac.h |  6 +++---
>  libavcodec/vvc/ctu.c   | 39 +++++++++++++++++++++++++--------------
>  3 files changed, 44 insertions(+), 28 deletions(-)
>
> diff --git a/libavcodec/vvc/cabac.c b/libavcodec/vvc/cabac.c
> index 6847ce59af..e0cc82c3e1 100644
> --- a/libavcodec/vvc/cabac.c
> +++ b/libavcodec/vvc/cabac.c
> @@ -929,24 +929,29 @@ static int truncated_binary_decode(VVCLocalContext
> *lc, const int c_max)
>  }
>
>  // 9.3.3.5 k-th order Exp - Golomb binarization process
> -static int kth_order_egk_decode(CABACContext *c, int k)
> +static int kth_order_egk_decode(CABACContext *c, int *value, int k, const
> int max)

The return value is never negative under normal conditions, so we can use a
negative value to indicate an error. This allows us to simplify the
function signature as follows:
static int kth_order_egk_decode(CABACContext *c, int k, const int max)

>

 {
>      int bit    = 1;
> -    int value  = 0;
>      int symbol = 0;
> +    *value     = 0;
>
>      while (bit) {
>          bit = get_cabac_bypass(c);
> -        value += bit << k++;
> +        if (max - *value < (bit << k))
> +            return AVERROR_INVALIDDATA;
> +        *value += bit << k++;
>      }
>
>      if (--k) {
>          for (int i = 0; i < k; i++)
>              symbol = (symbol << 1) | get_cabac_bypass(c);
> -        value += symbol;
> +        *value += symbol;
>      }
>
> -    return value;
> +    if (*value > max)
> +        return AVERROR_INVALIDDATA;
> +
> +    return 0;
>  }
>
>  // 9.3.3.6 Limited k-th order Exp-Golomb binarization process
> @@ -1377,14 +1382,14 @@ int ff_vvc_intra_chroma_pred_mode(VVCLocalContext
> *lc)
>      return (get_cabac_bypass(&lc->ep->cc) << 1) |
> get_cabac_bypass(&lc->ep->cc);
>  }
>
> -int ff_vvc_palette_predictor_run(VVCLocalContext *lc)
> +int ff_vvc_palette_predictor_run(VVCLocalContext *lc, int *value, const
> int max)
>  {
> -    return kth_order_egk_decode(&lc->ep->cc, 0);
> +    return kth_order_egk_decode(&lc->ep->cc, value, 0, max);
>  }
>
> -int ff_vvc_num_signalled_palette_entries(VVCLocalContext *lc)
> +int ff_vvc_num_signalled_palette_entries(VVCLocalContext *lc, int *value,
> const int max)
>  {
> -    return kth_order_egk_decode(&lc->ep->cc, 0);
> +    return kth_order_egk_decode(&lc->ep->cc, value, 0, max);
>  }
>
>  int ff_vvc_new_palette_entries(VVCLocalContext *lc, const int bit_depth)
> @@ -1424,9 +1429,9 @@ int ff_vvc_palette_idx_idc(VVCLocalContext *lc,
> const int max_palette_index, con
>      return truncated_binary_decode(lc, max_palette_index - adjust);
>  }
>
> -int ff_vvc_palette_escape_val(VVCLocalContext *lc)
> +int ff_vvc_palette_escape_val(VVCLocalContext *lc, int *value, const int
> max)
>  {
> -    return kth_order_egk_decode(&lc->ep->cc, 5);
> +    return kth_order_egk_decode(&lc->ep->cc, value, 5, max);
>  }
>
>  int ff_vvc_general_merge_flag(VVCLocalContext *lc)
> diff --git a/libavcodec/vvc/cabac.h b/libavcodec/vvc/cabac.h
> index 972890317e..a0bea4a426 100644
> --- a/libavcodec/vvc/cabac.h
> +++ b/libavcodec/vvc/cabac.h
> @@ -81,15 +81,15 @@ int ff_vvc_intra_luma_mpm_remainder(VVCLocalContext
> *lc);
>  int ff_vvc_cclm_mode_flag(VVCLocalContext *lc);
>  int ff_vvc_cclm_mode_idx(VVCLocalContext *lc);
>  int ff_vvc_intra_chroma_pred_mode(VVCLocalContext *lc);
> -int ff_vvc_palette_predictor_run(VVCLocalContext *lc);
> -int ff_vvc_num_signalled_palette_entries(VVCLocalContext *lc);
> +int ff_vvc_palette_predictor_run(VVCLocalContext *lc, int *value, const
> int max);
> +int ff_vvc_num_signalled_palette_entries(VVCLocalContext *lc, int *value,
> const int max);
>  int ff_vvc_new_palette_entries(VVCLocalContext *lc, int bit_dpeth);
>  bool ff_vvc_palette_escape_val_present_flag(VVCLocalContext *lc);
>  bool ff_vvc_palette_transpose_flag(VVCLocalContext *lc);
>  bool ff_vvc_run_copy_flag(VVCLocalContext *lc, int prev_run_type, int
> prev_run_position, int cur_pos);
>  bool ff_vvc_copy_above_palette_indices_flag(VVCLocalContext *lc);
>  int ff_vvc_palette_idx_idc(VVCLocalContext *lc, int max_palette_index,
> bool adjust);
> -int ff_vvc_palette_escape_val(VVCLocalContext *lc);
> +int ff_vvc_palette_escape_val(VVCLocalContext *lc, int *value, const int
> max);
>
>  //inter
>  int ff_vvc_general_merge_flag(VVCLocalContext *lc);
> diff --git a/libavcodec/vvc/ctu.c b/libavcodec/vvc/ctu.c
> index cf7edccb8b..b0fe36e817 100644
> --- a/libavcodec/vvc/ctu.c
> +++ b/libavcodec/vvc/ctu.c
> @@ -1850,6 +1850,7 @@ static int palette_predicted(VVCLocalContext *lc,
> const bool local_dual_tree, in
>  {
>      CodingUnit  *cu  = lc->cu;
>      int nb_predicted = 0;
> +    int ret;
>
>      if (local_dual_tree) {
>          start = LUMA;
> @@ -1857,16 +1858,17 @@ static int palette_predicted(VVCLocalContext *lc,
> const bool local_dual_tree, in
>      }
>
>      for (int i = 0; i < predictor_size && nb_predicted < max_entries;
> i++) {
> -        const int run = ff_vvc_palette_predictor_run(lc);
> +        int run;
> +        ret = ff_vvc_palette_predictor_run(lc, &run, predictor_size - i);
> +        if (ret < 0)
> +            return ret;
> +
>          if (run == 1)
>              break;
>
>          if (run > 1)
>              i += run - 1;
>
> -        if (i >= predictor_size)
> -            return AVERROR_INVALIDDATA;
> -
>          predictor_reused[i] = true;
>          for (int c = start; c < end; c++)
>              cu->plt[c].entries[nb_predicted] = lc->ep->pp[c].entries[i];
> @@ -1885,12 +1887,17 @@ static int palette_signaled(VVCLocalContext *lc,
> const bool local_dual_tree,
>      const VVCSPS *sps         = lc->fc->ps.sps;
>      CodingUnit  *cu           = lc->cu;
>      const int nb_predicted    = cu->plt[start].size;
> -    const int nb_signaled     = nb_predicted < max_entries ?
> ff_vvc_num_signalled_palette_entries(lc) : 0;
> -    const int size            = nb_predicted + nb_signaled;
>      const bool dual_tree_luma = local_dual_tree && cu->tree_type ==
> DUAL_TREE_LUMA;
> +    int nb_signaled, size;
>
-    if (size > max_entries || nb_signaled < 0)
> -        return AVERROR_INVALIDDATA;
> +    if (nb_predicted < max_entries) {
> +        const int ret = ff_vvc_num_signalled_palette_entries(lc,
> &nb_signaled, max_entries - nb_predicted);
> +        if (ret < 0)
> +            return ret;
> +    } else
> +        nb_signaled = 0;
> +
> +    size = nb_predicted + nb_signaled;
>


>      for (int c = start; c < end; c++) {
>          Palette *plt = cu->plt + c;
> @@ -2052,10 +2059,11 @@ static int palette_subblock_data(VVCLocalContext
> *lc,
>              if (!(xc & hs) && !(yc & vs)) {
>                  const int v = PALETTE_INDEX(xc, yc);
>                  if (v == esc) {
> -                    const int coeff = ff_vvc_palette_escape_val(lc);
> -                    if (coeff >= (1U << sps->bit_depth))
> -                        return AVERROR_INVALIDDATA;
> -                    const int pixel = av_clip_intp2(RSHIFT(coeff * scale,
> 6), sps->bit_depth);
> +                    int coeff, pixel;
> +                    const int ret = ff_vvc_palette_escape_val(lc, &coeff,
> (1 << sps->bit_depth) - 1);
> +                    if (ret < 0)
> +                        return ret;
> +                    pixel = av_clip_intp2(RSHIFT(coeff * scale, 6),
> sps->bit_depth);
>                      PALETTE_SET_PIXEL(xc, yc, pixel);
>                  } else {
>                      PALETTE_SET_PIXEL(xc, yc, plt->entries[v]);
> @@ -2118,9 +2126,12 @@ static int hls_palette_coding(VVCLocalContext *lc,
> const VVCTreeType tree_type)
>      palette_qp(lc, tree_type, escape_present);
>
>      index[0] = 0;
> -    for (int i = 0; i <= (cu->cb_width * cu->cb_height - 1) >> 4; i++)
> -        palette_subblock_data(lc, max_index, i, transpose,
> +    for (int i = 0; i <= (cu->cb_width * cu->cb_height - 1) >> 4; i++) {
> +        ret = palette_subblock_data(lc, max_index, i, transpose,
>              run_type, index, &prev_run_pos, &adjust);
> +        if (ret < 0)
> +            return ret;
> +    }
>
>      return 0;
>  }
> --
> 2.47.0
>
>