| Message ID | 20211012135040.764167-1-derek.buitenhuis@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mov: Do not hard fail if bit rate calculation overflows unless in explode mode | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Tue, Oct 12, 2021 at 02:50:40PM +0100, Derek Buitenhuis wrote: > bit_rate is not a critical field, and we shouln't hard fail if we > can't caluclate it due to a large timebase - it needlessly breaks > valid files. > > Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> > --- > libavformat/mov.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index d0b8b2595b..99d10c2b33 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -7666,11 +7666,12 @@ static int mov_read_header(AVFormatContext *s) > MOVStreamContext *sc = st->priv_data; > if (st->duration > 0) { > if (sc->data_size > INT64_MAX / sc->time_scale / 8) { > - av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n", > + av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n", > sc->data_size, sc->time_scale); > - return AVERROR_INVALIDDATA; > - } > - st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration; > + if (s->error_recognition & AV_EF_EXPLODE) > + return AVERROR_INVALIDDATA; > + } else > + st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration; > } this should be using av_rescale() i think thx [...]
On 10/12/2021 3:43 PM, Michael Niedermayer wrote:
> this should be using av_rescale() i think
That seems unrelated to this patch - but I can send a second patch
that does that, if you want.
Is your intent something like this;
if (sc->data_size > INT64_MAX / 8) {
av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 > INT64_MAX\n",
sc->data_size);
if (s->error_recognition & AV_EF_EXPLODE)
return AVERROR_INVALIDDATA;
} else
st->codecpar->bit_rate = av_rescale(sc->data_size * 8, c->time_scale, st->duration);
- Derek
On Tue, Oct 12, 2021 at 04:17:31PM +0100, Derek Buitenhuis wrote: > On 10/12/2021 3:43 PM, Michael Niedermayer wrote: > > this should be using av_rescale() i think > > That seems unrelated to this patch - but I can send a second patch > that does that, if you want. > > Is your intent something like this; > > if (sc->data_size > INT64_MAX / 8) { > av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 > INT64_MAX\n", > sc->data_size); > if (s->error_recognition & AV_EF_EXPLODE) > return AVERROR_INVALIDDATA; > } else > st->codecpar->bit_rate = av_rescale(sc->data_size * 8, c->time_scale, st->duration); i was thinking of something like st->codecpar->bit_rate = av_rescale(sc->data_size, c->time_scale * 8LL, st->duration); because i thought that would fix the overflow but i didnt look beyond the code in this patch this may still require some check so the value fits in bit_rate, i didnt investigate that thx [...]
Hi, Sorry for the slow reply. On 10/12/2021 8:18 PM, Michael Niedermayer wrote: > i was thinking of something like > st->codecpar->bit_rate = av_rescale(sc->data_size, c->time_scale * 8LL, st->duration); > > because i thought that would fix the overflow > but i didnt look beyond the code in this patch > this may still require some check so the value fits in bit_rate, i didnt > investigate that It's unclear to me from reading the code of av_rescale what happens if the return value is not representative as an int64_t, for example: av_rescale(INT64_MAX/2, INT64_MAX/2, 1); I don't think we can even check this usng only av_rescale without explicit checks before calling it? - Derek
On Mon, Oct 18, 2021 at 02:24:08PM +0100, Derek Buitenhuis wrote: > Hi, > > Sorry for the slow reply. > > On 10/12/2021 8:18 PM, Michael Niedermayer wrote: > > i was thinking of something like > > st->codecpar->bit_rate = av_rescale(sc->data_size, c->time_scale * 8LL, st->duration); > > > > because i thought that would fix the overflow > > but i didnt look beyond the code in this patch > > this may still require some check so the value fits in bit_rate, i didnt > > investigate that > > It's unclear to me from reading the code of av_rescale what happens if > the return value is not representative as an int64_t, for example: > > av_rescale(INT64_MAX/2, INT64_MAX/2, 1); > > I don't think we can even check this usng only av_rescale without > explicit checks before calling it? av_rescale() should return INT64_MIN on overflows thx [...]
diff --git a/libavformat/mov.c b/libavformat/mov.c index d0b8b2595b..99d10c2b33 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -7666,11 +7666,12 @@ static int mov_read_header(AVFormatContext *s) MOVStreamContext *sc = st->priv_data; if (st->duration > 0) { if (sc->data_size > INT64_MAX / sc->time_scale / 8) { - av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n", + av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n", sc->data_size, sc->time_scale); - return AVERROR_INVALIDDATA; - } - st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration; + if (s->error_recognition & AV_EF_EXPLODE) + return AVERROR_INVALIDDATA; + } else + st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / st->duration; } } } @@ -7681,12 +7682,13 @@ static int mov_read_header(AVFormatContext *s) MOVStreamContext *sc = st->priv_data; if (sc->duration_for_fps > 0) { if (sc->data_size > INT64_MAX / sc->time_scale / 8) { - av_log(s, AV_LOG_ERROR, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n", + av_log(s, AV_LOG_WARNING, "Overflow during bit rate calculation %"PRId64" * 8 * %d\n", sc->data_size, sc->time_scale); - return AVERROR_INVALIDDATA; - } - st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / - sc->duration_for_fps; + if (s->error_recognition & AV_EF_EXPLODE) + return AVERROR_INVALIDDATA; + } else + st->codecpar->bit_rate = sc->data_size * 8 * sc->time_scale / + sc->duration_for_fps; } } }
bit_rate is not a critical field, and we shouln't hard fail if we can't caluclate it due to a large timebase - it needlessly breaks valid files. Signed-off-by: Derek Buitenhuis <derek.buitenhuis@gmail.com> --- libavformat/mov.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)