diff mbox series

[FFmpeg-devel,3/3] avcodec/cbs_av1: abort when the written inferred value is not the expected one

Message ID 20200611161320.5136-2-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/cbs_h2645: keep separate parameter set lists for reading and writing
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer June 11, 2020, 4:13 p.m. UTC
If this happens, it's a sign of parsing issues earlier in the process, or
misuse by the calling module.

Prevents creating invalid bitstreams.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/cbs_av1.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos June 14, 2020, 2:11 p.m. UTC | #1
Am Do., 11. Juni 2020 um 18:14 Uhr schrieb James Almer <jamrial@gmail.com>:
>
> If this happens, it's a sign of parsing issues earlier in the process, or
> misuse by the calling module.
>
> Prevents creating invalid bitstreams.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/cbs_av1.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index fc228086c2..456bd9b1d5 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -711,10 +711,11 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>
>  #define infer(name, value) do { \
>          if (current->name != (value)) { \
> -            av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
> +            av_log(ctx->log_ctx, AV_LOG_ERROR, \
>                     "%s does not match inferred value: " \
>                     "%"PRId64", but should be %"PRId64".\n", \
>                     #name, (int64_t)current->name, (int64_t)(value)); \

> +            return AVERROR_BUG; \

For this return value, please explain in the commit message
why this can never happen and in the mail why you are not
using an assert().

Carl Eugen
James Almer June 14, 2020, 2:17 p.m. UTC | #2
On 6/14/2020 11:11 AM, Carl Eugen Hoyos wrote:
> Am Do., 11. Juni 2020 um 18:14 Uhr schrieb James Almer <jamrial@gmail.com>:
>>
>> If this happens, it's a sign of parsing issues earlier in the process, or
>> misuse by the calling module.
>>
>> Prevents creating invalid bitstreams.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/cbs_av1.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
>> index fc228086c2..456bd9b1d5 100644
>> --- a/libavcodec/cbs_av1.c
>> +++ b/libavcodec/cbs_av1.c
>> @@ -711,10 +711,11 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>
>>  #define infer(name, value) do { \
>>          if (current->name != (value)) { \
>> -            av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
>> +            av_log(ctx->log_ctx, AV_LOG_ERROR, \
>>                     "%s does not match inferred value: " \
>>                     "%"PRId64", but should be %"PRId64".\n", \
>>                     #name, (int64_t)current->name, (int64_t)(value)); \
> 
>> +            return AVERROR_BUG; \
> 
> For this return value, please explain in the commit message
> why this can never happen and in the mail why you are not
> using an assert().

I could, i suggested it. But nobody commented if they preferred it or
not, so i went with a normal error code.
I could also just use the invalid data code, same as the other cases
here. I'm fine with any of those options.
Michael Niedermayer June 14, 2020, 4:24 p.m. UTC | #3
On Thu, Jun 11, 2020 at 01:13:20PM -0300, James Almer wrote:
> If this happens, it's a sign of parsing issues earlier in the process, or
> misuse by the calling module.
> 
> Prevents creating invalid bitstreams.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/cbs_av1.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Iam ok with this but there are other people who know this code
better than i do.

Also long term, it may be interresting to split the check between
inferences that refer to security vs. ones refering to compliance
There may be situations where passing non compliant values through
is desirable

thx

[...]
James Almer June 14, 2020, 8:23 p.m. UTC | #4
On 6/14/2020 1:24 PM, Michael Niedermayer wrote:
> On Thu, Jun 11, 2020 at 01:13:20PM -0300, James Almer wrote:
>> If this happens, it's a sign of parsing issues earlier in the process, or
>> misuse by the calling module.
>>
>> Prevents creating invalid bitstreams.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavcodec/cbs_av1.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Iam ok with this but there are other people who know this code
> better than i do.

Pushed and backported these two patches, but postponed the split
reference one. I want Mark's opinion on the approach before applying it.

Thanks.

> 
> Also long term, it may be interresting to split the check between
> inferences that refer to security vs. ones refering to compliance
> There may be situations where passing non compliant values through
> is desirable
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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_av1.c b/libavcodec/cbs_av1.c
index fc228086c2..456bd9b1d5 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -711,10 +711,11 @@  static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
 
 #define infer(name, value) do { \
         if (current->name != (value)) { \
-            av_log(ctx->log_ctx, AV_LOG_WARNING, "Warning: " \
+            av_log(ctx->log_ctx, AV_LOG_ERROR, \
                    "%s does not match inferred value: " \
                    "%"PRId64", but should be %"PRId64".\n", \
                    #name, (int64_t)current->name, (int64_t)(value)); \
+            return AVERROR_BUG; \
         } \
     } while (0)