* [PATCH 0/2] migration: fix coverity nits
@ 2022-07-21 11:52 Peter Maydell
2022-07-21 11:52 ` [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value Peter Maydell
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Peter Maydell @ 2022-07-21 11:52 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Dr. David Alan Gilbert, Juan Quintela, Fam Zheng,
Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value
2022-07-21 11:52 [PATCH 0/2] migration: fix coverity nits Peter Maydell
@ 2022-07-21 11:52 ` Peter Maydell
2022-07-21 12:02 ` Dr. David Alan Gilbert
2022-07-22 11:00 ` Juan Quintela
2022-07-21 11:52 ` [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long Peter Maydell
2022-08-01 10:38 ` [PATCH 0/2] migration: fix coverity nits Peter Maydell
2 siblings, 2 replies; 11+ messages in thread
From: Peter Maydell @ 2022-07-21 11:52 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Dr. David Alan Gilbert, Juan Quintela, Fam Zheng,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long
2022-07-21 11:52 [PATCH 0/2] migration: fix coverity nits Peter Maydell
2022-07-21 11:52 ` [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value Peter Maydell
@ 2022-07-21 11:52 ` Peter Maydell
2022-07-21 12:07 ` Dr. David Alan Gilbert
2022-07-22 12:47 ` Juan Quintela
2022-08-01 10:38 ` [PATCH 0/2] migration: fix coverity nits Peter Maydell
2 siblings, 2 replies; 11+ messages in thread
From: Peter Maydell @ 2022-07-21 11:52 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Dr. David Alan Gilbert, Juan Quintela, Fam Zheng,
Stefan Hajnoczi
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value
2022-07-21 11:52 ` [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value Peter Maydell
@ 2022-07-21 12:02 ` Dr. David Alan Gilbert
2022-07-22 11:00 ` Juan Quintela
1 sibling, 0 replies; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2022-07-21 12:02 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-block, Juan Quintela, Fam Zheng, Stefan Hajnoczi
* 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long
2022-07-21 11:52 ` [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long Peter Maydell
@ 2022-07-21 12:07 ` Dr. David Alan Gilbert
2022-07-21 12:44 ` Peter Maydell
2022-07-22 12:47 ` Juan Quintela
1 sibling, 1 reply; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2022-07-21 12:07 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-block, Juan Quintela, Fam Zheng, Stefan Hajnoczi
* 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long
2022-07-21 12:07 ` Dr. David Alan Gilbert
@ 2022-07-21 12:44 ` Peter Maydell
2022-07-21 13:06 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2022-07-21 12:44 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: qemu-devel, qemu-block, Juan Quintela, Fam Zheng, Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long
2022-07-21 12:44 ` Peter Maydell
@ 2022-07-21 13:06 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2022-07-21 13:06 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-block, Juan Quintela, Fam Zheng, Stefan Hajnoczi
* 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value
2022-07-21 11:52 ` [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value Peter Maydell
2022-07-21 12:02 ` Dr. David Alan Gilbert
@ 2022-07-22 11:00 ` Juan Quintela
1 sibling, 0 replies; 11+ messages in thread
From: Juan Quintela @ 2022-07-22 11:00 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-block, Dr. David Alan Gilbert, Fam Zheng,
Stefan Hajnoczi
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long
2022-07-21 11:52 ` [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long Peter Maydell
2022-07-21 12:07 ` Dr. David Alan Gilbert
@ 2022-07-22 12:47 ` Juan Quintela
1 sibling, 0 replies; 11+ messages in thread
From: Juan Quintela @ 2022-07-22 12:47 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-block, Dr. David Alan Gilbert, Fam Zheng,
Stefan Hajnoczi
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] migration: fix coverity nits
2022-07-21 11:52 [PATCH 0/2] migration: fix coverity nits Peter Maydell
2022-07-21 11:52 ` [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value Peter Maydell
2022-07-21 11:52 ` [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long Peter Maydell
@ 2022-08-01 10:38 ` Peter Maydell
2022-08-02 13:49 ` Dr. David Alan Gilbert
2 siblings, 1 reply; 11+ messages in thread
From: Peter Maydell @ 2022-08-01 10:38 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-block, Dr. David Alan Gilbert, Juan Quintela, Fam Zheng,
Stefan Hajnoczi
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] migration: fix coverity nits
2022-08-01 10:38 ` [PATCH 0/2] migration: fix coverity nits Peter Maydell
@ 2022-08-02 13:49 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 11+ messages in thread
From: Dr. David Alan Gilbert @ 2022-08-02 13:49 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, qemu-block, Juan Quintela, Fam Zheng, Stefan Hajnoczi
* 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-08-02 13:59 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-21 11:52 [PATCH 0/2] migration: fix coverity nits Peter Maydell
2022-07-21 11:52 ` [PATCH 1/2] migration: Assert that migrate_multifd_compression() returns an in-range value Peter Maydell
2022-07-21 12:02 ` Dr. David Alan Gilbert
2022-07-22 11:00 ` Juan Quintela
2022-07-21 11:52 ` [PATCH 2/2] migration: Define BLK_MIG_BLOCK_SIZE as unsigned long long Peter Maydell
2022-07-21 12:07 ` Dr. David Alan Gilbert
2022-07-21 12:44 ` Peter Maydell
2022-07-21 13:06 ` Dr. David Alan Gilbert
2022-07-22 12:47 ` Juan Quintela
2022-08-01 10:38 ` [PATCH 0/2] migration: fix coverity nits Peter Maydell
2022-08-02 13:49 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.