Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread
@ 2024-01-26 21:46 Michael Niedermayer
  2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss Michael Niedermayer
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Michael Niedermayer @ 2024-01-26 21:46 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

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

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-26 21:46 [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
@ 2024-01-26 21:46 ` Michael Niedermayer
  2024-01-27 12:25   ` James Almer
  2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 3/3] avcodec/vvc/vvc_ps: check aps_adaptation_parameter_set_id Michael Niedermayer
  2024-04-01 17:06 ` [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
  2 siblings, 1 reply; 15+ messages in thread
From: Michael Niedermayer @ 2024-01-26 21:46 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

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

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [FFmpeg-devel] [PATCH 3/3] avcodec/vvc/vvc_ps: check aps_adaptation_parameter_set_id
  2024-01-26 21:46 [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
  2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss Michael Niedermayer
@ 2024-01-26 21:46 ` Michael Niedermayer
  2024-01-26 22:17   ` James Almer
  2024-04-01 17:06 ` [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
  2 siblings, 1 reply; 15+ messages in thread
From: Michael Niedermayer @ 2024-01-26 21:46 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

"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

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 3/3] avcodec/vvc/vvc_ps: check aps_adaptation_parameter_set_id
  2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 3/3] avcodec/vvc/vvc_ps: check aps_adaptation_parameter_set_id Michael Niedermayer
@ 2024-01-26 22:17   ` James Almer
  0 siblings, 0 replies; 15+ messages in thread
From: James Almer @ 2024-01-26 22:17 UTC (permalink / raw)
  To: ffmpeg-devel

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));
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss Michael Niedermayer
@ 2024-01-27 12:25   ` James Almer
  2024-01-27 23:56     ` Michael Niedermayer
  0 siblings, 1 reply; 15+ messages in thread
From: James Almer @ 2024-01-27 12:25 UTC (permalink / raw)
  To: ffmpeg-devel

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.
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-27 12:25   ` James Almer
@ 2024-01-27 23:56     ` Michael Niedermayer
  2024-01-28  0:02       ` James Almer
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Niedermayer @ 2024-01-27 23:56 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 5362 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-27 23:56     ` Michael Niedermayer
@ 2024-01-28  0:02       ` James Almer
  2024-01-28  0:05         ` Michael Niedermayer
  0 siblings, 1 reply; 15+ messages in thread
From: James Almer @ 2024-01-28  0:02 UTC (permalink / raw)
  To: ffmpeg-devel

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.
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-28  0:02       ` James Almer
@ 2024-01-28  0:05         ` Michael Niedermayer
  2024-01-29 19:04           ` James Almer
  0 siblings, 1 reply; 15+ messages in thread
From: Michael Niedermayer @ 2024-01-28  0:05 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 2701 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-28  0:05         ` Michael Niedermayer
@ 2024-01-29 19:04           ` James Almer
  2024-01-29 20:19             ` Frank Plowman
  0 siblings, 1 reply; 15+ messages in thread
From: James Almer @ 2024-01-29 19:04 UTC (permalink / raw)
  To: ffmpeg-devel

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++;

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-29 19:04           ` James Almer
@ 2024-01-29 20:19             ` Frank Plowman
  2024-01-29 21:13               ` James Almer
  0 siblings, 1 reply; 15+ messages in thread
From: Frank Plowman @ 2024-01-29 20:19 UTC (permalink / raw)
  To: ffmpeg-devel

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)

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-29 20:19             ` Frank Plowman
@ 2024-01-29 21:13               ` James Almer
  2024-01-29 22:28                 ` Frank Plowman
  0 siblings, 1 reply; 15+ messages in thread
From: James Almer @ 2024-01-29 21:13 UTC (permalink / raw)
  To: ffmpeg-devel

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".
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-29 21:13               ` James Almer
@ 2024-01-29 22:28                 ` Frank Plowman
  2024-01-29 23:40                   ` James Almer
  0 siblings, 1 reply; 15+ messages in thread
From: Frank Plowman @ 2024-01-29 22:28 UTC (permalink / raw)
  To: ffmpeg-devel


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++) {

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss
  2024-01-29 22:28                 ` Frank Plowman
@ 2024-01-29 23:40                   ` James Almer
  0 siblings, 0 replies; 15+ messages in thread
From: James Almer @ 2024-01-29 23:40 UTC (permalink / raw)
  To: ffmpeg-devel

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?
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread
  2024-01-26 21:46 [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
  2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss Michael Niedermayer
  2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 3/3] avcodec/vvc/vvc_ps: check aps_adaptation_parameter_set_id Michael Niedermayer
@ 2024-04-01 17:06 ` Michael Niedermayer
  2024-04-02  2:50   ` Nuo Mi
  2 siblings, 1 reply; 15+ messages in thread
From: Michael Niedermayer @ 2024-04-01 17:06 UTC (permalink / raw)
  To: FFmpeg development discussions and patches


[-- Attachment #1.1: Type: text/plain, Size: 914 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread
  2024-04-01 17:06 ` [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
@ 2024-04-02  2:50   ` Nuo Mi
  0 siblings, 0 replies; 15+ messages in thread
From: Nuo Mi @ 2024-04-02  2:50 UTC (permalink / raw)
  To: FFmpeg development discussions and patches

On Tue, Apr 2, 2024 at 1:06 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> 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
>
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".
>
_______________________________________________
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".

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2024-04-02  2:50 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-26 21:46 [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_h266_syntax_template: sanity check num_multi_layer_olss Michael Niedermayer
2024-01-27 12:25   ` James Almer
2024-01-27 23:56     ` Michael Niedermayer
2024-01-28  0:02       ` James Almer
2024-01-28  0:05         ` Michael Niedermayer
2024-01-29 19:04           ` James Almer
2024-01-29 20:19             ` Frank Plowman
2024-01-29 21:13               ` James Almer
2024-01-29 22:28                 ` Frank Plowman
2024-01-29 23:40                   ` James Almer
2024-01-26 21:46 ` [FFmpeg-devel] [PATCH 3/3] avcodec/vvc/vvc_ps: check aps_adaptation_parameter_set_id Michael Niedermayer
2024-01-26 22:17   ` James Almer
2024-04-01 17:06 ` [FFmpeg-devel] [PATCH 1/3] avcodec/vvc/vvcdec: Do not submit frames without VVCFrameThread Michael Niedermayer
2024-04-02  2:50   ` Nuo Mi

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git