Message ID | 20200525140801.18889-1-andreas.rheinhardt@gmail.com |
---|---|
State | Accepted |
Commit | 28a078eded1c29985ed078b59d48ff59cf00394b |
Headers | show |
Series | [FFmpeg-devel,1/5] avformat/aviobuf: Don't check for overflow after it happened | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On 5/25/2020 11:07 AM, Andreas Rheinhardt wrote: > If adding two ints overflows, it doesn't matter whether the result will > be stored in an unsigned or not; and checking afterwards does not make it > retroactively defined. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/aviobuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index eb0387bdf7..33c2d6f037 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -1275,7 +1275,7 @@ static int dyn_buf_write(void *opaque, uint8_t *buf, int buf_size) > unsigned new_size, new_allocated_size; > > /* reallocate buffer if needed */ > - new_size = d->pos + buf_size; > + new_size = (unsigned)d->pos + buf_size; > new_allocated_size = d->allocated_size; > if (new_size < d->pos || new_size > INT_MAX/2) > return -1; You could instead do if (d->pos > INT_MAX / 2 - buf_size) return -1; new_size = d->pos + buf_size; Should also work fine with the changes in 3/5 adapted to it.
James Almer: > On 5/25/2020 11:07 AM, Andreas Rheinhardt wrote: >> If adding two ints overflows, it doesn't matter whether the result will >> be stored in an unsigned or not; and checking afterwards does not make it >> retroactively defined. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavformat/aviobuf.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c >> index eb0387bdf7..33c2d6f037 100644 >> --- a/libavformat/aviobuf.c >> +++ b/libavformat/aviobuf.c >> @@ -1275,7 +1275,7 @@ static int dyn_buf_write(void *opaque, uint8_t *buf, int buf_size) >> unsigned new_size, new_allocated_size; >> >> /* reallocate buffer if needed */ >> - new_size = d->pos + buf_size; >> + new_size = (unsigned)d->pos + buf_size; >> new_allocated_size = d->allocated_size; >> if (new_size < d->pos || new_size > INT_MAX/2) >> return -1; > > You could instead do > > if (d->pos > INT_MAX / 2 - buf_size) > return -1; > new_size = d->pos + buf_size; > > Should also work fine with the changes in 3/5 adapted to it. This would drop the check for whether buf_size is negative. While I am not opposed to this (such a check should be made generically, so that every AVIOContext.write_packet() can rely on buf_size being >= 0 (0 could probably be also dealt with generically)), I intended to do this only later (in a future patchset). This implied for my/the current version that one could drop the check for new_size < d->pos. And with this change your proposal loses its advantage. (Also notice that if buf_size is negative, INT_MAX / 2 - buf_size might overflow and INT_MAX - buf_size does overflow.) - Andreas
Andreas Rheinhardt: > If adding two ints overflows, it doesn't matter whether the result will > be stored in an unsigned or not; and checking afterwards does not make it > retroactively defined. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavformat/aviobuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index eb0387bdf7..33c2d6f037 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -1275,7 +1275,7 @@ static int dyn_buf_write(void *opaque, uint8_t *buf, int buf_size) > unsigned new_size, new_allocated_size; > > /* reallocate buffer if needed */ > - new_size = d->pos + buf_size; > + new_size = (unsigned)d->pos + buf_size; > new_allocated_size = d->allocated_size; > if (new_size < d->pos || new_size > INT_MAX/2) > return -1; > Will apply this patchset tomorrow unless there are objections. - Andreas
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index eb0387bdf7..33c2d6f037 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -1275,7 +1275,7 @@ static int dyn_buf_write(void *opaque, uint8_t *buf, int buf_size) unsigned new_size, new_allocated_size; /* reallocate buffer if needed */ - new_size = d->pos + buf_size; + new_size = (unsigned)d->pos + buf_size; new_allocated_size = d->allocated_size; if (new_size < d->pos || new_size > INT_MAX/2) return -1;
If adding two ints overflows, it doesn't matter whether the result will be stored in an unsigned or not; and checking afterwards does not make it retroactively defined. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/aviobuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)