diff mbox series

[FFmpeg-devel] lavc/vvc: Add check to num_multi_layer_olss

Message ID 20240130094057.62515-1-post@frankplowman.com
State Accepted
Commit 36a986d9a193e39382de4bac95e2e314cc30ca7a
Headers show
Series [FFmpeg-devel] lavc/vvc: Add check to num_multi_layer_olss | expand

Checks

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

Commit Message

Frank Plowman Jan. 30, 2024, 9:40 a.m. UTC 
From: Frank Plowman <post@frankplowman.com>

Check that vps_each_layer_is_an_ols_flag, which indicates that "at
least one OLS specified by the VPS contains more than one layer," is
set if num_multi_layer_olss is non-zero.

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: Frank Plowman <post@frankplowman.com>
---
 libavcodec/cbs_h266_syntax_template.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

James Almer Jan. 30, 2024, 12:28 p.m. UTC | #1 
On 1/30/2024 6:40 AM, post@frankplowman.com wrote:
> From: Frank Plowman <post@frankplowman.com>
> 
> Check that vps_each_layer_is_an_ols_flag, which indicates that "at
> least one OLS specified by the VPS contains more than one layer," is
> set if num_multi_layer_olss is non-zero.
> 
> 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: Frank Plowman <post@frankplowman.com>
> ---
>   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 2f3478e5e1..37dc3acba0 100644
> --- a/libavcodec/cbs_h266_syntax_template.c
> +++ b/libavcodec/cbs_h266_syntax_template.c
> @@ -911,6 +911,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++) {

Applied, thanks.
Nuo Mi Jan. 30, 2024, 12:31 p.m. UTC | #2 
On Tue, Jan 30, 2024 at 5:41 PM <post@frankplowman.com> wrote:

> From: Frank Plowman <post@frankplowman.com>
>
> Check that vps_each_layer_is_an_ols_flag, which indicates that "at
> least one OLS specified by the VPS contains more than one layer," is
> set if num_multi_layer_olss is non-zero.
>
> 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
> <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
> Frank Plowman <post@frankplowman.com>
> ---
>  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 2f3478e5e1..37dc3acba0 100644
> --- a/libavcodec/cbs_h266_syntax_template.c
> +++ b/libavcodec/cbs_h266_syntax_template.c
> @@ -911,6 +911,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;
>      }
>
> The specification does not provide information on how to obtain
TotalNumOlss (total_num_olss) when ols_mode_idc is set to 3.
Therefore, the earlier line "u(8, vps_num_ptls_minus1, 0, total_num_olss -
1)" is undefined.
We'd better return a patch welcome error instead of printing a warning
before vps_num_ptls_minus1 line

     for (i = 0; i <= current->vps_num_ptls_minus1; i++) {
> --
> 2.43.0
>
> _______________________________________________
> 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".
>
Frank Plowman Jan. 30, 2024, 12:55 p.m. UTC | #3 
On 30/01/2024 12:31, Nuo Mi wrote:

> On Tue, Jan 30, 2024 at 5:41 PM<post@frankplowman.com>  wrote:
>> From: Frank Plowman<post@frankplowman.com>
>>
>> Check that vps_each_layer_is_an_ols_flag, which indicates that "at
>> least one OLS specified by the VPS contains more than one layer," is
>> set if num_multi_layer_olss is non-zero.
>>
>> 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
>> <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
>> Frank Plowman<post@frankplowman.com>
>> ---
>>   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 2f3478e5e1..37dc3acba0 100644
>> --- a/libavcodec/cbs_h266_syntax_template.c
>> +++ b/libavcodec/cbs_h266_syntax_template.c
>> @@ -911,6 +911,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;
>>       }
> The specification does not provide information on how to obtain
> TotalNumOlss (total_num_olss) when ols_mode_idc is set to 3.
> Therefore, the earlier line "u(8, vps_num_ptls_minus1, 0, total_num_olss -
> 1)" is undefined.
> We'd better return a patch welcome error instead of printing a warning
> before vps_num_ptls_minus1 line

This is the same behaviour James suggested in an earlier patch. The spec 
says "decoders conforming to this version of this Specification shall 
ignore the OLSs with vps_ols_mode_idc equal to 3." I don't think this 
should be an error as the spec is unambiguous here. Perhaps we can 
instead skip the remainder of the VPS if vps_ols_mode_idc is 3? Or is 
there some better way to ignore these OLSs?
Frank Plowman Jan. 30, 2024, 1:13 p.m. UTC | #4 
On 30/01/2024 12:55, Frank Plowman wrote:
> On 30/01/2024 12:31, Nuo Mi wrote:
> 
>> On Tue, Jan 30, 2024 at 5:41 PM<post@frankplowman.com>  wrote:
>>> From: Frank Plowman<post@frankplowman.com>
>>>
>>> Check that vps_each_layer_is_an_ols_flag, which indicates that "at
>>> least one OLS specified by the VPS contains more than one layer," is
>>> set if num_multi_layer_olss is non-zero.
>>>
>>> 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
>>> <https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by>:
>>> Frank Plowman<post@frankplowman.com>
>>> ---
>>>   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 2f3478e5e1..37dc3acba0 100644
>>> --- a/libavcodec/cbs_h266_syntax_template.c
>>> +++ b/libavcodec/cbs_h266_syntax_template.c
>>> @@ -911,6 +911,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;
>>>       }
>> The specification does not provide information on how to obtain
>> TotalNumOlss (total_num_olss) when ols_mode_idc is set to 3.
>> Therefore, the earlier line "u(8, vps_num_ptls_minus1, 0, 
>> total_num_olss -
>> 1)" is undefined.
>> We'd better return a patch welcome error instead of printing a warning
>> before vps_num_ptls_minus1 line
> 
> This is the same behaviour James suggested in an earlier patch. The spec 
> says "decoders conforming to this version of this Specification shall 
> ignore the OLSs with vps_ols_mode_idc equal to 3." I don't think this 
> should be an error as the spec is unambiguous here. Perhaps we can 
> instead skip the remainder of the VPS if vps_ols_mode_idc is 3? Or is 
> there some better way to ignore these OLSs?

