| Message ID | 20220721115207.729615-3-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | migration: fix coverity nits | expand |
* Peter Maydell (peter.maydell@linaro.org) wrote: > When we use BLK_MIG_BLOCK_SIZE in expressions like > block_mig_state.submitted * BLK_MIG_BLOCK_SIZE, this multiplication > is done as 32 bits, because both operands are 32 bits. Coverity > complains about possible overflows because we then accumulate that > into a 64 bit variable. > > Define BLK_MIG_BLOCK_SIZE as unsigned long long using the ULL suffix. > The only two current uses of it with this problem are both in > block_save_pending(), so we could just cast to uint64_t there, but > using the ULL suffix is simpler and ensures that we don't > accidentally introduce new variants of the same issue in future. > > Resolves: Coverity CID 1487136, 1487175 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > I haven't tried to analyse the code to see if the multiplications > could ever actually end up overflowing, but I would assume > probably not. > > migration/block.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/migration/block.c b/migration/block.c > index 9e5aae58982..3577c815a94 100644 > --- a/migration/block.c > +++ b/migration/block.c > @@ -28,7 +28,7 @@ > #include "sysemu/block-backend.h" > #include "trace.h" > > -#define BLK_MIG_BLOCK_SIZE (1 << 20) > +#define BLK_MIG_BLOCK_SIZE (1ULL << 20) Is it a problem that this is passed to bdrv_create_dirty_bitmap that takes a uint32_t ? Dave > #define BDRV_SECTORS_PER_DIRTY_CHUNK (BLK_MIG_BLOCK_SIZE >> BDRV_SECTOR_BITS) > > #define BLK_MIG_FLAG_DEVICE_BLOCK 0x01 > -- > 2.25.1 >
On Thu, 21 Jul 2022 at 13:07, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > When we use BLK_MIG_BLOCK_SIZE in expressions like > > block_mig_state.submitted * BLK_MIG_BLOCK_SIZE, this multiplication > > is done as 32 bits, because both operands are 32 bits. Coverity > > complains about possible overflows because we then accumulate that > > into a 64 bit variable. > > > > Define BLK_MIG_BLOCK_SIZE as unsigned long long using the ULL suffix. > > The only two current uses of it with this problem are both in > > block_save_pending(), so we could just cast to uint64_t there, but > > using the ULL suffix is simpler and ensures that we don't > > accidentally introduce new variants of the same issue in future. > > > > Resolves: Coverity CID 1487136, 1487175 > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > I haven't tried to analyse the code to see if the multiplications > > could ever actually end up overflowing, but I would assume > > probably not. > > > > migration/block.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/migration/block.c b/migration/block.c > > index 9e5aae58982..3577c815a94 100644 > > --- a/migration/block.c > > +++ b/migration/block.c > > @@ -28,7 +28,7 @@ > > #include "sysemu/block-backend.h" > > #include "trace.h" > > > > -#define BLK_MIG_BLOCK_SIZE (1 << 20) > > +#define BLK_MIG_BLOCK_SIZE (1ULL << 20) > > Is it a problem that this is passed to bdrv_create_dirty_bitmap that > takes a uint32_t ? Shouldn't be -- the constant value still fits within 32 bits. -- PMM
* Peter Maydell (peter.maydell@linaro.org) wrote: > On Thu, 21 Jul 2022 at 13:07, Dr. David Alan Gilbert > <dgilbert@redhat.com> wrote: > > > > * Peter Maydell (peter.maydell@linaro.org) wrote: > > > When we use BLK_MIG_BLOCK_SIZE in expressions like > > > block_mig_state.submitted * BLK_MIG_BLOCK_SIZE, this multiplication > > > is done as 32 bits, because both operands are 32 bits. Coverity > > > complains about possible overflows because we then accumulate that > > > into a 64 bit variable. > > > > > > Define BLK_MIG_BLOCK_SIZE as unsigned long long using the ULL suffix. > > > The only two current uses of it with this problem are both in > > > block_save_pending(), so we could just cast to uint64_t there, but > > > using the ULL suffix is simpler and ensures that we don't > > > accidentally introduce new variants of the same issue in future. > > > > > > Resolves: Coverity CID 1487136, 1487175 > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > > --- > > > I haven't tried to analyse the code to see if the multiplications > > > could ever actually end up overflowing, but I would assume > > > probably not. > > > > > > migration/block.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/migration/block.c b/migration/block.c > > > index 9e5aae58982..3577c815a94 100644 > > > --- a/migration/block.c > > > +++ b/migration/block.c > > > @@ -28,7 +28,7 @@ > > > #include "sysemu/block-backend.h" > > > #include "trace.h" > > > > > > -#define BLK_MIG_BLOCK_SIZE (1 << 20) > > > +#define BLK_MIG_BLOCK_SIZE (1ULL << 20) > > > > Is it a problem that this is passed to bdrv_create_dirty_bitmap that > > takes a uint32_t ? > > Shouldn't be -- the constant value still fits within 32 bits. Hmm OK, lets keep an eye out for build problems on any odd combos Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > -- PMM >
Peter Maydell <peter.maydell@linaro.org> wrote: > When we use BLK_MIG_BLOCK_SIZE in expressions like > block_mig_state.submitted * BLK_MIG_BLOCK_SIZE, this multiplication > is done as 32 bits, because both operands are 32 bits. Coverity > complains about possible overflows because we then accumulate that > into a 64 bit variable. > > Define BLK_MIG_BLOCK_SIZE as unsigned long long using the ULL suffix. > The only two current uses of it with this problem are both in > block_save_pending(), so we could just cast to uint64_t there, but > using the ULL suffix is simpler and ensures that we don't > accidentally introduce new variants of the same issue in future. > > Resolves: Coverity CID 1487136, 1487175 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Juan Quintela <quintela@redhat.com>
diff --git a/migration/block.c b/migration/block.c index 9e5aae58982..3577c815a94 100644 --- a/migration/block.c +++ b/migration/block.c @@ -28,7 +28,7 @@ #include "sysemu/block-backend.h" #include "trace.h" -#define BLK_MIG_BLOCK_SIZE (1 << 20) +#define BLK_MIG_BLOCK_SIZE (1ULL << 20) #define BDRV_SECTORS_PER_DIRTY_CHUNK (BLK_MIG_BLOCK_SIZE >> BDRV_SECTOR_BITS) #define BLK_MIG_FLAG_DEVICE_BLOCK 0x01
When we use BLK_MIG_BLOCK_SIZE in expressions like block_mig_state.submitted * BLK_MIG_BLOCK_SIZE, this multiplication is done as 32 bits, because both operands are 32 bits. Coverity complains about possible overflows because we then accumulate that into a 64 bit variable. Define BLK_MIG_BLOCK_SIZE as unsigned long long using the ULL suffix. The only two current uses of it with this problem are both in block_save_pending(), so we could just cast to uint64_t there, but using the ULL suffix is simpler and ensures that we don't accidentally introduce new variants of the same issue in future. Resolves: Coverity CID 1487136, 1487175 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- I haven't tried to analyse the code to see if the multiplications could ever actually end up overflowing, but I would assume probably not. migration/block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)