On 14/07/2025 13:43, Nuo Mi wrote: > Hi Frank, > thank you for the patch
Thank you very much for your review. Responses inline.
> On Sat, Jul 12, 2025 at 6:41 PM Frank Plowman <p...@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.
Done in v2.
>
>>
>> Signed-off-by: Frank Plowman <p...@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.
Great, thanks for checking.
>
>> ---
>> 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)
Done in v2.
>
>>
>
> {
>> 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
>>
>>
OpenPGP_0x03A84C6A098F2C6B.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
