| Message ID | 20240403225134.31764-5-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | bf3b74142e4402912e26b5e58a0b63f87ec3cd21 |
| Headers | show |
| Series | [FFmpeg-devel,1/5] avcodec/wavarc: fix signed integer overflow in block type 6/19 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Thu, 4 Apr 2024, Michael Niedermayer wrote: > Fixes: signed integer overflow: 65792 * 65312 cannot be represented in type 'int' > Fixes: 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavformat/pcm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/pcm.c b/libavformat/pcm.c > index 051e86dd464..a774dbc3726 100644 > --- a/libavformat/pcm.c > +++ b/libavformat/pcm.c > @@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par) > /* Don't trust the codecpar bitrate if we can calculate it ourselves */ > if (bits_per_sample > 0 && par->sample_rate > 0 && par->ch_layout.nb_channels > 0) > if ((int64_t)par->sample_rate * par->ch_layout.nb_channels < INT64_MAX / bits_per_sample) > - bitrate = bits_per_sample * par->sample_rate * par->ch_layout.nb_channels; > + bitrate = bits_per_sample * (int64_t)par->sample_rate * par->ch_layout.nb_channels; LGTM, thanks. I wonder why we usually cast the second operand and not the first to 64 bit, since cast has higher precedence than multiplication, it should not matter, should it? Regards, Marton
Marton Balint: > > > On Thu, 4 Apr 2024, Michael Niedermayer wrote: > >> Fixes: signed integer overflow: 65792 * 65312 cannot be represented in >> type 'int' >> Fixes: >> 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344 >> >> Found-by: continuous fuzzing process >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> --- >> libavformat/pcm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/pcm.c b/libavformat/pcm.c >> index 051e86dd464..a774dbc3726 100644 >> --- a/libavformat/pcm.c >> +++ b/libavformat/pcm.c >> @@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par) >> /* Don't trust the codecpar bitrate if we can calculate it >> ourselves */ >> if (bits_per_sample > 0 && par->sample_rate > 0 && >> par->ch_layout.nb_channels > 0) >> if ((int64_t)par->sample_rate * par->ch_layout.nb_channels < >> INT64_MAX / bits_per_sample) >> - bitrate = bits_per_sample * par->sample_rate * >> par->ch_layout.nb_channels; >> + bitrate = bits_per_sample * (int64_t)par->sample_rate * >> par->ch_layout.nb_channels; > > LGTM, thanks. > > I wonder why we usually cast the second operand and not the first to 64 > bit, since cast has higher precedence than multiplication, it should not > matter, should it? > Presuming that all variables here have integer conversion rank <= int64_t (true here for normal systems), then it does not matter: Casting the first operand would automatically promote all the other operands to int64_t, too; but if you add the cast to the last operand only, the first multiplication will be performed with ints only. - Andreas
On Thu, Apr 04, 2024 at 07:12:40PM +0200, Marton Balint wrote: > > > On Thu, 4 Apr 2024, Michael Niedermayer wrote: > > > Fixes: signed integer overflow: 65792 * 65312 cannot be represented in type 'int' > > Fixes: 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavformat/pcm.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavformat/pcm.c b/libavformat/pcm.c > > index 051e86dd464..a774dbc3726 100644 > > --- a/libavformat/pcm.c > > +++ b/libavformat/pcm.c > > @@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par) > > /* Don't trust the codecpar bitrate if we can calculate it ourselves */ > > if (bits_per_sample > 0 && par->sample_rate > 0 && par->ch_layout.nb_channels > 0) > > if ((int64_t)par->sample_rate * par->ch_layout.nb_channels < INT64_MAX / bits_per_sample) > > - bitrate = bits_per_sample * par->sample_rate * par->ch_layout.nb_channels; > > + bitrate = bits_per_sample * (int64_t)par->sample_rate * par->ch_layout.nb_channels; > > LGTM, thanks. > > I wonder why we usually cast the second operand and not the first to 64 bit, > since cast has higher precedence than multiplication, it should not matter, > should it? If the reader isnt sure about the precedence, writing it as a * (int64_t)b has the advantage that precedence doesnt matter, so the code is easier to understand and verify than (int64_t)a * b thx [...]
diff --git a/libavformat/pcm.c b/libavformat/pcm.c index 051e86dd464..a774dbc3726 100644 --- a/libavformat/pcm.c +++ b/libavformat/pcm.c @@ -41,7 +41,7 @@ int ff_pcm_default_packet_size(AVCodecParameters *par) /* Don't trust the codecpar bitrate if we can calculate it ourselves */ if (bits_per_sample > 0 && par->sample_rate > 0 && par->ch_layout.nb_channels > 0) if ((int64_t)par->sample_rate * par->ch_layout.nb_channels < INT64_MAX / bits_per_sample) - bitrate = bits_per_sample * par->sample_rate * par->ch_layout.nb_channels; + bitrate = bits_per_sample * (int64_t)par->sample_rate * par->ch_layout.nb_channels; if (bitrate > 0) { nb_samples = av_clip64(bitrate / 8 / PCM_DEMUX_TARGET_FPS / par->block_align, 1, max_samples);
Fixes: signed integer overflow: 65792 * 65312 cannot be represented in type 'int' Fixes: 67819/clusterfuzz-testcase-minimized-ffmpeg_dem_WADY_fuzzer-5236100912185344 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/pcm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)