| Message ID | 20240126214651.22783-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread | expand |
| 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 |
On 1/26/2024 6:46 PM, Michael Niedermayer wrote: > It is not possible to encode a index into an empty list. Thus > this must be invalid at this point or before. > Its likely a broader earlier check can be used here, someone knowing > VVC should look at that. Its not immedeatly obvious from the spec > by looking for numlayerolss Can you check if the following fixes it? > diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c > index 549d021211..40572dadb5 100644 > --- a/libavcodec/cbs_h266_syntax_template.c > +++ b/libavcodec/cbs_h266_syntax_template.c > @@ -793,6 +793,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > { > //calc NumMultiLayerOlss > int m; > + int num_layers_in_ols = 0; > uint8_t dependency_flag[VVC_MAX_LAYERS][VVC_MAX_LAYERS]; > uint16_t num_output_layers_in_ols[VVC_MAX_TOTAL_NUM_OLSS]; > uint8_t num_sub_layers_in_layer_in_ols[VVC_MAX_TOTAL_NUM_OLSS][VVC_MAX_TOTAL_NUM_OLSS]; > @@ -895,7 +896,6 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > return AVERROR_INVALIDDATA; > } > for (i = 1; i < total_num_olss; i++) { > - int num_layers_in_ols = 0; > if (current->vps_each_layer_is_an_ols_flag) { > num_layers_in_ols = 1; > } else if (current->vps_ols_mode_idc == 0 || num_layers_in_ols is not meant to be reset on every loop.
On Sat, Jan 27, 2024 at 09:25:16AM -0300, James Almer wrote: > On 1/26/2024 6:46 PM, Michael Niedermayer wrote: > > It is not possible to encode a index into an empty list. Thus > > this must be invalid at this point or before. > > Its likely a broader earlier check can be used here, someone knowing > > VVC should look at that. Its not immedeatly obvious from the spec > > by looking for numlayerolss > > Can you check if the following fixes it? > > > diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c > > index 549d021211..40572dadb5 100644 > > --- a/libavcodec/cbs_h266_syntax_template.c > > +++ b/libavcodec/cbs_h266_syntax_template.c > > @@ -793,6 +793,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > > { > > //calc NumMultiLayerOlss > > int m; > > + int num_layers_in_ols = 0; > > uint8_t dependency_flag[VVC_MAX_LAYERS][VVC_MAX_LAYERS]; > > uint16_t num_output_layers_in_ols[VVC_MAX_TOTAL_NUM_OLSS]; > > uint8_t num_sub_layers_in_layer_in_ols[VVC_MAX_TOTAL_NUM_OLSS][VVC_MAX_TOTAL_NUM_OLSS]; > > @@ -895,7 +896,6 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > > return AVERROR_INVALIDDATA; > > } > > for (i = 1; i < total_num_olss; i++) { > > - int num_layers_in_ols = 0; > > if (current->vps_each_layer_is_an_ols_flag) { > > num_layers_in_ols = 1; > > } else if (current->vps_ols_mode_idc == 0 || > > num_layers_in_ols is not meant to be reset on every loop. replacing my patch by yours does not change num_multi_layer_olss from being 0 and if its 0 then "num_multi_layer_olss - 1" causes problems as a limit more precissely this: i can also send you the file if you want? tools/target_bsf_vvc_metadata_fuzzer: Running 1 inputs 1 time(s) each. Running: /home/michael/libfuzz-repro/65160/clusterfuzz-testcase-minimized-ffmpeg_BSF_VVC_METADATA_fuzzer-4665241535119360 libavcodec/cbs_h266_syntax_template.c:1001:21: runtime error: index 257 out of bounds for type 'uint8_t [257]' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/cbs_h266_syntax_template.c:1001:21 in libavcodec/cbs_h266_syntax_template.c:1004:38: runtime error: index 257 out of bounds for type 'uint8_t [257]' SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/cbs_h266_syntax_template.c:1004:38 in ================================================================= ==29823==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7ff104e7ca18 at pc 0x0000006a4fdb bp 0x7fffd376e7f0 sp 0x7fffd376e7e8 WRITE of size 1 at 0x7ff104e7ca18 thread T0 #0 0x6a4fda in cbs_h266_read_vps libavcodec/cbs_h266_syntax_template.c:1001:21 #1 0x5c87bc in cbs_h266_read_nal_unit libavcodec/cbs_h2645.c:1071:19 #2 0x4ff3aa in cbs_read_fragment_content libavcodec/cbs.c:215:15 #3 0x4ff3aa in cbs_read_data libavcodec/cbs.c:282 #4 0x5a53ea in ff_cbs_bsf_generic_filter libavcodec/cbs_bsf.c:75:11 #5 0x4c9b48 in LLVMFuzzerTestOneInput tools/target_bsf_fuzzer.c:154:16 #6 0x138eb7d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 #7 0x1383752 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 #8 0x1388951 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 #9 0x1383430 in main Fuzzer/build/../FuzzerMain.cpp:20:10 #10 0x7ff10889ac86 in __libc_start_main /build/glibc-WcQU6j/glibc-2.27/csu/../csu/libc-start.c:310 #11 0x41f429 in _start (tools/target_bsf_vvc_metadata_fuzzer+0x41f429) 0x7ff104e7ca18 is located 0 bytes to the right of 393752-byte region [0x7ff104e1c800,0x7ff104e7ca18) allocated by thread T0 here: #0 0x497e47 in posix_memalign /b/swarming/w/ir/cache/builder/src/third_party/llvm/compiler-rt/lib/asan/asan_malloc_linux.cc:226:3 #1 0x130d3a8 in av_malloc libavutil/mem.c:105:9 #2 0x8fe1c5 in ff_refstruct_alloc_ext_c libavcodec/refstruct.c:109:11 #3 0x50c2bb in ff_cbs_alloc_unit_content libavcodec/cbs.c:933:25 #4 0x5c864f in cbs_h266_read_nal_unit libavcodec/cbs_h2645.c:1046:11 #5 0x4ff3aa in cbs_read_fragment_content libavcodec/cbs.c:215:15 #6 0x4ff3aa in cbs_read_data libavcodec/cbs.c:282 #7 0x5a53ea in ff_cbs_bsf_generic_filter libavcodec/cbs_bsf.c:75:11 #8 0x4c9b48 in LLVMFuzzerTestOneInput tools/target_bsf_fuzzer.c:154:16 #9 0x138eb7d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) Fuzzer/build/../FuzzerLoop.cpp:495:13 #10 0x1383752 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) Fuzzer/build/../FuzzerDriver.cpp:273:6 #11 0x1388951 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) Fuzzer/build/../FuzzerDriver.cpp:690:9 #12 0x1383430 in main Fuzzer/build/../FuzzerMain.cpp:20:10 #13 0x7ff10889ac86 in __libc_start_main /build/glibc-WcQU6j/glibc-2.27/csu/../csu/libc-start.c:310 [...]
On 1/27/2024 8:56 PM, Michael Niedermayer wrote: > On Sat, Jan 27, 2024 at 09:25:16AM -0300, James Almer wrote: >> On 1/26/2024 6:46 PM, Michael Niedermayer wrote: >>> It is not possible to encode a index into an empty list. Thus >>> this must be invalid at this point or before. >>> Its likely a broader earlier check can be used here, someone knowing >>> VVC should look at that. Its not immedeatly obvious from the spec >>> by looking for numlayerolss >> >> Can you check if the following fixes it? >> >>> diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c >>> index 549d021211..40572dadb5 100644 >>> --- a/libavcodec/cbs_h266_syntax_template.c >>> +++ b/libavcodec/cbs_h266_syntax_template.c >>> @@ -793,6 +793,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, >>> { >>> //calc NumMultiLayerOlss >>> int m; >>> + int num_layers_in_ols = 0; >>> uint8_t dependency_flag[VVC_MAX_LAYERS][VVC_MAX_LAYERS]; >>> uint16_t num_output_layers_in_ols[VVC_MAX_TOTAL_NUM_OLSS]; >>> uint8_t num_sub_layers_in_layer_in_ols[VVC_MAX_TOTAL_NUM_OLSS][VVC_MAX_TOTAL_NUM_OLSS]; >>> @@ -895,7 +896,6 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, >>> return AVERROR_INVALIDDATA; >>> } >>> for (i = 1; i < total_num_olss; i++) { >>> - int num_layers_in_ols = 0; >>> if (current->vps_each_layer_is_an_ols_flag) { >>> num_layers_in_ols = 1; >>> } else if (current->vps_ols_mode_idc == 0 || >> >> num_layers_in_ols is not meant to be reset on every loop. > > replacing my patch by yours does not change > num_multi_layer_olss from being 0 > and if its 0 then "num_multi_layer_olss - 1" causes problems as a limit > > more precissely this: > i can also send you the file if you want? No, this should be looked at by someone more familiar with VVC. And my patch should be applied either way. The current code is wrong.
On Sat, Jan 27, 2024 at 09:02:30PM -0300, James Almer wrote: > On 1/27/2024 8:56 PM, Michael Niedermayer wrote: > > On Sat, Jan 27, 2024 at 09:25:16AM -0300, James Almer wrote: > > > On 1/26/2024 6:46 PM, Michael Niedermayer wrote: > > > > It is not possible to encode a index into an empty list. Thus > > > > this must be invalid at this point or before. > > > > Its likely a broader earlier check can be used here, someone knowing > > > > VVC should look at that. Its not immedeatly obvious from the spec > > > > by looking for numlayerolss > > > > > > Can you check if the following fixes it? > > > > > > > diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c > > > > index 549d021211..40572dadb5 100644 > > > > --- a/libavcodec/cbs_h266_syntax_template.c > > > > +++ b/libavcodec/cbs_h266_syntax_template.c > > > > @@ -793,6 +793,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > > > > { > > > > //calc NumMultiLayerOlss > > > > int m; > > > > + int num_layers_in_ols = 0; > > > > uint8_t dependency_flag[VVC_MAX_LAYERS][VVC_MAX_LAYERS]; > > > > uint16_t num_output_layers_in_ols[VVC_MAX_TOTAL_NUM_OLSS]; > > > > uint8_t num_sub_layers_in_layer_in_ols[VVC_MAX_TOTAL_NUM_OLSS][VVC_MAX_TOTAL_NUM_OLSS]; > > > > @@ -895,7 +896,6 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > > > > return AVERROR_INVALIDDATA; > > > > } > > > > for (i = 1; i < total_num_olss; i++) { > > > > - int num_layers_in_ols = 0; > > > > if (current->vps_each_layer_is_an_ols_flag) { > > > > num_layers_in_ols = 1; > > > > } else if (current->vps_ols_mode_idc == 0 || > > > > > > num_layers_in_ols is not meant to be reset on every loop. > > > > replacing my patch by yours does not change > > num_multi_layer_olss from being 0 > > and if its 0 then "num_multi_layer_olss - 1" causes problems as a limit > > > > more precissely this: > > i can also send you the file if you want? > > No, this should be looked at by someone more familiar with VVC. ive already sent the fuzzer samples to nuomi and frank plowman > And my patch should be applied either way. The current code is wrong. I did not suggest not to do that :) just that it alone was not enough to fix this thx [...]
On 1/27/2024 9:05 PM, Michael Niedermayer wrote: > On Sat, Jan 27, 2024 at 09:02:30PM -0300, James Almer wrote: >> On 1/27/2024 8:56 PM, Michael Niedermayer wrote: >>> On Sat, Jan 27, 2024 at 09:25:16AM -0300, James Almer wrote: >>>> On 1/26/2024 6:46 PM, Michael Niedermayer wrote: >>>>> It is not possible to encode a index into an empty list. Thus >>>>> this must be invalid at this point or before. >>>>> Its likely a broader earlier check can be used here, someone knowing >>>>> VVC should look at that. Its not immedeatly obvious from the spec >>>>> by looking for numlayerolss >>>> >>>> Can you check if the following fixes it? >>>> >>>>> diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c >>>>> index 549d021211..40572dadb5 100644 >>>>> --- a/libavcodec/cbs_h266_syntax_template.c >>>>> +++ b/libavcodec/cbs_h266_syntax_template.c >>>>> @@ -793,6 +793,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, >>>>> { >>>>> //calc NumMultiLayerOlss >>>>> int m; >>>>> + int num_layers_in_ols = 0; >>>>> uint8_t dependency_flag[VVC_MAX_LAYERS][VVC_MAX_LAYERS]; >>>>> uint16_t num_output_layers_in_ols[VVC_MAX_TOTAL_NUM_OLSS]; >>>>> uint8_t num_sub_layers_in_layer_in_ols[VVC_MAX_TOTAL_NUM_OLSS][VVC_MAX_TOTAL_NUM_OLSS]; >>>>> @@ -895,7 +896,6 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, >>>>> return AVERROR_INVALIDDATA; >>>>> } >>>>> for (i = 1; i < total_num_olss; i++) { >>>>> - int num_layers_in_ols = 0; >>>>> if (current->vps_each_layer_is_an_ols_flag) { >>>>> num_layers_in_ols = 1; >>>>> } else if (current->vps_ols_mode_idc == 0 || >>>> >>>> num_layers_in_ols is not meant to be reset on every loop. >>> >>> replacing my patch by yours does not change >>> num_multi_layer_olss from being 0 >>> and if its 0 then "num_multi_layer_olss - 1" causes problems as a limit >>> >>> more precissely this: >>> i can also send you the file if you want? >> >> No, this should be looked at by someone more familiar with VVC. > > ive already sent the fuzzer samples to nuomi and frank plowman > > >> And my patch should be applied either way. The current code is wrong. > > I did not suggest not to do that :) Well, turns out the current code is fine and my suggested change above is wrong. Fun how that goes. Can you test the following instead? > diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c > index 549d021211..30b4ae3bc0 100644 > --- a/libavcodec/cbs_h266_syntax_template.c > +++ b/libavcodec/cbs_h266_syntax_template.c > @@ -764,7 +764,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > infer(vps_each_layer_is_an_ols_flag, 0); > if (!current->vps_each_layer_is_an_ols_flag) { > if (!current->vps_all_independent_layers_flag) > - ub(2, vps_ols_mode_idc); > + u(2, vps_ols_mode_idc, 0, 2); > else > infer(vps_ols_mode_idc, 2); > if (current->vps_ols_mode_idc == 2) { > @@ -902,11 +902,10 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, > current->vps_ols_mode_idc == 1) { > num_layers_in_ols = i + 1; > } else if (current->vps_ols_mode_idc == 2) { > - for (k = 0, j = 0; k <= current->vps_max_layers_minus1; k++) { > + for (k = 0, j = 0; k <= current->vps_max_layers_minus1; k++) > if (layer_included_in_ols_flag[i][k]) > j++; > - num_layers_in_ols = j; > - } > + num_layers_in_ols = j; > } > if (num_layers_in_ols > 1) { > num_multi_layer_olss++;
On 29/01/2024 19:04, James Almer wrote: > > Well, turns out the current code is fine and my suggested change above > is wrong. Fun how that goes. > > Can you test the following instead? > >> diff --git a/libavcodec/cbs_h266_syntax_template.c >> b/libavcodec/cbs_h266_syntax_template.c >> index 549d021211..30b4ae3bc0 100644 >> --- a/libavcodec/cbs_h266_syntax_template.c >> +++ b/libavcodec/cbs_h266_syntax_template.c >> @@ -764,7 +764,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, >> RWContext *rw, >> infer(vps_each_layer_is_an_ols_flag, 0); >> if (!current->vps_each_layer_is_an_ols_flag) { >> if (!current->vps_all_independent_layers_flag) >> - ub(2, vps_ols_mode_idc); >> + u(2, vps_ols_mode_idc, 0, 2); >> else >> infer(vps_ols_mode_idc, 2); >> if (current->vps_ols_mode_idc == 2) { The spec reads "Decoders conforming to this version of this Specification shall *ignore* the OLSs with vps_ols_mode_idc equal to 3." This change throws an error for these OLSs, which I don't think is correct. There is already some logic just below this to warn the user if vps_ols_mode_idc is 3. >> @@ -902,11 +902,10 @@ static int FUNC(vps) (CodedBitstreamContext >> *ctx, RWContext *rw, >> current->vps_ols_mode_idc == 1) { >> num_layers_in_ols = i + 1; >> } else if (current->vps_ols_mode_idc == 2) { >> - for (k = 0, j = 0; k <= >> current->vps_max_layers_minus1; k++) { >> + for (k = 0, j = 0; k <= >> current->vps_max_layers_minus1; k++) >> if (layer_included_in_ols_flag[i][k]) >> j++; >> - num_layers_in_ols = j; >> - } >> + num_layers_in_ols = j; >> } >> if (num_layers_in_ols > 1) { >> num_multi_layer_olss++; This looks good to me, the old behaviour was wrong. I don't think this is what was causing this particular crash however. Below is a patch which addresses the issue, an integer overflow when calculating the bounds for vps_num_ols_timing_hrd_params_minus1. There's also a similar fix for vps_num_dpb_params_minus1. diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c index 549d021211..49bf2e45ac 100644 --- a/libavcodec/cbs_h266_syntax_template.c +++ b/libavcodec/cbs_h266_syntax_template.c @@ -946,7 +946,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, if (!current->vps_each_layer_is_an_ols_flag) { uint16_t vps_num_dpb_params; - ue(vps_num_dpb_params_minus1, 0, num_multi_layer_olss - 1); + ue(vps_num_dpb_params_minus1, 0, + num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0); if (current->vps_each_layer_is_an_ols_flag) vps_num_dpb_params = 0; else @@ -991,7 +992,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, else infer(vps_sublayer_cpb_params_present_flag, 0); ue(vps_num_ols_timing_hrd_params_minus1, 0, - num_multi_layer_olss - 1); + num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0); for (i = 0; i <= current->vps_num_ols_timing_hrd_params_minus1; i++) { uint8_t first_sublayer; if (!current->vps_default_ptl_dpb_hrd_max_tid_flag)
On 1/29/2024 5:19 PM, Frank Plowman wrote: > On 29/01/2024 19:04, James Almer wrote: > >> >> Well, turns out the current code is fine and my suggested change above >> is wrong. Fun how that goes. >> >> Can you test the following instead? >> >>> diff --git a/libavcodec/cbs_h266_syntax_template.c >>> b/libavcodec/cbs_h266_syntax_template.c >>> index 549d021211..30b4ae3bc0 100644 >>> --- a/libavcodec/cbs_h266_syntax_template.c >>> +++ b/libavcodec/cbs_h266_syntax_template.c >>> @@ -764,7 +764,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, >>> RWContext *rw, >>> infer(vps_each_layer_is_an_ols_flag, 0); >>> if (!current->vps_each_layer_is_an_ols_flag) { >>> if (!current->vps_all_independent_layers_flag) >>> - ub(2, vps_ols_mode_idc); >>> + u(2, vps_ols_mode_idc, 0, 2); >>> else >>> infer(vps_ols_mode_idc, 2); >>> if (current->vps_ols_mode_idc == 2) { > The spec reads "Decoders conforming to this version of this > Specification shall *ignore* the OLSs with > vps_ols_mode_idc equal to 3." This change throws an error for these > OLSs, which I don't think is correct. > There is already some logic just below this to warn the user if > vps_ols_mode_idc is 3. >>> @@ -902,11 +902,10 @@ static int FUNC(vps) (CodedBitstreamContext >>> *ctx, RWContext *rw, >>> current->vps_ols_mode_idc == 1) { >>> num_layers_in_ols = i + 1; >>> } else if (current->vps_ols_mode_idc == 2) { >>> - for (k = 0, j = 0; k <= >>> current->vps_max_layers_minus1; k++) { >>> + for (k = 0, j = 0; k <= >>> current->vps_max_layers_minus1; k++) >>> if (layer_included_in_ols_flag[i][k]) >>> j++; >>> - num_layers_in_ols = j; >>> - } >>> + num_layers_in_ols = j; >>> } >>> if (num_layers_in_ols > 1) { >>> num_multi_layer_olss++; > > This looks good to me, the old behaviour was wrong. I don't think this > is what was causing this > particular crash however. Will apply this part then. > > Below is a patch which addresses the issue, an integer overflow when > calculating the bounds for > vps_num_ols_timing_hrd_params_minus1. There's also a similar fix for > vps_num_dpb_params_minus1. > > diff --git a/libavcodec/cbs_h266_syntax_template.c > b/libavcodec/cbs_h266_syntax_template.c > index 549d021211..49bf2e45ac 100644 > --- a/libavcodec/cbs_h266_syntax_template.c > +++ b/libavcodec/cbs_h266_syntax_template.c > @@ -946,7 +946,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, > RWContext *rw, > > if (!current->vps_each_layer_is_an_ols_flag) { > uint16_t vps_num_dpb_params; > - ue(vps_num_dpb_params_minus1, 0, num_multi_layer_olss - 1); > + ue(vps_num_dpb_params_minus1, 0, > + num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0); FFMAX(0, num_multi_layer_olss - 1); looks better. If the spec explicitly states num_multi_layer_olss - 1 should be used here, wouldn't that mean that it doesn't expect num_multi_layer_olss to be 0 at this point for vps_ols_mode_idc >= 0 && vps_ols_mode_idc < 3? When vps_each_layer_is_an_ols_flag is true, num_multi_layer_olss is inferred as 1, so I'd expect it to also be at least 1 for vps_each_layer_is_an_ols_flag == false. > if (current->vps_each_layer_is_an_ols_flag) > vps_num_dpb_params = 0; > else > @@ -991,7 +992,7 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, > RWContext *rw, > else > infer(vps_sublayer_cpb_params_present_flag, 0); > ue(vps_num_ols_timing_hrd_params_minus1, 0, > - num_multi_layer_olss - 1); > + num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0); > for (i = 0; i <= > current->vps_num_ols_timing_hrd_params_minus1; i++) { > uint8_t first_sublayer; > if (!current->vps_default_ptl_dpb_hrd_max_tid_flag) > > _______________________________________________ > 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".
On 29/01/2024 21:13, James Almer wrote: > On 1/29/2024 5:19 PM, Frank Plowman wrote: >> Below is a patch which addresses the issue, an integer overflow when >> calculating the bounds for >> vps_num_ols_timing_hrd_params_minus1. There's also a similar fix for >> vps_num_dpb_params_minus1. >> >> diff --git a/libavcodec/cbs_h266_syntax_template.c >> b/libavcodec/cbs_h266_syntax_template.c >> index 549d021211..49bf2e45ac 100644 >> --- a/libavcodec/cbs_h266_syntax_template.c >> +++ b/libavcodec/cbs_h266_syntax_template.c >> @@ -946,7 +946,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, >> RWContext *rw, >> >> if (!current->vps_each_layer_is_an_ols_flag) { >> uint16_t vps_num_dpb_params; >> - ue(vps_num_dpb_params_minus1, 0, num_multi_layer_olss - 1); >> + ue(vps_num_dpb_params_minus1, 0, >> + num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0); > > FFMAX(0, num_multi_layer_olss - 1); looks better. > > If the spec explicitly states num_multi_layer_olss - 1 should be used > here, wouldn't that mean that it doesn't expect num_multi_layer_olss > to be 0 at this point for vps_ols_mode_idc >= 0 && vps_ols_mode_idc < 3? > When vps_each_layer_is_an_ols_flag is true, num_multi_layer_olss is > inferred as 1 num_multi_layer_olss is 0 if vps_each_layer_is_an_ols_flag is true, it is num_layers_in_ols which must be 1. > so I'd expect it to also be at least 1 for > vps_each_layer_is_an_ols_flag == false. Yes I think you're right. The spec isn't especially clear, but the description of the vps_each_layer_is_an_ols_flag leads me to believe we are safe to assert that. Patch which does so below. diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c index 549d021211..5ae3fb9f08 100644 --- a/libavcodec/cbs_h266_syntax_template.c +++ b/libavcodec/cbs_h266_syntax_template.c @@ -912,6 +912,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, num_multi_layer_olss++; } } + if (!current->vps_each_layer_is_an_ols_flag && num_multi_layer_olss == 0) + return AVERROR_INVALIDDATA; } for (i = 0; i <= current->vps_num_ptls_minus1; i++) {
On 1/29/2024 7:28 PM, Frank Plowman wrote: > > On 29/01/2024 21:13, James Almer wrote: >> On 1/29/2024 5:19 PM, Frank Plowman wrote: >>> Below is a patch which addresses the issue, an integer overflow when >>> calculating the bounds for >>> vps_num_ols_timing_hrd_params_minus1. There's also a similar fix for >>> vps_num_dpb_params_minus1. >>> >>> diff --git a/libavcodec/cbs_h266_syntax_template.c >>> b/libavcodec/cbs_h266_syntax_template.c >>> index 549d021211..49bf2e45ac 100644 >>> --- a/libavcodec/cbs_h266_syntax_template.c >>> +++ b/libavcodec/cbs_h266_syntax_template.c >>> @@ -946,7 +946,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, >>> RWContext *rw, >>> >>> if (!current->vps_each_layer_is_an_ols_flag) { >>> uint16_t vps_num_dpb_params; >>> - ue(vps_num_dpb_params_minus1, 0, num_multi_layer_olss - 1); >>> + ue(vps_num_dpb_params_minus1, 0, >>> + num_multi_layer_olss > 0 ? num_multi_layer_olss - 1 : 0); >> >> FFMAX(0, num_multi_layer_olss - 1); looks better. >> >> If the spec explicitly states num_multi_layer_olss - 1 should be used >> here, wouldn't that mean that it doesn't expect num_multi_layer_olss >> to be 0 at this point for vps_ols_mode_idc >= 0 && vps_ols_mode_idc < 3? >> When vps_each_layer_is_an_ols_flag is true, num_multi_layer_olss is >> inferred as 1 > > num_multi_layer_olss is 0 if vps_each_layer_is_an_ols_flag is true, it > is num_layers_in_ols which must be 1. You're right, i read it wrong. These names are all too similar :p > >> so I'd expect it to also be at least 1 for >> vps_each_layer_is_an_ols_flag == false. > > Yes I think you're right. The spec isn't especially clear, but the > description of the vps_each_layer_is_an_ols_flag leads me to believe we > are safe to assert that. Patch which does so below. > > diff --git a/libavcodec/cbs_h266_syntax_template.c > b/libavcodec/cbs_h266_syntax_template.c > index 549d021211..5ae3fb9f08 100644 > --- a/libavcodec/cbs_h266_syntax_template.c > +++ b/libavcodec/cbs_h266_syntax_template.c > @@ -912,6 +912,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, > RWContext *rw, > num_multi_layer_olss++; > } > } > + if (!current->vps_each_layer_is_an_ols_flag && > num_multi_layer_olss == 0) > + return AVERROR_INVALIDDATA; > } LGTM, can you send a patch for it?
diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c index 549d0212119..9e479c9c314 100644 --- a/libavcodec/cbs_h266_syntax_template.c +++ b/libavcodec/cbs_h266_syntax_template.c @@ -946,6 +946,8 @@ static int FUNC(vps) (CodedBitstreamContext *ctx, RWContext *rw, if (!current->vps_each_layer_is_an_ols_flag) { uint16_t vps_num_dpb_params; + if (!num_multi_layer_olss) + return AVERROR_INVALIDDATA; ue(vps_num_dpb_params_minus1, 0, num_multi_layer_olss - 1); if (current->vps_each_layer_is_an_ols_flag) vps_num_dpb_params = 0;
It is not possible to encode a index into an empty list. Thus this must be invalid at this point or before. Its likely a broader earlier check can be used here, someone knowing VVC should look at that. Its not immedeatly obvious from the spec by looking for numlayerolss Fixes: out of array access Fixes: 65160/clusterfuzz-testcase-minimized-ffmpeg_BSF_VVC_METADATA_fuzzer-4665241535119360 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/cbs_h266_syntax_template.c | 2 ++ 1 file changed, 2 insertions(+)