| Message ID | 20230416164830.15664-5-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | fd7352660be0211aabb11dc6d586836515772f81 |
| Headers | show |
| Series | [FFmpeg-devel,01/11] avcodec/adpcm: Fix integer overflow in intermediate in ADPCM_XMD | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 4/16/2023 1:48 PM, Michael Niedermayer wrote: > Fixes: signed integer overflow: 3011809745540902265 + 6323452730883571725 cannot be represented in type 'long' > Fixes: 45982/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-6687553022722048 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/flacdec.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c > index cc778a8dff1..524a0469495 100644 > --- a/libavcodec/flacdec.c > +++ b/libavcodec/flacdec.c > @@ -513,7 +513,7 @@ static int decode_subframe_lpc_33bps(FLACContext *s, int64_t *decoded, > for (i = pred_order; i < s->blocksize; i++, decoded++) { > int64_t sum = 0; > for (j = 0; j < pred_order; j++) > - sum += (int64_t)coeffs[j] * decoded[j]; > + sum += (int64_t)coeffs[j] * (uint64_t)decoded[j]; Why not instead do sum = av_sat_add64(sum, (int64_t)coeffs[j] * decoded[j]); Also, decoded[j] is an int64_t, so wouldn't coeffs[j] be promoted if you swap the order in the multiplication, thus saving the cast? > decoded[j] = residual[i] + (sum >> qlevel); > } >
On Fri, May 05, 2023 at 07:36:05PM -0300, James Almer wrote: > On 4/16/2023 1:48 PM, Michael Niedermayer wrote: > > Fixes: signed integer overflow: 3011809745540902265 + 6323452730883571725 cannot be represented in type 'long' > > Fixes: 45982/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-6687553022722048 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/flacdec.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c > > index cc778a8dff1..524a0469495 100644 > > --- a/libavcodec/flacdec.c > > +++ b/libavcodec/flacdec.c > > @@ -513,7 +513,7 @@ static int decode_subframe_lpc_33bps(FLACContext *s, int64_t *decoded, > > for (i = pred_order; i < s->blocksize; i++, decoded++) { > > int64_t sum = 0; > > for (j = 0; j < pred_order; j++) > > - sum += (int64_t)coeffs[j] * decoded[j]; > > + sum += (int64_t)coeffs[j] * (uint64_t)decoded[j]; > > Why not instead do > > sum = av_sat_add64(sum, (int64_t)coeffs[j] * decoded[j]); Why should this be clipping ? flac is a lossless codec, i see nothing in the specification that calls for cliping. > > Also, decoded[j] is an int64_t, so wouldn't coeffs[j] be promoted if you > swap the order in the multiplication, thus saving the cast? it can be shuffled around to achieve the same, do you prefer coeffs[j] * (uint64_t)decoded[j]; ? thx [...]
On 5/6/2023 12:08 PM, Michael Niedermayer wrote: > On Fri, May 05, 2023 at 07:36:05PM -0300, James Almer wrote: >> On 4/16/2023 1:48 PM, Michael Niedermayer wrote: >>> Fixes: signed integer overflow: 3011809745540902265 + 6323452730883571725 cannot be represented in type 'long' >>> Fixes: 45982/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-6687553022722048 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/flacdec.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/flacdec.c b/libavcodec/flacdec.c >>> index cc778a8dff1..524a0469495 100644 >>> --- a/libavcodec/flacdec.c >>> +++ b/libavcodec/flacdec.c >>> @@ -513,7 +513,7 @@ static int decode_subframe_lpc_33bps(FLACContext *s, int64_t *decoded, >>> for (i = pred_order; i < s->blocksize; i++, decoded++) { >>> int64_t sum = 0; >>> for (j = 0; j < pred_order; j++) >>> - sum += (int64_t)coeffs[j] * decoded[j]; >>> + sum += (int64_t)coeffs[j] * (uint64_t)decoded[j]; >> >> Why not instead do >> >> sum = av_sat_add64(sum, (int64_t)coeffs[j] * decoded[j]); > > Why should this be clipping ? > flac is a lossless codec, i see nothing in the specification that calls for > cliping. No, but an overflowing case like 3011809745540902265 + 6323452730883571725 isn't supported either and will generate bad output, so might as well use an optimized function for this. > > >> >> Also, decoded[j] is an int64_t, so wouldn't coeffs[j] be promoted if you >> swap the order in the multiplication, thus saving the cast? > > it can be shuffled around to achieve the same, do you prefer > coeffs[j] * (uint64_t)decoded[j]; ? No gain doing that compared to your first version. I suggested it for av_sat_add64(), which i insist you should use, if you want with a comment about it being there not for spec reasons but to prevent integer overflows, and removing all casts. > > thx > > [...] > > > _______________________________________________ > 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/flacdec.c b/libavcodec/flacdec.c index cc778a8dff1..524a0469495 100644 --- a/libavcodec/flacdec.c +++ b/libavcodec/flacdec.c @@ -513,7 +513,7 @@ static int decode_subframe_lpc_33bps(FLACContext *s, int64_t *decoded, for (i = pred_order; i < s->blocksize; i++, decoded++) { int64_t sum = 0; for (j = 0; j < pred_order; j++) - sum += (int64_t)coeffs[j] * decoded[j]; + sum += (int64_t)coeffs[j] * (uint64_t)decoded[j]; decoded[j] = residual[i] + (sum >> qlevel); }
Fixes: signed integer overflow: 3011809745540902265 + 6323452730883571725 cannot be represented in type 'long' Fixes: 45982/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-6687553022722048 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/flacdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)