For reference, VTM's behaviour is the same as the current behaviour: 
TotalNumOlss is assumed to be 0 when ols_mode_idc, hence most of the 
remaining syntax elements in the VPS are not read as they are within

for (i = 0; i < total_num_olss; i++)

loops or other loops with bounds derived from total_num_olss.  On the 
other hand, VVdeC's behaviour is the same as you suggest: it throws an 
error if total_num_olss is 3.
Nuo Mi Jan. 30, 2024, 1:16 p.m. UTC | #5 
On Tue, Jan 30, 2024 at 8:56 PM Frank Plowman <post@frankplowman.com> wrote:

> On 30/01/2024 12:31, Nuo Mi wrote:
>
> > On Tue, Jan 30, 2024 at 5:41 PM<post@frankplowman.com>  wrote:
> >> From: Frank Plowman<post@frankplowman.com>
> >>
> >> Check that vps_each_layer_is_an_ols_flag, which indicates that "at
> >> least one OLS specified by the VPS contains more than one layer," is
> >> set if num_multi_layer_olss is non-zero.
> >>
> >> 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
> >> <
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by
> >:
> >> Frank Plowman<post@frankplowman.com>
> >> ---
> >>   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 2f3478e5e1..37dc3acba0 100644
> >> --- a/libavcodec/cbs_h266_syntax_template.c
> >> +++ b/libavcodec/cbs_h266_syntax_template.c
> >> @@ -911,6 +911,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;
> >>       }
> > The specification does not provide information on how to obtain
> > TotalNumOlss (total_num_olss) when ols_mode_idc is set to 3.
> > Therefore, the earlier line "u(8, vps_num_ptls_minus1, 0, total_num_olss
> -
> > 1)" is undefined.
> > We'd better return a patch welcome error instead of printing a warning
> > before vps_num_ptls_minus1 line
>
> This is the same behaviour James suggested in an earlier patch. The spec
> says "decoders conforming to this version of this Specification shall
> ignore the OLSs with vps_ols_mode_idc equal to 3." I don't think this
> should be an error as the spec is unambiguous here. Perhaps we can
> instead skip the remainder of the VPS if vps_ols_mode_idc is 3? Or is
> there some better way to ignore these OLSs?
>
Even the specification editor is uncertain about what will be included in
the future.
Perhaps logic later than the point needs a total rewrite.
It's not an error in terms of invalid data; we simply do not know how to
handle it and are asking others to provide the patch.
Nuo Mi Jan. 30, 2024, 1:25 p.m. UTC | #6 
On Tue, Jan 30, 2024 at 9:13 PM Frank Plowman <post@frankplowman.com> wrote:

> On 30/01/2024 12:55, Frank Plowman wrote:
> > On 30/01/2024 12:31, Nuo Mi wrote:
> >
> >> On Tue, Jan 30, 2024 at 5:41 PM<post@frankplowman.com>  wrote:
> >>> From: Frank Plowman<post@frankplowman.com>
> >>>
> >>> Check that vps_each_layer_is_an_ols_flag, which indicates that "at
> >>> least one OLS specified by the VPS contains more than one layer," is
> >>> set if num_multi_layer_olss is non-zero.
> >>>
> >>> 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
> >>> <
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpegSigned-off-by
> >:
> >>> Frank Plowman<post@frankplowman.com>
> >>> ---
> >>>   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 2f3478e5e1..37dc3acba0 100644
> >>> --- a/libavcodec/cbs_h266_syntax_template.c
> >>> +++ b/libavcodec/cbs_h266_syntax_template.c
> >>> @@ -911,6 +911,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;
> >>>       }
> >> The specification does not provide information on how to obtain
> >> TotalNumOlss (total_num_olss) when ols_mode_idc is set to 3.
> >> Therefore, the earlier line "u(8, vps_num_ptls_minus1, 0,
> >> total_num_olss -
> >> 1)" is undefined.
> >> We'd better return a patch welcome error instead of printing a warning
> >> before vps_num_ptls_minus1 line
> >
> > This is the same behaviour James suggested in an earlier patch. The spec
> > says "decoders conforming to this version of this Specification shall
> > ignore the OLSs with vps_ols_mode_idc equal to 3." I don't think this
> > should be an error as the spec is unambiguous here. Perhaps we can
> > instead skip the remainder of the VPS if vps_ols_mode_idc is 3? Or is
> > there some better way to ignore these OLSs?
>
> For reference, VTM's behaviour is the same as the current behaviour:
> TotalNumOlss is assumed to be 0 when ols_mode_idc, hence most of the
> remaining syntax elements in the VPS are not read as they are within
>
But once you read the  vps_num_ptls_minus1, your behaviors are undefined.
because you do not know vps_num_ptls_minus1 should be less than
TotalNumOlss.  and TotalNumOlss is undefined for ols_mode_idc == 3.
:)

>
> for (i = 0; i < total_num_olss; i++)
>
> loops or other loops with bounds derived from total_num_olss.  On the
> other hand, VVdeC's behaviour is the same as you suggest: it throws an
> error if total_num_olss is 3.
> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/cbs_h266_syntax_template.c b/libavcodec/cbs_h266_syntax_template.c
index 2f3478e5e1..37dc3acba0 100644
--- a/libavcodec/cbs_h266_syntax_template.c
+++ b/libavcodec/cbs_h266_syntax_template.c
@@ -911,6 +911,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++) {