| Message ID | 20170818061447.27158-1-vitalybuka@google.com |
|---|---|
| State | Superseded |
| Headers | show |
On 2017-08-18 08:14, Vitaly Buka wrote: > Signed integer overflow is undefined behavior. > Detected with clang and -fsanitize=signed-integer-overflow > > Signed-off-by: Vitaly Buka <vitalybuka@google.com> > --- > libavcodec/utils.c | 2 +- > libavformat/aviobuf.c | 4 +++- > libavformat/mov.c | 2 +- > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 1336e921c9..024dc1f3e2 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > > if (!avctx->rc_initial_buffer_occupancy) > - avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3 / 4; > + avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3ll / 4; Didn't know you could use lower case for long long constants. Neat > if (avctx->ticks_per_frame && avctx->time_base.num && > avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) { > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index 7f4e740a33..319a402faf 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -259,7 +259,9 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) > offset1 = pos + (s->buf_ptr - s->buffer); > if (offset == 0) > return offset1; > - offset += offset1; > + // Use unsigned type to avoid undefined behavior of singed overflow. > + // Code below will report error on overflow anyway. > + offset += (uint64_t)offset1; I presume offset1 is some value which is never >= 1<<63? > } > if (offset < 0) > return AVERROR(EINVAL); > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 522ce60c2d..a14c9f182b 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom) > > if (atom.size < 0) > atom.size = INT64_MAX; > - while (total_size + 8 <= atom.size && !avio_feof(pb)) { > + while (total_size <= atom.size - 8 && !avio_feof(pb)) { atom.size can never be < 8? /Tomas
2017-08-18 8:14 GMT+02:00 Vitaly Buka <vitalybuka-at-google.com@ffmpeg.org>: > Signed integer overflow is undefined behavior. > Detected with clang and -fsanitize=signed-integer-overflow > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom) > > if (atom.size < 0) > atom.size = INT64_MAX; > - while (total_size + 8 <= atom.size && !avio_feof(pb)) { > + while (total_size <= atom.size - 8 && !avio_feof(pb)) { Can you provide the sample that produces this overflow? Carl Eugen
On Fri, Aug 18, 2017 at 1:11 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote: > On 2017-08-18 08:14, Vitaly Buka wrote: > >> Signed integer overflow is undefined behavior. >> Detected with clang and -fsanitize=signed-integer-overflow >> >> Signed-off-by: Vitaly Buka <vitalybuka@google.com> >> --- >> libavcodec/utils.c | 2 +- >> libavformat/aviobuf.c | 4 +++- >> libavformat/mov.c | 2 +- >> 3 files changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/libavcodec/utils.c b/libavcodec/utils.c >> index 1336e921c9..024dc1f3e2 100644 >> --- a/libavcodec/utils.c >> +++ b/libavcodec/utils.c >> @@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS >> } >> if (!avctx->rc_initial_buffer_occupancy) >> - avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size >> * 3 / 4; >> + avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size >> * 3ll / 4; >> > > Didn't know you could use lower case for long long constants. Neat > > if (avctx->ticks_per_frame && avctx->time_base.num && >> avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) { >> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c >> index 7f4e740a33..319a402faf 100644 >> --- a/libavformat/aviobuf.c >> +++ b/libavformat/aviobuf.c >> @@ -259,7 +259,9 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int >> whence) >> offset1 = pos + (s->buf_ptr - s->buffer); >> if (offset == 0) >> return offset1; >> - offset += offset1; >> + // Use unsigned type to avoid undefined behavior of singed >> overflow. >> + // Code below will report error on overflow anyway. >> + offset += (uint64_t)offset1; >> > > I presume offset1 is some value which is never >= 1<<63? Yes. it's it int64 so max value is (1ll<<63 - 1) > > > > } >> if (offset < 0) >> return AVERROR(EINVAL); >> diff --git a/libavformat/mov.c b/libavformat/mov.c >> index 522ce60c2d..a14c9f182b 100644 >> --- a/libavformat/mov.c >> +++ b/libavformat/mov.c >> @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c, >> AVIOContext *pb, MOVAtom atom) >> if (atom.size < 0) >> atom.size = INT64_MAX; >> - while (total_size + 8 <= atom.size && !avio_feof(pb)) { >> + while (total_size <= atom.size - 8 && !avio_feof(pb)) { >> > > atom.size can never be < 8? > It does not matter. We just need atom.size never < (INT64_MIN + 8) > > /Tomas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
Not sure or it's going to be very hard for me. third_party/ffmpeg/LGPL_pristine/libavformat/aviobuf.c:225:16 Error was: mov.c:3961:23: runtime error: signed integer overflow: 9223372036854775807 + 8 cannot be represented in type 'long' On Fri, Aug 18, 2017 at 1:13 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-08-18 8:14 GMT+02:00 Vitaly Buka <vitalybuka-at-google.com@ffmpeg.org > >: > > Signed integer overflow is undefined behavior. > > Detected with clang and -fsanitize=signed-integer-overflow > > > --- a/libavformat/mov.c > > +++ b/libavformat/mov.c > > @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c, > AVIOContext *pb, MOVAtom atom) > > > > if (atom.size < 0) > > atom.size = INT64_MAX; > > - while (total_size + 8 <= atom.size && !avio_feof(pb)) { > > + while (total_size <= atom.size - 8 && !avio_feof(pb)) { > > Can you provide the sample that produces this overflow? > > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Thu, Aug 17, 2017 at 11:14:47PM -0700, Vitaly Buka wrote: > Signed integer overflow is undefined behavior. > Detected with clang and -fsanitize=signed-integer-overflow > > Signed-off-by: Vitaly Buka <vitalybuka@google.com> > --- > libavcodec/utils.c | 2 +- > libavformat/aviobuf.c | 4 +++- > libavformat/mov.c | 2 +- > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/utils.c b/libavcodec/utils.c > index 1336e921c9..024dc1f3e2 100644 > --- a/libavcodec/utils.c > +++ b/libavcodec/utils.c > @@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS > } > > if (!avctx->rc_initial_buffer_occupancy) > - avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3 / 4; > + avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3ll / 4; > > if (avctx->ticks_per_frame && avctx->time_base.num && > avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) { > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index 7f4e740a33..319a402faf 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -259,7 +259,9 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) > offset1 = pos + (s->buf_ptr - s->buffer); > if (offset == 0) > return offset1; > - offset += offset1; > + // Use unsigned type to avoid undefined behavior of singed overflow. > + // Code below will report error on overflow anyway. > + offset += (uint64_t)offset1; instead of 2 lines of comments why not add a if() that checks for the specififc case and error out instead of the cast? The code from the patch depends on the input being limited range and being followed by a check. If either changes then the cast to uin64_t would silently give something wrong [...]
diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 1336e921c9..024dc1f3e2 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -971,7 +971,7 @@ FF_ENABLE_DEPRECATION_WARNINGS } if (!avctx->rc_initial_buffer_occupancy) - avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3 / 4; + avctx->rc_initial_buffer_occupancy = avctx->rc_buffer_size * 3ll / 4; if (avctx->ticks_per_frame && avctx->time_base.num && avctx->ticks_per_frame > INT_MAX / avctx->time_base.num) { diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 7f4e740a33..319a402faf 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -259,7 +259,9 @@ int64_t avio_seek(AVIOContext *s, int64_t offset, int whence) offset1 = pos + (s->buf_ptr - s->buffer); if (offset == 0) return offset1; - offset += offset1; + // Use unsigned type to avoid undefined behavior of singed overflow. + // Code below will report error on overflow anyway. + offset += (uint64_t)offset1; } if (offset < 0) return AVERROR(EINVAL); diff --git a/libavformat/mov.c b/libavformat/mov.c index 522ce60c2d..a14c9f182b 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -5572,7 +5572,7 @@ static int mov_read_default(MOVContext *c, AVIOContext *pb, MOVAtom atom) if (atom.size < 0) atom.size = INT64_MAX; - while (total_size + 8 <= atom.size && !avio_feof(pb)) { + while (total_size <= atom.size - 8 && !avio_feof(pb)) { int (*parse)(MOVContext*, AVIOContext*, MOVAtom) = NULL; a.size = atom.size; a.type=0;
Signed integer overflow is undefined behavior. Detected with clang and -fsanitize=signed-integer-overflow Signed-off-by: Vitaly Buka <vitalybuka@google.com> --- libavcodec/utils.c | 2 +- libavformat/aviobuf.c | 4 +++- libavformat/mov.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-)