| Message ID | 20181117020145.17164-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 2ae39d795613f3c6925c59852b625029b747fe42 |
| Headers | show |
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 <michael@niedermayer.cc> > --- > 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
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 <michael@niedermayer.cc> > > --- > > 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 [...]
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 <michael@niedermayer.cc> > > > > > > --- > > > 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
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 <michael@niedermayer.cc> > > > > > > > > --- > > > > 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. [...]
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 <michael@niedermayer.cc> > > > > > > > > > > --- > > > > > 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
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 <michael@niedermayer.cc> > > > > > > > > > > > > --- > > > > > > 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 [...]
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;
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 <michael@niedermayer.cc> --- libavcodec/truemotion2.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)