Re: [FFmpeg-devel] [PATCH] cbs_av1: Reject thirty-two zero bits in uvlc code

[Michael Niedermayer]

On Tue, Dec 26, 2023 at 12:50:42AM +0100, Michael Niedermayer wrote:
> Hi
> 
> On Sun, Oct 22, 2023 at 07:35:52PM +0100, Mark Thompson wrote:
> > The spec allows at least thirty-two zero bits followed by a one to mean
> > 2^32-1, with no constraint on the number of zeroes.  The libaom
> > reference decoder does not match this, instead reading thirty-two zeroes
> > but not the following one to mean 2^32-1.  These two interpretations are
> > incompatible and other implementations may follow one or the other.
> > Therefore reject thirty-two zeroes because the intended behaviour is not
> > clear.
> > ---
> > libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of 
> > stopping at thirty-two zeroes and not reading the one.  gav1 just rejects 
> > thirty-two zeroes.
> > 
> > This is also a source of arbitrarily large single syntax elements to hit 
> > .
> > 
> >  libavcodec/cbs_av1.c | 18 +-
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> > index 1d9ac5ab44..13c749a25b 100644
> > --- a/libavcodec/cbs_av1.c
> > +++ b/libavcodec/cbs_av1.c
> > @@ -36,7 +36,7 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, 
> > GetBitContext *gbc,
> >  CBS_TRACE_READ_START();
> > 
> >  zeroes = 0;
> > -while (1) {
> > +while (zeroes < 32) {
> 
> what happened with this patch ?
> the git master code still aborts

timeout (many times in fact)

will apply

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] cbs_av1: Reject thirty-two zero bits in uvlc code

[Michael Niedermayer]

Hi

On Sun, Oct 22, 2023 at 07:35:52PM +0100, Mark Thompson wrote:
> The spec allows at least thirty-two zero bits followed by a one to mean
> 2^32-1, with no constraint on the number of zeroes.  The libaom
> reference decoder does not match this, instead reading thirty-two zeroes
> but not the following one to mean 2^32-1.  These two interpretations are
> incompatible and other implementations may follow one or the other.
> Therefore reject thirty-two zeroes because the intended behaviour is not
> clear.
> ---
> libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping 
> at thirty-two zeroes and not reading the one.  gav1 just rejects thirty-two 
> zeroes.
> 
> This is also a source of arbitrarily large single syntax elements to hit 
> .
> 
>  libavcodec/cbs_av1.c | 18 +-
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index 1d9ac5ab44..13c749a25b 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -36,7 +36,7 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, 
> GetBitContext *gbc,
>  CBS_TRACE_READ_START();
> 
>  zeroes = 0;
> -while (1) {
> +while (zeroes < 32) {

what happened with this patch ?
the git master code still aborts

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] cbs_av1: Reject thirty-two zero bits in uvlc code

[Mark Thompson]

On 25/10/2023 21:55, Michael Niedermayer wrote:

On Sun, Oct 22, 2023 at 07:35:52PM +0100, Mark Thompson wrote:

The spec allows at least thirty-two zero bits followed by a one to mean
2^32-1, with no constraint on the number of zeroes.  The libaom
reference decoder does not match this, instead reading thirty-two zeroes
but not the following one to mean 2^32-1.  These two interpretations are
incompatible and other implementations may follow one or the other.
Therefore reject thirty-two zeroes because the intended behaviour is not
clear.
---
libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping 
at thirty-two zeroes and not reading the one.  gav1 just rejects thirty-two 
zeroes.


I would suggest to contact the authors of the spec to bring this discrepancy
to their attention, unless this is already known and
unless this sequence is declared invalid by some other part of the spec
(which would make the discrepancy only occur in invalid sequences)



Since this only occurs in nonconforming streams, fixing the spec seems like the 
better option.

Thanks,

- Mark
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] cbs_av1: Reject thirty-two zero bits in uvlc code

[Michael Niedermayer]

On Sun, Oct 22, 2023 at 07:35:52PM +0100, Mark Thompson wrote:
> The spec allows at least thirty-two zero bits followed by a one to mean
> 2^32-1, with no constraint on the number of zeroes.  The libaom
> reference decoder does not match this, instead reading thirty-two zeroes
> but not the following one to mean 2^32-1.  These two interpretations are
> incompatible and other implementations may follow one or the other.
> Therefore reject thirty-two zeroes because the intended behaviour is not
> clear.
> ---
> libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping 
> at thirty-two zeroes and not reading the one.  gav1 just rejects thirty-two 
> zeroes.

I would suggest to contact the authors of the spec to bring this discrepancy
to their attention, unless this is already known and
unless this sequence is declared invalid by some other part of the spec
(which would make the discrepancy only occur in invalid sequences)

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are best at talking, realize last or never when they are wrong.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] cbs_av1: Reject thirty-two zero bits in uvlc code

[Mark Thompson]

The spec allows at least thirty-two zero bits followed by a one to mean
2^32-1, with no constraint on the number of zeroes.  The libaom
reference decoder does not match this, instead reading thirty-two zeroes
but not the following one to mean 2^32-1.  These two interpretations are
incompatible and other implementations may follow one or the other.
Therefore reject thirty-two zeroes because the intended behaviour is not
clear.
---
libaom, dav1d and SVT-AV1 all have the same nonstandard behaviour of stopping 
at thirty-two zeroes and not reading the one.  gav1 just rejects thirty-two 
zeroes.

This is also a source of arbitrarily large single syntax elements to hit 
.

 libavcodec/cbs_av1.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
index 1d9ac5ab44..13c749a25b 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -36,7 +36,7 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, 
GetBitContext *gbc,
 CBS_TRACE_READ_START();

 zeroes = 0;
-while (1) {
+while (zeroes < 32) {
 if (get_bits_left(gbc) < 1) {
 av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid uvlc code at "
"%s: bitstream ended.\n", name);
@@ -49,10 +49,18 @@ static int cbs_av1_read_uvlc(CodedBitstreamContext *ctx, 
GetBitContext *gbc,
 }

 if (zeroes >= 32) {
-// Note that the spec allows an arbitrarily large number of
-// zero bits followed by a one bit in this case, but the
-// libaom implementation does not support it.
-value = MAX_UINT_BITS(32);
+// The spec allows at least thirty-two zero bits followed by a
+// one to mean 2^32-1, with no constraint on the number of
+// zeroes.  The libaom reference decoder does not match this,
+// instead reading thirty-two zeroes but not the following one
+// to mean 2^32-1.  These two interpretations are incompatible
+// and other implementations may follow one or the other.
+// Therefore we reject thirty-two zeroes because the intended
+// behaviour is not clear.
+av_log(ctx->log_ctx, AV_LOG_ERROR, "Thirty-two zero bits in "
+   "%s uvlc code: considered invalid due to conflicting "
+   "standard and reference decoder behaviour.\n", name);
+return AVERROR_INVALIDDATA;
 } else {
 if (get_bits_left(gbc) < zeroes) {
 av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid uvlc code at "
--
2.39.2
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".