| Message ID | 20231017203855.298260-1-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Fix parse_ramblock() on overwritten retvals | expand |
Peter Xu <peterx@redhat.com> writes: > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. > > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Fabiano Rosas <farosas@suse.de>
Peter Xu <peterx@redhat.com> wrote: > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. > > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/ram.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index c844151ee9..d8bdb53a8f 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > ret = qemu_ram_resize(block, length, &local_err); > if (local_err) { > error_report_err(local_err); > + assert(ret < 0); > + return ret; I hate that assert. If you really want that: if (ret < 0) { error_report_err(local_err); assert(ret < 0); return ret; } Rest of the patch looks ok. Later, Juan.
On Wed, Oct 18, 2023 at 09:12:36AM +0200, Juan Quintela wrote: > Peter Xu <peterx@redhat.com> wrote: > > It's possible that some errors can be overwritten with success retval later > > on, and then ignored. Always capture all errors and report. > > > > Reported by Coverity 1522861, but actually I spot one more in the same > > function. > > > > Fixes: CID 1522861 > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/ram.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/migration/ram.c b/migration/ram.c > > index c844151ee9..d8bdb53a8f 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > > ret = qemu_ram_resize(block, length, &local_err); > > if (local_err) { > > error_report_err(local_err); > > + assert(ret < 0); > > + return ret; > > I hate that assert. If you really want that: Please have a look at qemu_ram_resize(). It only contains two error paths. > > > if (ret < 0) { > error_report_err(local_err); This will be similar to above, if qemu_ram_resize() return <0 with err==NULL, it'll crash in error_report_err() too.. at error_get_pretty(). > assert(ret < 0); This is not necessary.. if in this "if" section. So we can drop it (instead of assert it). > return ret; > } > > Rest of the patch looks ok. I tend to prefer just merging this.. but if you strongly prefer the other way, I can drop the assert(). But then I'll prefer "return -EINVAL" rather than "return ret", if you're fine with it. Thanks,
Peter Xu <peterx@redhat.com> wrote: > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. > > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@redhat.com> queued.
On Tue, 17 Oct 2023 at 21:40, Peter Xu <peterx@redhat.com> wrote: > > It's possible that some errors can be overwritten with success retval later > on, and then ignored. Always capture all errors and report. > > Reported by Coverity 1522861, but actually I spot one more in the same > function. The other one is CID 1522862, I think. > Fixes: CID 1522861 > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/ram.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/migration/ram.c b/migration/ram.c > index c844151ee9..d8bdb53a8f 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > ret = qemu_ram_resize(block, length, &local_err); > if (local_err) { > error_report_err(local_err); > + assert(ret < 0); We usually don't bother asserting for this kind of "function reports errors two ways" code. > + return ret; > } thanks -- PMM
On Thu, Oct 19, 2023 at 01:40:29PM +0100, Peter Maydell wrote: > On Tue, 17 Oct 2023 at 21:40, Peter Xu <peterx@redhat.com> wrote: > > > > It's possible that some errors can be overwritten with success retval later > > on, and then ignored. Always capture all errors and report. > > > > Reported by Coverity 1522861, but actually I spot one more in the same > > function. > > The other one is CID 1522862, I think. Yes.. > > > Fixes: CID 1522861 > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > --- > > migration/ram.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/migration/ram.c b/migration/ram.c > > index c844151ee9..d8bdb53a8f 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) > > ret = qemu_ram_resize(block, length, &local_err); > > if (local_err) { > > error_report_err(local_err); > > + assert(ret < 0); > > We usually don't bother asserting for this kind of "function > reports errors two ways" code. Juan, please feel free to drop the assert() if it's in the queue. After this one lands, I'll send a patch to remove qemu_ram_resize retval and only rely on Error*. Thanks,
diff --git a/migration/ram.c b/migration/ram.c index c844151ee9..d8bdb53a8f 100644 --- a/migration/ram.c +++ b/migration/ram.c @@ -3888,6 +3888,8 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) ret = qemu_ram_resize(block, length, &local_err); if (local_err) { error_report_err(local_err); + assert(ret < 0); + return ret; } } /* For postcopy we need to check hugepage sizes match */ @@ -3898,7 +3900,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) error_report("Mismatched RAM page size %s " "(local) %zd != %" PRId64, block->idstr, block->page_size, remote_page_size); - ret = -EINVAL; + return -EINVAL; } } if (migrate_ignore_shared()) { @@ -3908,7 +3910,7 @@ static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length) error_report("Mismatched GPAs for block %s " "%" PRId64 "!= %" PRId64, block->idstr, (uint64_t)addr, (uint64_t)block->mr->addr); - ret = -EINVAL; + return -EINVAL; } } ret = rdma_block_notification_handle(f, block->idstr);
It's possible that some errors can be overwritten with success retval later on, and then ignored. Always capture all errors and report. Reported by Coverity 1522861, but actually I spot one more in the same function. Fixes: CID 1522861 Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/ram.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)