Re: [FFmpeg-devel] [PATCH 2/2] avcodec/pixlet: Fixes: undefined shift in av_mod_uintp2()

[Michael Niedermayer]

On Sun, Aug 20, 2017 at 04:06:46PM +0200, Paul B Mahol wrote:
> On 8/19/17, Michael Niedermayer  wrote:
> > On Sat, Aug 19, 2017 at 04:00:18PM +0200, Paul B Mahol wrote:
> >> On 8/19/17, Michael Niedermayer  wrote:
> >> > On Fri, Aug 18, 2017 at 06:21:56PM +0200, Paul B Mahol wrote:
> >> >> On 8/18/17, Michael Niedermayer  wrote:
> >> >> > Fixes: runtime error: shift exponent 4294967289 is too large for
> >> >> > 32-bit
> >> >> > type
> >> >> > 'int'
> >> >> > Fixes: 3030/clusterfuzz-testcase-minimized-4649809254285312
> >> >> >
> >> >> > Found-by: continuous fuzzing process
> >> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> >> > Signed-off-by: Michael Niedermayer 
> >> >> > ---
> >> >> >  libavcodec/pixlet.c | 2 +-
> >> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/libavcodec/pixlet.c b/libavcodec/pixlet.c
> >> >> > index 088226bdda..a9cfe085c9 100644
> >> >> > --- a/libavcodec/pixlet.c
> >> >> > +++ b/libavcodec/pixlet.c
> >> >> > @@ -262,7 +262,7 @@ static int read_high_coeffs(AVCodecContext
> >> >> > *avctx,
> >> >> > uint8_t *src, int16_t *dst, i
> >> >> >
> >> >> >  flag = 0;
> >> >> >
> >> >> > -if (state * 4ULL > 0xFF || i >= size)
> >> >> > +if ((uint64_t)state > 0xFF / 4 || i >= size)
> >> >>
> >> >> This is not exact same behaviour.
> >> >
> >> > no, it differs for a small number of cases that have one of the 2
> >> > most significant bits (of the 64 bits) set in state.
> >> > I think this was a bug i introduced in a previous commit.
> >> >
> >> > If above change is not the correct thing to do, please elaborate.
> >> > state is passed into ff_clz() later which has no deterministic
> >> > behavior when these MSBs are set as ff_clz() uses unsigned and state
> >> > is int64_t and unsigned can be 32 or 64bit.
> >>
> >> Only 32 bits matters for ff_clz() anyway.
> >
> > your reply is a bit terse.
> > Does that mean you have no objections to the patch ?
> >
> > about ff_clz(), with 64bit unsigned a ff_clz(1) should return 63, the
> > code looks like it expects 31
> 
> yes.

will apply the 2 patches

thanks

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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates


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

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/pixlet: Fixes: undefined shift in av_mod_uintp2()

[Paul B Mahol]

On 8/19/17, Michael Niedermayer  wrote:
> On Sat, Aug 19, 2017 at 04:00:18PM +0200, Paul B Mahol wrote:
>> On 8/19/17, Michael Niedermayer  wrote:
>> > On Fri, Aug 18, 2017 at 06:21:56PM +0200, Paul B Mahol wrote:
>> >> On 8/18/17, Michael Niedermayer  wrote:
>> >> > Fixes: runtime error: shift exponent 4294967289 is too large for
>> >> > 32-bit
>> >> > type
>> >> > 'int'
>> >> > Fixes: 3030/clusterfuzz-testcase-minimized-4649809254285312
>> >> >
>> >> > Found-by: continuous fuzzing process
>> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> >> > Signed-off-by: Michael Niedermayer 
>> >> > ---
>> >> >  libavcodec/pixlet.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/libavcodec/pixlet.c b/libavcodec/pixlet.c
>> >> > index 088226bdda..a9cfe085c9 100644
>> >> > --- a/libavcodec/pixlet.c
>> >> > +++ b/libavcodec/pixlet.c
>> >> > @@ -262,7 +262,7 @@ static int read_high_coeffs(AVCodecContext
>> >> > *avctx,
>> >> > uint8_t *src, int16_t *dst, i
>> >> >
>> >> >  flag = 0;
>> >> >
>> >> > -if (state * 4ULL > 0xFF || i >= size)
>> >> > +if ((uint64_t)state > 0xFF / 4 || i >= size)
>> >>
>> >> This is not exact same behaviour.
>> >
>> > no, it differs for a small number of cases that have one of the 2
>> > most significant bits (of the 64 bits) set in state.
>> > I think this was a bug i introduced in a previous commit.
>> >
>> > If above change is not the correct thing to do, please elaborate.
>> > state is passed into ff_clz() later which has no deterministic
>> > behavior when these MSBs are set as ff_clz() uses unsigned and state
>> > is int64_t and unsigned can be 32 or 64bit.
>>
>> Only 32 bits matters for ff_clz() anyway.
>
> your reply is a bit terse.
> Does that mean you have no objections to the patch ?
>
> about ff_clz(), with 64bit unsigned a ff_clz(1) should return 63, the
> code looks like it expects 31

yes.

>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you fake or manipulate statistics in a paper in physics you will never
> get a job again.
> If you fake or manipulate statistics in a paper in medicin you will get
> a job for life at the pharma industry.
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/pixlet: Fixes: undefined shift in av_mod_uintp2()

[Michael Niedermayer]

On Sat, Aug 19, 2017 at 04:00:18PM +0200, Paul B Mahol wrote:
> On 8/19/17, Michael Niedermayer  wrote:
> > On Fri, Aug 18, 2017 at 06:21:56PM +0200, Paul B Mahol wrote:
> >> On 8/18/17, Michael Niedermayer  wrote:
> >> > Fixes: runtime error: shift exponent 4294967289 is too large for 32-bit
> >> > type
> >> > 'int'
> >> > Fixes: 3030/clusterfuzz-testcase-minimized-4649809254285312
> >> >
> >> > Found-by: continuous fuzzing process
> >> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> > Signed-off-by: Michael Niedermayer 
> >> > ---
> >> >  libavcodec/pixlet.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/libavcodec/pixlet.c b/libavcodec/pixlet.c
> >> > index 088226bdda..a9cfe085c9 100644
> >> > --- a/libavcodec/pixlet.c
> >> > +++ b/libavcodec/pixlet.c
> >> > @@ -262,7 +262,7 @@ static int read_high_coeffs(AVCodecContext *avctx,
> >> > uint8_t *src, int16_t *dst, i
> >> >
> >> >  flag = 0;
> >> >
> >> > -if (state * 4ULL > 0xFF || i >= size)
> >> > +if ((uint64_t)state > 0xFF / 4 || i >= size)
> >>
> >> This is not exact same behaviour.
> >
> > no, it differs for a small number of cases that have one of the 2
> > most significant bits (of the 64 bits) set in state.
> > I think this was a bug i introduced in a previous commit.
> >
> > If above change is not the correct thing to do, please elaborate.
> > state is passed into ff_clz() later which has no deterministic
> > behavior when these MSBs are set as ff_clz() uses unsigned and state
> > is int64_t and unsigned can be 32 or 64bit.
> 
> Only 32 bits matters for ff_clz() anyway.

your reply is a bit terse.
Does that mean you have no objections to the patch ?

about ff_clz(), with 64bit unsigned a ff_clz(1) should return 63, the
code looks like it expects 31

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

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.


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

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/pixlet: Fixes: undefined shift in av_mod_uintp2()

[Paul B Mahol]

On 8/19/17, Michael Niedermayer  wrote:
> On Fri, Aug 18, 2017 at 06:21:56PM +0200, Paul B Mahol wrote:
>> On 8/18/17, Michael Niedermayer  wrote:
>> > Fixes: runtime error: shift exponent 4294967289 is too large for 32-bit
>> > type
>> > 'int'
>> > Fixes: 3030/clusterfuzz-testcase-minimized-4649809254285312
>> >
>> > Found-by: continuous fuzzing process
>> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> > Signed-off-by: Michael Niedermayer 
>> > ---
>> >  libavcodec/pixlet.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/libavcodec/pixlet.c b/libavcodec/pixlet.c
>> > index 088226bdda..a9cfe085c9 100644
>> > --- a/libavcodec/pixlet.c
>> > +++ b/libavcodec/pixlet.c
>> > @@ -262,7 +262,7 @@ static int read_high_coeffs(AVCodecContext *avctx,
>> > uint8_t *src, int16_t *dst, i
>> >
>> >  flag = 0;
>> >
>> > -if (state * 4ULL > 0xFF || i >= size)
>> > +if ((uint64_t)state > 0xFF / 4 || i >= size)
>>
>> This is not exact same behaviour.
>
> no, it differs for a small number of cases that have one of the 2
> most significant bits (of the 64 bits) set in state.
> I think this was a bug i introduced in a previous commit.
>
> If above change is not the correct thing to do, please elaborate.
> state is passed into ff_clz() later which has no deterministic
> behavior when these MSBs are set as ff_clz() uses unsigned and state
> is int64_t and unsigned can be 32 or 64bit.

Only 32 bits matters for ff_clz() anyway.

> So this really just change cases that do not work equally
> across platforms
>
> looking at this now, i suspect independant of this patch
> the 32 and 24 in
> pfx = ((state + 8) >> 5) + (state ? ff_clz(state): 32) - 24;
> should be CHAR_BIT * sizeof(unsiged) or something similar
> or something else should be changed
>
> [...]
> --
> 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.
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/pixlet: Fixes: undefined shift in av_mod_uintp2()

[Michael Niedermayer]

On Fri, Aug 18, 2017 at 06:21:56PM +0200, Paul B Mahol wrote:
> On 8/18/17, Michael Niedermayer  wrote:
> > Fixes: runtime error: shift exponent 4294967289 is too large for 32-bit type
> > 'int'
> > Fixes: 3030/clusterfuzz-testcase-minimized-4649809254285312
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavcodec/pixlet.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/pixlet.c b/libavcodec/pixlet.c
> > index 088226bdda..a9cfe085c9 100644
> > --- a/libavcodec/pixlet.c
> > +++ b/libavcodec/pixlet.c
> > @@ -262,7 +262,7 @@ static int read_high_coeffs(AVCodecContext *avctx,
> > uint8_t *src, int16_t *dst, i
> >
> >  flag = 0;
> >
> > -if (state * 4ULL > 0xFF || i >= size)
> > +if ((uint64_t)state > 0xFF / 4 || i >= size)
> 
> This is not exact same behaviour.

no, it differs for a small number of cases that have one of the 2
most significant bits (of the 64 bits) set in state.
I think this was a bug i introduced in a previous commit.

If above change is not the correct thing to do, please elaborate.
state is passed into ff_clz() later which has no deterministic
behavior when these MSBs are set as ff_clz() uses unsigned and state
is int64_t and unsigned can be 32 or 64bit.
So this really just change cases that do not work equally
across platforms

looking at this now, i suspect independant of this patch
the 32 and 24 in
pfx = ((state + 8) >> 5) + (state ? ff_clz(state): 32) - 24;
should be CHAR_BIT * sizeof(unsiged) or something similar
or something else should be changed

[...]
-- 
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.


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

Re: [FFmpeg-devel] [PATCH 2/2] avcodec/pixlet: Fixes: undefined shift in av_mod_uintp2()

[Paul B Mahol]

On 8/18/17, Michael Niedermayer  wrote:
> Fixes: runtime error: shift exponent 4294967289 is too large for 32-bit type
> 'int'
> Fixes: 3030/clusterfuzz-testcase-minimized-4649809254285312
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/pixlet.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/pixlet.c b/libavcodec/pixlet.c
> index 088226bdda..a9cfe085c9 100644
> --- a/libavcodec/pixlet.c
> +++ b/libavcodec/pixlet.c
> @@ -262,7 +262,7 @@ static int read_high_coeffs(AVCodecContext *avctx,
> uint8_t *src, int16_t *dst, i
>
>  flag = 0;
>
> -if (state * 4ULL > 0xFF || i >= size)
> +if ((uint64_t)state > 0xFF / 4 || i >= size)

This is not exact same behaviour.

>  continue;
>
>  pfx = ((state + 8) >> 5) + (state ? ff_clz(state): 32) - 24;
> --
> 2.14.1
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 2/2] avcodec/pixlet: Fixes: undefined shift in av_mod_uintp2()

[Michael Niedermayer]

Fixes: runtime error: shift exponent 4294967289 is too large for 32-bit type 
'int'
Fixes: 3030/clusterfuzz-testcase-minimized-4649809254285312

Found-by: continuous fuzzing process 
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer 
---
 libavcodec/pixlet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/pixlet.c b/libavcodec/pixlet.c
index 088226bdda..a9cfe085c9 100644
--- a/libavcodec/pixlet.c
+++ b/libavcodec/pixlet.c
@@ -262,7 +262,7 @@ static int read_high_coeffs(AVCodecContext *avctx, uint8_t 
*src, int16_t *dst, i
 
 flag = 0;
 
-if (state * 4ULL > 0xFF || i >= size)
+if ((uint64_t)state > 0xFF / 4 || i >= size)
 continue;
 
 pfx = ((state + 8) >> 5) + (state ? ff_clz(state): 32) - 24;
-- 
2.14.1

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