| Message ID | 20201006001714.19577-4-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/apedec: fix decoding insane files with recent versions | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Quoting Paul B Mahol (2020-10-06 02:17:14) > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavcodec/apedec.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > index 8fe7b5ee86..ea36247eb9 100644 > --- a/libavcodec/apedec.c > +++ b/libavcodec/apedec.c > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > base = range_decode_culfreq(ctx, pivot); > range_decode_update(ctx, 1, base); > } else { > - int base_hi = pivot, base_lo; > - int bbits = 0; > + int base_hi, base_lo; > + int bbits = 16 - ff_clz(pivot); This assumes unsigned is always 32bit, no?
On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > Quoting Paul B Mahol (2020-10-06 02:17:14) > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > --- > > libavcodec/apedec.c | 10 +++------- > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > index 8fe7b5ee86..ea36247eb9 100644 > > --- a/libavcodec/apedec.c > > +++ b/libavcodec/apedec.c > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > > base = range_decode_culfreq(ctx, pivot); > > range_decode_update(ctx, 1, base); > > } else { > > - int base_hi = pivot, base_lo; > > - int bbits = 0; > > + int base_hi, base_lo; > > + int bbits = 16 - ff_clz(pivot); > > This assumes unsigned is always 32bit, no? ksum is 32 bit, from which pivot is derived. Should I use explicitly uint32_t type instead? Where is unsigned not 32bit? > > -- > Anton Khirnov > _______________________________________________ > 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".
Quoting Paul B Mahol (2020-10-06 10:19:13) > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > --- > > > libavcodec/apedec.c | 10 +++------- > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > index 8fe7b5ee86..ea36247eb9 100644 > > > --- a/libavcodec/apedec.c > > > +++ b/libavcodec/apedec.c > > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > base = range_decode_culfreq(ctx, pivot); > > > range_decode_update(ctx, 1, base); > > > } else { > > > - int base_hi = pivot, base_lo; > > > - int bbits = 0; > > > + int base_hi, base_lo; > > > + int bbits = 16 - ff_clz(pivot); > > > > This assumes unsigned is always 32bit, no? > > ksum is 32 bit, from which pivot is derived. > > Should I use explicitly uint32_t type instead? > Where is unsigned not 32bit? I don't think uint32_t would help, as ff_clz() can expand to a compiler builtin. What I'm concerned about is the unstated assumption about sizeof(int/unsigned) == 4 spreading through the codebase. It's already present in plenty of places, so I certainly don't intend to block your patch over this. But we should consider explitly documenting this, and maybe testing in configure.
On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > Quoting Paul B Mahol (2020-10-06 10:19:13) > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > --- > > > > libavcodec/apedec.c | 10 +++------- > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > --- a/libavcodec/apedec.c > > > > +++ b/libavcodec/apedec.c > > > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > base = range_decode_culfreq(ctx, pivot); > > > > range_decode_update(ctx, 1, base); > > > > } else { > > > > - int base_hi = pivot, base_lo; > > > > - int bbits = 0; > > > > + int base_hi, base_lo; > > > > + int bbits = 16 - ff_clz(pivot); > > > > > > This assumes unsigned is always 32bit, no? > > > > ksum is 32 bit, from which pivot is derived. > > > > Should I use explicitly uint32_t type instead? > > Where is unsigned not 32bit? > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > builtin. > > What I'm concerned about is the unstated assumption about > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > present in plenty of places, so I certainly don't intend to block your > patch over this. But we should consider explitly documenting this, and > maybe testing in configure. At least in code i wrote and write i consider it a bug if it would assume sizeof(int/unsigned) == 4 We also could add a ILP64 fate target, that way we would better understand how much really assumes 32bit int thx [...]
On Wed, Oct 07, 2020 at 04:45:56PM +0200, Michael Niedermayer wrote: > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > > --- > > > > > libavcodec/apedec.c | 10 +++------- > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > --- a/libavcodec/apedec.c > > > > > +++ b/libavcodec/apedec.c > > > > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > range_decode_update(ctx, 1, base); > > > > > } else { > > > > > - int base_hi = pivot, base_lo; > > > > > - int bbits = 0; > > > > > + int base_hi, base_lo; > > > > > + int bbits = 16 - ff_clz(pivot); > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > Should I use explicitly uint32_t type instead? > > > Where is unsigned not 32bit? > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > builtin. > > > > What I'm concerned about is the unstated assumption about > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > present in plenty of places, so I certainly don't intend to block your > > patch over this. But we should consider explitly documenting this, and > > maybe testing in configure. > > At least in code i wrote and write i consider it a bug if it would > assume sizeof(int/unsigned) == 4 > > We also could add a ILP64 fate target, that way we would better understand > how much really assumes 32bit int So you basically saying we should not use ff_clz() at all because of this. Then we should remove it from our code. > > thx > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > In a rich man's house there is no place to spit but his face. > -- Diogenes of Sinope > _______________________________________________ > 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".
Quoting Michael Niedermayer (2020-10-07 16:45:56) > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > > --- > > > > > libavcodec/apedec.c | 10 +++------- > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > --- a/libavcodec/apedec.c > > > > > +++ b/libavcodec/apedec.c > > > > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > range_decode_update(ctx, 1, base); > > > > > } else { > > > > > - int base_hi = pivot, base_lo; > > > > > - int bbits = 0; > > > > > + int base_hi, base_lo; > > > > > + int bbits = 16 - ff_clz(pivot); > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > Should I use explicitly uint32_t type instead? > > > Where is unsigned not 32bit? > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > builtin. > > > > What I'm concerned about is the unstated assumption about > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > present in plenty of places, so I certainly don't intend to block your > > patch over this. But we should consider explitly documenting this, and > > maybe testing in configure. > > At least in code i wrote and write i consider it a bug if it would > assume sizeof(int/unsigned) == 4 It is my impression (not sure how justified) that there are many overflow checks that cast a multiplication of two ints to int64, thus assuming that int64 is strictly larger. E.g. 6b7bcd437e5 > > We also could add a ILP64 fate target, that way we would better understand > how much really assumes 32bit int I suppose that would be interesting.
On Wed, Oct 07, 2020 at 05:12:47PM +0200, Paul B Mahol wrote: > On Wed, Oct 07, 2020 at 04:45:56PM +0200, Michael Niedermayer wrote: > > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > > > --- > > > > > > libavcodec/apedec.c | 10 +++------- > > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > > --- a/libavcodec/apedec.c > > > > > > +++ b/libavcodec/apedec.c > > > > > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > > range_decode_update(ctx, 1, base); > > > > > > } else { > > > > > > - int base_hi = pivot, base_lo; > > > > > > - int bbits = 0; > > > > > > + int base_hi, base_lo; > > > > > > + int bbits = 16 - ff_clz(pivot); > > > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > > > Should I use explicitly uint32_t type instead? > > > > Where is unsigned not 32bit? > > > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > > builtin. > > > > > > What I'm concerned about is the unstated assumption about > > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > > present in plenty of places, so I certainly don't intend to block your > > > patch over this. But we should consider explitly documenting this, and > > > maybe testing in configure. > > > > At least in code i wrote and write i consider it a bug if it would > > assume sizeof(int/unsigned) == 4 > > > > We also could add a ILP64 fate target, that way we would better understand > > how much really assumes 32bit int > > So you basically saying we should not use ff_clz() at all because of this. > Then we should remove it from our code. i guess thats the conclusion yes or the hardcoded 32 that is used by code calling it would need to be changed thx [...]
On Thu, Oct 08, 2020 at 09:33:35PM +0200, Michael Niedermayer wrote: > On Wed, Oct 07, 2020 at 05:12:47PM +0200, Paul B Mahol wrote: > > On Wed, Oct 07, 2020 at 04:45:56PM +0200, Michael Niedermayer wrote: > > > On Tue, Oct 06, 2020 at 12:08:27PM +0200, Anton Khirnov wrote: > > > > Quoting Paul B Mahol (2020-10-06 10:19:13) > > > > > On Tue, Oct 06, 2020 at 10:08:58AM +0200, Anton Khirnov wrote: > > > > > > Quoting Paul B Mahol (2020-10-06 02:17:14) > > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > > > > > --- > > > > > > > libavcodec/apedec.c | 10 +++------- > > > > > > > 1 file changed, 3 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c > > > > > > > index 8fe7b5ee86..ea36247eb9 100644 > > > > > > > --- a/libavcodec/apedec.c > > > > > > > +++ b/libavcodec/apedec.c > > > > > > > @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) > > > > > > > base = range_decode_culfreq(ctx, pivot); > > > > > > > range_decode_update(ctx, 1, base); > > > > > > > } else { > > > > > > > - int base_hi = pivot, base_lo; > > > > > > > - int bbits = 0; > > > > > > > + int base_hi, base_lo; > > > > > > > + int bbits = 16 - ff_clz(pivot); > > > > > > > > > > > > This assumes unsigned is always 32bit, no? > > > > > > > > > > ksum is 32 bit, from which pivot is derived. > > > > > > > > > > Should I use explicitly uint32_t type instead? > > > > > Where is unsigned not 32bit? > > > > > > > > I don't think uint32_t would help, as ff_clz() can expand to a compiler > > > > builtin. > > > > > > > > What I'm concerned about is the unstated assumption about > > > > sizeof(int/unsigned) == 4 spreading through the codebase. It's already > > > > present in plenty of places, so I certainly don't intend to block your > > > > patch over this. But we should consider explitly documenting this, and > > > > maybe testing in configure. > > > > > > At least in code i wrote and write i consider it a bug if it would > > > assume sizeof(int/unsigned) == 4 > > > > > > We also could add a ILP64 fate target, that way we would better understand > > > how much really assumes 32bit int > > > > So you basically saying we should not use ff_clz() at all because of this. > > Then we should remove it from our code. > > i guess thats the conclusion yes or > the hardcoded 32 that is used by code calling it would need to be changed Will apply this ape patchset soon, without this last controversial patch. > > thx > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Rewriting code that is poorly written but fully understood is good. > Rewriting code that one doesnt understand is a sign that one is less smart > then the original author, trying to rewrite it will not make it better. > _______________________________________________ > 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 --git a/libavcodec/apedec.c b/libavcodec/apedec.c index 8fe7b5ee86..ea36247eb9 100644 --- a/libavcodec/apedec.c +++ b/libavcodec/apedec.c @@ -575,14 +575,10 @@ static inline int ape_decode_value_3990(APEContext *ctx, APERice *rice) base = range_decode_culfreq(ctx, pivot); range_decode_update(ctx, 1, base); } else { - int base_hi = pivot, base_lo; - int bbits = 0; + int base_hi, base_lo; + int bbits = 16 - ff_clz(pivot); - while (base_hi & ~0xFFFF) { - base_hi >>= 1; - bbits++; - } - base_hi = range_decode_culfreq(ctx, base_hi + 1); + base_hi = range_decode_culfreq(ctx, (pivot >> bbits) + 1); range_decode_update(ctx, 1, base_hi); base_lo = range_decode_culfreq(ctx, 1 << bbits); range_decode_update(ctx, 1, base_lo);
Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavcodec/apedec.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)