Re: [FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()
On Tue, Nov 20, 2018 at 12:03:31PM +0100, Tomas Härdin wrote:
> tis 2018-11-20 klockan 00:04 +0100 skrev Michael Niedermayer:
> > On Mon, Nov 19, 2018 at 09:37:28AM +0100, Tomas Härdin wrote:
> > > mån 2018-11-19 klockan 02:02 +0100 skrev Michael Niedermayer:
> > > > On Sun, Nov 18, 2018 at 11:32:21PM +0100, Tomas Härdin wrote:
> > > > > lör 2018-11-17 klockan 03:01 +0100 skrev Michael Niedermayer:
> > > > > > Fixes:
> > > > > > 11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
> > > > > >
> > > > > > Found-by: continuous fuzzing process
> > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > Signed-off-by: Michael Niedermayer
> > > > > >
> > > > > > ---
> > > > > > libavcodec/truemotion2.c | 6 +++---
> > > > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> > > > > > index 58a577f53c..c583ff4032 100644
> > > > > > --- a/libavcodec/truemotion2.c
> > > > > > +++ b/libavcodec/truemotion2.c
> > > > > > @@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data,
> > > > > > int stride, int *last, unsigned *C
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > -static inline void tm2_low_chroma(int *data, int stride, int
> > > > > > *clast, int *CD, int *deltas, int bx)
> > > > > > +static inline void tm2_low_chroma(int *data, int stride, int
> > > > > > *clast, unsigned *CD, int *deltas, int bx)
> > > > > > {
> > > > > > int t;
> > > > > > int l;
> > > > > > @@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data,
> > > > > > int stride, int *clast, int *CD, in
> > > > > > prev = clast[-3];
> > > > > > else
> > > > > > prev = 0;
> > > > > > -t= (CD[0] + CD[1]) >> 1;
> > > > > > -l= (prev - CD[0] - CD[1] + clast[1]) >> 1;
> > > > > > +t= (int)(CD[0] + CD[1]) >> 1;
> > > > >
> > > > > I presume the old code would overflow for sums exceeding INT_MAX.
> > > >
> > > > There were overflows in the sense of undefined behavior, yes
> > > >
> > > >
> > > > > Why
> > > > > then is there a cast to int before the shift and not after?
> > > >
> > > > because shifts of signed and unsigned values produce different results
> > > >
> > > > maybe the unsigned type confused you. CD is signed in a semantic sense
> > > > its
> > > > stored in a unsigned type because otherwise this code has undefined
> > > > behavior.
> > > > It would be better to use a type with different name but that had lead
> > > > to
> > > > long arguments that went nowhere in the past. So i tend to use plain
> > > > unsigned now for these kind of cases as it seems that is what most
> > > > people
> > > > prefer.
> > >
> > > So signed overflow is intended? Huh, weird format.
> >
> > it occurs in the fuzzed sample used here. i do not know if it occurs in
> > normal samples, i would suspect it does not.
>
> I see. Some kind of spec might be nice, but I suspect the spec as it is
> is named Kostya
yes, probably ...
will apply
thanks
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()
tis 2018-11-20 klockan 00:04 +0100 skrev Michael Niedermayer:
> On Mon, Nov 19, 2018 at 09:37:28AM +0100, Tomas Härdin wrote:
> > mån 2018-11-19 klockan 02:02 +0100 skrev Michael Niedermayer:
> > > On Sun, Nov 18, 2018 at 11:32:21PM +0100, Tomas Härdin wrote:
> > > > lör 2018-11-17 klockan 03:01 +0100 skrev Michael Niedermayer:
> > > > > Fixes:
> > > > > 11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
> > > > >
> > > > > Found-by: continuous fuzzing process
> > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > Signed-off-by: Michael Niedermayer
> > > > >
> > > > > ---
> > > > > libavcodec/truemotion2.c | 6 +++---
> > > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> > > > > index 58a577f53c..c583ff4032 100644
> > > > > --- a/libavcodec/truemotion2.c
> > > > > +++ b/libavcodec/truemotion2.c
> > > > > @@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data, int
> > > > > stride, int *last, unsigned *C
> > > > > }
> > > > > }
> > > > >
> > > > > -static inline void tm2_low_chroma(int *data, int stride, int *clast,
> > > > > int *CD, int *deltas, int bx)
> > > > > +static inline void tm2_low_chroma(int *data, int stride, int *clast,
> > > > > unsigned *CD, int *deltas, int bx)
> > > > > {
> > > > > int t;
> > > > > int l;
> > > > > @@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data, int
> > > > > stride, int *clast, int *CD, in
> > > > > prev = clast[-3];
> > > > > else
> > > > > prev = 0;
> > > > > -t= (CD[0] + CD[1]) >> 1;
> > > > > -l= (prev - CD[0] - CD[1] + clast[1]) >> 1;
> > > > > +t= (int)(CD[0] + CD[1]) >> 1;
> > > >
> > > > I presume the old code would overflow for sums exceeding INT_MAX.
> > >
> > > There were overflows in the sense of undefined behavior, yes
> > >
> > >
> > > > Why
> > > > then is there a cast to int before the shift and not after?
> > >
> > > because shifts of signed and unsigned values produce different results
> > >
> > > maybe the unsigned type confused you. CD is signed in a semantic sense its
> > > stored in a unsigned type because otherwise this code has undefined
> > > behavior.
> > > It would be better to use a type with different name but that had lead to
> > > long arguments that went nowhere in the past. So i tend to use plain
> > > unsigned now for these kind of cases as it seems that is what most people
> > > prefer.
> >
> > So signed overflow is intended? Huh, weird format.
>
> it occurs in the fuzzed sample used here. i do not know if it occurs in
> normal samples, i would suspect it does not.
I see. Some kind of spec might be nice, but I suspect the spec as it is
is named Kostya
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()
On Mon, Nov 19, 2018 at 09:37:28AM +0100, Tomas Härdin wrote:
> mån 2018-11-19 klockan 02:02 +0100 skrev Michael Niedermayer:
> > On Sun, Nov 18, 2018 at 11:32:21PM +0100, Tomas Härdin wrote:
> > > lör 2018-11-17 klockan 03:01 +0100 skrev Michael Niedermayer:
> > > > Fixes:
> > > > 11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
> > > >
> > > > Found-by: continuous fuzzing process
> > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer
> > > >
> > > > ---
> > > > libavcodec/truemotion2.c | 6 +++---
> > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> > > > index 58a577f53c..c583ff4032 100644
> > > > --- a/libavcodec/truemotion2.c
> > > > +++ b/libavcodec/truemotion2.c
> > > > @@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data, int
> > > > stride, int *last, unsigned *C
> > > > }
> > > > }
> > > >
> > > > -static inline void tm2_low_chroma(int *data, int stride, int *clast,
> > > > int *CD, int *deltas, int bx)
> > > > +static inline void tm2_low_chroma(int *data, int stride, int *clast,
> > > > unsigned *CD, int *deltas, int bx)
> > > > {
> > > > int t;
> > > > int l;
> > > > @@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data, int
> > > > stride, int *clast, int *CD, in
> > > > prev = clast[-3];
> > > > else
> > > > prev = 0;
> > > > -t= (CD[0] + CD[1]) >> 1;
> > > > -l= (prev - CD[0] - CD[1] + clast[1]) >> 1;
> > > > +t= (int)(CD[0] + CD[1]) >> 1;
> > >
> > > I presume the old code would overflow for sums exceeding INT_MAX.
> >
> > There were overflows in the sense of undefined behavior, yes
> >
> >
> > > Why
> > > then is there a cast to int before the shift and not after?
> >
> > because shifts of signed and unsigned values produce different results
> >
> > maybe the unsigned type confused you. CD is signed in a semantic sense its
> > stored in a unsigned type because otherwise this code has undefined
> > behavior.
> > It would be better to use a type with different name but that had lead to
> > long arguments that went nowhere in the past. So i tend to use plain
> > unsigned now for these kind of cases as it seems that is what most people
> > prefer.
>
> So signed overflow is intended? Huh, weird format.
it occurs in the fuzzed sample used here. i do not know if it occurs in
normal samples, i would suspect it does not.
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
When the tyrant has disposed of foreign enemies by conquest or treaty, and
there is nothing more to fear from them, then he is always stirring up
some war or other, in order that the people may require a leader. -- Plato
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()
mån 2018-11-19 klockan 02:02 +0100 skrev Michael Niedermayer:
> On Sun, Nov 18, 2018 at 11:32:21PM +0100, Tomas Härdin wrote:
> > lör 2018-11-17 klockan 03:01 +0100 skrev Michael Niedermayer:
> > > Fixes:
> > > 11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
> > >
> > > Found-by: continuous fuzzing process
> > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer
> > >
> > > ---
> > > libavcodec/truemotion2.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> > > index 58a577f53c..c583ff4032 100644
> > > --- a/libavcodec/truemotion2.c
> > > +++ b/libavcodec/truemotion2.c
> > > @@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data, int
> > > stride, int *last, unsigned *C
> > > }
> > > }
> > >
> > > -static inline void tm2_low_chroma(int *data, int stride, int *clast, int
> > > *CD, int *deltas, int bx)
> > > +static inline void tm2_low_chroma(int *data, int stride, int *clast,
> > > unsigned *CD, int *deltas, int bx)
> > > {
> > > int t;
> > > int l;
> > > @@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data, int
> > > stride, int *clast, int *CD, in
> > > prev = clast[-3];
> > > else
> > > prev = 0;
> > > -t= (CD[0] + CD[1]) >> 1;
> > > -l= (prev - CD[0] - CD[1] + clast[1]) >> 1;
> > > +t= (int)(CD[0] + CD[1]) >> 1;
> >
> > I presume the old code would overflow for sums exceeding INT_MAX.
>
> There were overflows in the sense of undefined behavior, yes
>
>
> > Why
> > then is there a cast to int before the shift and not after?
>
> because shifts of signed and unsigned values produce different results
>
> maybe the unsigned type confused you. CD is signed in a semantic sense its
> stored in a unsigned type because otherwise this code has undefined behavior.
> It would be better to use a type with different name but that had lead to
> long arguments that went nowhere in the past. So i tend to use plain
> unsigned now for these kind of cases as it seems that is what most people
> prefer.
So signed overflow is intended? Huh, weird format.
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()
On Sun, Nov 18, 2018 at 11:32:21PM +0100, Tomas Härdin wrote:
> lör 2018-11-17 klockan 03:01 +0100 skrev Michael Niedermayer:
> > Fixes:
> > 11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
> >
> > Found-by: continuous fuzzing process
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer
> > ---
> > libavcodec/truemotion2.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> > index 58a577f53c..c583ff4032 100644
> > --- a/libavcodec/truemotion2.c
> > +++ b/libavcodec/truemotion2.c
> > @@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data, int
> > stride, int *last, unsigned *C
> > }
> > }
> >
> > -static inline void tm2_low_chroma(int *data, int stride, int *clast, int
> > *CD, int *deltas, int bx)
> > +static inline void tm2_low_chroma(int *data, int stride, int *clast,
> > unsigned *CD, int *deltas, int bx)
> > {
> > int t;
> > int l;
> > @@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data, int
> > stride, int *clast, int *CD, in
> > prev = clast[-3];
> > else
> > prev = 0;
> > -t= (CD[0] + CD[1]) >> 1;
> > -l= (prev - CD[0] - CD[1] + clast[1]) >> 1;
> > +t= (int)(CD[0] + CD[1]) >> 1;
>
> I presume the old code would overflow for sums exceeding INT_MAX.
There were overflows in the sense of undefined behavior, yes
> Why
> then is there a cast to int before the shift and not after?
because shifts of signed and unsigned values produce different results
maybe the unsigned type confused you. CD is signed in a semantic sense its
stored in a unsigned type because otherwise this code has undefined behavior.
It would be better to use a type with different name but that had lead to
long arguments that went nowhere in the past. So i tend to use plain
unsigned now for these kind of cases as it seems that is what most people
prefer.
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()
lör 2018-11-17 klockan 03:01 +0100 skrev Michael Niedermayer:
> Fixes:
> 11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer
> ---
> libavcodec/truemotion2.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
> index 58a577f53c..c583ff4032 100644
> --- a/libavcodec/truemotion2.c
> +++ b/libavcodec/truemotion2.c
> @@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data, int stride,
> int *last, unsigned *C
> }
> }
>
> -static inline void tm2_low_chroma(int *data, int stride, int *clast, int
> *CD, int *deltas, int bx)
> +static inline void tm2_low_chroma(int *data, int stride, int *clast,
> unsigned *CD, int *deltas, int bx)
> {
> int t;
> int l;
> @@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data, int stride,
> int *clast, int *CD, in
> prev = clast[-3];
> else
> prev = 0;
> -t= (CD[0] + CD[1]) >> 1;
> -l= (prev - CD[0] - CD[1] + clast[1]) >> 1;
> +t= (int)(CD[0] + CD[1]) >> 1;
I presume the old code would overflow for sums exceeding INT_MAX. Why
then is there a cast to int before the shift and not after?
/Tomas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/3] avcodec/truemotion2: fix integer overflows in tm2_low_chroma()
Fixes:
11295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_TRUEMOTION2_fuzzer-4888953459572736
Found-by: continuous fuzzing process
https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer
---
libavcodec/truemotion2.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/libavcodec/truemotion2.c b/libavcodec/truemotion2.c
index 58a577f53c..c583ff4032 100644
--- a/libavcodec/truemotion2.c
+++ b/libavcodec/truemotion2.c
@@ -484,7 +484,7 @@ static inline void tm2_high_chroma(int *data, int stride,
int *last, unsigned *C
}
}
-static inline void tm2_low_chroma(int *data, int stride, int *clast, int *CD,
int *deltas, int bx)
+static inline void tm2_low_chroma(int *data, int stride, int *clast, unsigned
*CD, int *deltas, int bx)
{
int t;
int l;
@@ -494,8 +494,8 @@ static inline void tm2_low_chroma(int *data, int stride,
int *clast, int *CD, in
prev = clast[-3];
else
prev = 0;
-t= (CD[0] + CD[1]) >> 1;
-l= (prev - CD[0] - CD[1] + clast[1]) >> 1;
+t= (int)(CD[0] + CD[1]) >> 1;
+l= (int)(prev - CD[0] - CD[1] + clast[1]) >> 1;
CD[1]= CD[0] + CD[1] - t;
CD[0]= t;
clast[0] = l;
--
2.19.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
