On 6/8/23 03:57, Sam Li wrote: > If the write operation fails and the wps is NULL, then accessing it will > lead to data corruption. > > Solving the issue by adding a nullptr checking in get_zones_wp() where > the wps is used. > > This issue is found by Peter Maydell using the Coverity Tool (CID > 1512459). > > Signed-off-by: Sam Li <faithilike...@gmail.com> > --- > block/file-posix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index ac1ed54811..4a6c71c7f5 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -2523,7 +2523,7 @@ out: > } > } > } else { > - if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) { > + if (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND) && wps) { > update_zones_wp(bs, s->fd, 0, 1);
Nit: this could be: } else if (wps && type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND)) { However, both if & else side do something only if the above condition is true and we only need to that for a zoned drive. So the entire code block could really be simplified to be a lot more readable. Something like this (totally untested, not even compiled): #if defined(CONFIG_BLKZONED) if (bs->bl.zone_size && (type & (QEMU_AIO_WRITE | QEMU_AIO_ZONE_APPEND))) { BlockZoneWps *wps = bs->wps; uint64_t *wp; if (!wps) { return ret; } if (ret) { /* write error: update the wp from the underlying device */ update_zones_wp(bs, s->fd, 0, 1); goto unlock; } wp = &wps->wp[offset / bs->bl.zone_size]; if (BDRV_ZT_IS_CONV(*wp)) { /* Conventional zones do not have a write pointer */ goto unlock; } /* Return the written position for zone append */ if (type & QEMU_AIO_ZONE_APPEND) { *s->offset = *wp; trace_zbd_zone_append_complete(bs, *s->offset >> BDRV_SECTOR_BITS); } /* Advance the wp if needed */ if (offset + bytes > *wp) { *wp = offset + bytes; } unlock: qemu_co_mutex_unlock(&wps->colock); } #endif And making this entire block a helper function (e.g. advance_zone_wp()) would further clean the code. But that should be done in another patch. Care to send one ? -- Damien Le Moal Western Digital Research