[PATCH 0/2] migration: fix coverity nits

[PATCH 0/2] migration: fix coverity nits

[Peter Maydell]

This patchset fixes four Coverity nits in the migration code.
The first patch is just adding an assert() to clue coverity in
that an array index must be in-bounds. The second adds an ULL
suffix to force a multiplication to be done at 64 bits.

thanks
-- PMM

Peter Maydell (2):
  migration: Assert that migrate_multifd_compression() returns an
    in-range value
  migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

 migration/block.c     | 2 +-
 migration/migration.c | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value

[Peter Maydell]

Coverity complains that when we use the return value from
migrate_multifd_compression() as an array index:
  multifd_recv_state->ops = multifd_ops[migrate_multifd_compression()];

that this might overrun the array (which is declared to have size
MULTIFD_COMPRESSION__MAX).  This is because the function return type
is MultiFDCompression, which is an autogenerated enum.  The code
generator includes the "one greater than the maximum possible value"
MULTIFD_COMPRESSION__MAX in the enum, even though this is not
actually a valid value for the enum, and this makes Coverity think
that migrate_multifd_compression() could return that __MAX value and
index off the end of the array.

Suppress the Coverity error by asserting that the value we're going
to return is within range.

Resolves: Coverity CID 1487239, 1487254
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 migration/migration.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/migration/migration.c b/migration/migration.c
index e03f698a3ca..befd4c58a69 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2617,6 +2617,7 @@ MultiFDCompression migrate_multifd_compression(void)
 
     s = migrate_get_current();
 
+    assert(s->parameters.multifd_compression < MULTIFD_COMPRESSION__MAX);
     return s->parameters.multifd_compression;
 }
 
-- 
2.25.1

Re: [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> Coverity complains that when we use the return value from
> migrate_multifd_compression() as an array index:
>   multifd_recv_state->ops = multifd_ops[migrate_multifd_compression()];
> 
> that this might overrun the array (which is declared to have size
> MULTIFD_COMPRESSION__MAX).  This is because the function return type
> is MultiFDCompression, which is an autogenerated enum.  The code
> generator includes the "one greater than the maximum possible value"
> MULTIFD_COMPRESSION__MAX in the enum, even though this is not
> actually a valid value for the enum, and this makes Coverity think
> that migrate_multifd_compression() could return that __MAX value and
> index off the end of the array.
> 
> Suppress the Coverity error by asserting that the value we're going
> to return is within range.
> 
> Resolves: Coverity CID 1487239, 1487254
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index e03f698a3ca..befd4c58a69 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2617,6 +2617,7 @@ MultiFDCompression migrate_multifd_compression(void)
>  
>      s = migrate_get_current();
>  
> +    assert(s->parameters.multifd_compression < MULTIFD_COMPRESSION__MAX);
>      return s->parameters.multifd_compression;
>  }
>  
> -- 
> 2.25.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value

[Juan Quintela]

Peter Maydell <peter.maydell@linaro.org> wrote:
> Coverity complains that when we use the return value from
> migrate_multifd_compression() as an array index:
>   multifd_recv_state->ops = multifd_ops[migrate_multifd_compression()];
>
> that this might overrun the array (which is declared to have size
> MULTIFD_COMPRESSION__MAX).  This is because the function return type
> is MultiFDCompression, which is an autogenerated enum.  The code
> generator includes the "one greater than the maximum possible value"
> MULTIFD_COMPRESSION__MAX in the enum, even though this is not
> actually a valid value for the enum, and this makes Coverity think
> that migrate_multifd_compression() could return that __MAX value and
> index off the end of the array.
>
> Suppress the Coverity error by asserting that the value we're going
> to return is within range.
>
> Resolves: Coverity CID 1487239, 1487254
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

[Peter Maydell]

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)
 #define BDRV_SECTORS_PER_DIRTY_CHUNK (BLK_MIG_BLOCK_SIZE >> BDRV_SECTOR_BITS)
 
 #define BLK_MIG_FLAG_DEVICE_BLOCK       0x01
-- 
2.25.1

Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

[Dr. David Alan Gilbert]

* 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

[Peter Maydell]

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

Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

[Dr. David Alan Gilbert]

* 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
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

[Juan Quintela]

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>

Re: [PATCH 0/2] migration: fix coverity nits

[Peter Maydell]

On Thu, 21 Jul 2022 at 12:52, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> This patchset fixes four Coverity nits in the migration code.
> The first patch is just adding an assert() to clue coverity in
> that an array index must be in-bounds. The second adds an ULL
> suffix to force a multiplication to be done at 64 bits.
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   migration: Assert that migrate_multifd_compression() returns an
>     in-range value
>   migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long

Ping? This series got reviewed but doesn't seem to have
made it into a migration pullreq yet.

thanks
-- PMM

Re: [PATCH 0/2] migration: fix coverity nits

[Dr. David Alan Gilbert]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Thu, 21 Jul 2022 at 12:52, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > This patchset fixes four Coverity nits in the migration code.
> > The first patch is just adding an assert() to clue coverity in
> > that an array index must be in-bounds. The second adds an ULL
> > suffix to force a multiplication to be done at 64 bits.
> >
> > thanks
> > -- PMM
> >
> > Peter Maydell (2):
> >   migration: Assert that migrate_multifd_compression() returns an
> >     in-range value
> >   migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long
> 
> Ping? This series got reviewed but doesn't seem to have
> made it into a migration pullreq yet.

Queued

> thanks
> -- PMM
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK