Message ID | 1431653951-28178-2-git-send-email-famz@redhat.com |
---|---|
State | New |
Headers | show |
On Fri 15 May 2015 03:39:10 AM CEST, Fam Zheng <famz@redhat.com> wrote: > int64_t ret = bdrv_nb_sectors(bs); > > + ret = (int64_t)(ret * BDRV_SECTOR_SIZE) < 0 ? -EFBIG : ret; > return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE; Maybe in this case you're safe, but in general there's no guarantee that if there's an overflow the result will be negative. You can do something like this instead: ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret; Of course this is only valid if BDRV_SECTOR_SIZE != 0 ;) Berto
Fam Zheng <famz@redhat.com> writes: > Bogus image may have a large total_sectors that will overflow the > multiplication. For cleanness, fix the return code so the error message > will be meaningful. > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/block.c b/block.c > index 7904098..5ee3fdf 100644 > --- a/block.c > +++ b/block.c > @@ -2330,6 +2330,7 @@ int64_t bdrv_getlength(BlockDriverState *bs) > { > int64_t ret = bdrv_nb_sectors(bs); > > + ret = (int64_t)(ret * BDRV_SECTOR_SIZE) < 0 ? -EFBIG : ret; > return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE; > } Signed integer overflow is undefined behavior. Your code works just fine on any remotely sane machine, *except* when the optimizer decides to use its undefined behavior license to mess with you. A more prudent way to test for overflow would be something like ret > INT64_MAX / BDRV_SECTOR_SIZE
On Fri, 05/15 10:10, Markus Armbruster wrote: > Fam Zheng <famz@redhat.com> writes: > > > Bogus image may have a large total_sectors that will overflow the > > multiplication. For cleanness, fix the return code so the error message > > will be meaningful. > > > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/block.c b/block.c > > index 7904098..5ee3fdf 100644 > > --- a/block.c > > +++ b/block.c > > @@ -2330,6 +2330,7 @@ int64_t bdrv_getlength(BlockDriverState *bs) > > { > > int64_t ret = bdrv_nb_sectors(bs); > > > > + ret = (int64_t)(ret * BDRV_SECTOR_SIZE) < 0 ? -EFBIG : ret; > > return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE; > > } > > Signed integer overflow is undefined behavior. Your code works just > fine on any remotely sane machine, *except* when the optimizer decides > to use its undefined behavior license to mess with you. > > A more prudent way to test for overflow would be something like > > ret > INT64_MAX / BDRV_SECTOR_SIZE Yes, this is better, will fix. Thanks, Fam
diff --git a/block.c b/block.c index 7904098..5ee3fdf 100644 --- a/block.c +++ b/block.c @@ -2330,6 +2330,7 @@ int64_t bdrv_getlength(BlockDriverState *bs) { int64_t ret = bdrv_nb_sectors(bs); + ret = (int64_t)(ret * BDRV_SECTOR_SIZE) < 0 ? -EFBIG : ret; return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE; }
Bogus image may have a large total_sectors that will overflow the multiplication. For cleanness, fix the return code so the error message will be meaningful. Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 1 + 1 file changed, 1 insertion(+)