[PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread
Such frames will crash when pthread functions are called on the NULL pointer Fixes: member access within null pointer of type 'VVCFrameThread' (aka 'struct VVCFrameThread') Fixes: 65160/clusterfuzz-testcase-minimized-ffmpeg_BSF_VVC_METADATA_fuzzer-4665241535119360 (partly) Fixes: 65636/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VVC_fuzzer-5394745824182272 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vvc/vvcdec.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libavcodec/vvc/vvcdec.c b/libavcodec/vvc/vvcdec.c index 540a05f8cf4..2fa6aa46d9b 100644 --- a/libavcodec/vvc/vvcdec.c +++ b/libavcodec/vvc/vvcdec.c @@ -910,6 +910,9 @@ static int vvc_decode_frame(AVCodecContext *avctx, AVFrame *output, if (ret < 0) return ret; + if (!fc->ft) + return avpkt->size; + ret = submit_frame(s, fc, output, got_output); if (ret < 0) return ret; -- 2.17.1
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(+) 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; -- 2.17.1
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Those who are best at talking, realize last or never when they are wrong.
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 [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Take away the freedom of one citizen and you will be jailed, take away the freedom of all citizens and you will be congratulated by your peers in Parliament.
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?
"When aps_params_type is equal to ALF_APS or SCALING_APS, the value of aps_adaptation_parameter_set_id shall be in the range of 0 to 7, inclusive. When aps_params_type is equal to LMCS_APS, the value of aps_adaptation_parameter_set_id shall be in the range of 0 to 3, inclusive." Fixes: out of array accesses Fixes: 65932/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VVC_fuzzer-4563412340244480 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vvc/vvc_ps.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libavcodec/vvc/vvc_ps.c b/libavcodec/vvc/vvc_ps.c index c2afc0ac932..41589c138d1 100644 --- a/libavcodec/vvc/vvc_ps.c +++ b/libavcodec/vvc/vvc_ps.c @@ -991,12 +991,19 @@ int ff_vvc_decode_aps(VVCParamSets *ps, const CodedBitstreamUnit *unit) switch (aps->aps_params_type) { case APS_ALF: + if (aps->aps_adaptation_parameter_set_id >= FF_ARRAY_ELEMS(ps->alf_list)) + return AVERROR_INVALIDDATA; + ret = aps_decode_alf(&ps->alf_list[aps->aps_adaptation_parameter_set_id], aps); break; case APS_LMCS: + if (aps->aps_adaptation_parameter_set_id >= FF_ARRAY_ELEMS(ps->lmcs_list)) + return AVERROR_INVALIDDATA; ff_refstruct_replace(&ps->lmcs_list[aps->aps_adaptation_parameter_set_id], aps); break; case APS_SCALING: + if (aps->aps_adaptation_parameter_set_id >= FF_ARRAY_ELEMS(ps->scaling_list)) + return AVERROR_INVALIDDATA; ret = aps_decode_scaling(&ps->scaling_list[aps->aps_adaptation_parameter_set_id], aps); break; } -- 2.17.1
On 1/26/2024 6:46 PM, Michael Niedermayer wrote:
"When aps_params_type is equal to ALF_APS or SCALING_APS, the value of aps_adaptation_parameter_set_id shall be in the range of 0 to 7, inclusive. When aps_params_type is equal to LMCS_APS, the value of aps_adaptation_parameter_set_id shall be in the range of 0 to 3, inclusive."
This should be checked in CBS instead, and not one of its users. Something like:
diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c index 549d021211..9fd8399f0e 100644 --- a/libavcodec/cbs_h266_syntax_template.c +++ b/libavcodec/cbs_h266_syntax_template.c @@ -2455,6 +2455,7 @@ static int FUNC(scaling_list_data)(CodedBitstreamContext *ctx, RWContext *rw, static int FUNC(aps)(CodedBitstreamContext *ctx, RWContext *rw, H266RawAPS *current, int prefix) { + int aps_id_max = MAX_UINT_BITS(5); int err;
if (prefix) @@ -2467,7 +2468,12 @@ static int FUNC(aps)(CodedBitstreamContext *ctx, RWContext *rw, : VVC_SUFFIX_APS_NUT));
ub(3, aps_params_type); - ub(5, aps_adaptation_parameter_set_id); + if (current->aps_params_type == VVC_ASP_TYPE_ALF || + current->aps_params_type == VVC_ASP_TYPE_SCALING) + aps_id_max = 7; + else if (current->aps_params_type == VVC_ASP_TYPE_LMCS) + aps_id_max = 3; + u(5, aps_adaptation_parameter_set_id, 0, aps_id_max); flag(aps_chroma_present_flag); if (current->aps_params_type == VVC_ASP_TYPE_ALF) CHECK(FUNC(alf_data)(ctx, rw, current));
On Fri, Jan 26, 2024 at 10:46:49PM +0100, Michael Niedermayer wrote:
Such frames will crash when pthread functions are called on the NULL pointer
Fixes: member access within null pointer of type 'VVCFrameThread' (aka 'struct VVCFrameThread') Fixes: 65160/clusterfuzz-testcase-minimized-ffmpeg_BSF_VVC_METADATA_fuzzer-4665241535119360 (partly) Fixes: 65636/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VVC_fuzzer-5394745824182272
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vvc/vvcdec.c | 3 +++ 1 file changed, 3 insertions(+)
will apply [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time.
On Tue, Apr 2, 2024 at 1:06 AM Michael Niedermayer <michael@niedermayer.cc> wrote:
Such frames will crash when pthread functions are called on the NULL
On Fri, Jan 26, 2024 at 10:46:49PM +0100, Michael Niedermayer wrote: pointer
Fixes: member access within null pointer of type 'VVCFrameThread' (aka
'struct VVCFrameThread')
Fixes: 65160/clusterfuzz-testcase-minimized-ffmpeg_BSF_VVC_METADATA_fuzzer-4665241535119360 (partly) Fixes: 65636/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VVC_fuzzer-5394745824182272
Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/vvc/vvcdec.c | 3 +++ 1 file changed, 3 insertions(+)
will apply
sure, please
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
If you think the mosad wants you dead since a long time then you are either wrong or dead since a long time. _______________________________________________ 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".
participants (4)
-
Frank Plowman -
James Almer -
Michael Niedermayer -
Nuo Mi