Am 13.01.2023 um 08:30 hat Philippe Mathieu-Daudé geschrieben: > On 12/1/23 20:14, Kevin Wolf wrote: > > In order to write the bitmap table to the image file, it is converted to > > big endian. If the write fails, it is passed to clear_bitmap_table() to > > free all of the clusters it had allocated before. However, if we don't > > convert it back to native endianness first, we'll free things at a wrong > > offset. > > > > In practical terms, the offsets will be so high that we won't actually > > free any allocated clusters, but just run into an error, but in theory > > this can cause image corruption. > > > > Cc: qemu-sta...@nongnu.org > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > > --- > > block/qcow2-bitmap.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c > > index bcad567c0c..3dff99ba06 100644 > > --- a/block/qcow2-bitmap.c > > +++ b/block/qcow2-bitmap.c > > @@ -115,7 +115,7 @@ static int update_header_sync(BlockDriverState *bs) > > return bdrv_flush(bs->file->bs); > > } > > Maybe add a comment here remembering to bswap back to native endianness? > > > -static inline void bitmap_table_to_be(uint64_t *bitmap_table, size_t size) > > +static inline void bitmap_table_bswap_be(uint64_t *bitmap_table, size_t > > size) > > { > > This function uses cpu_to_be64(), semantically we convert back calling > be64_to_cpu(), but technically both functions end up being the same.
Yes, but we don't seem to have any public "neutral" functions, it's always either from or to. > Alternatively: > > for (i = 0; i < size; ++i) { > - bitmap_table[i] = cpu_to_be64(bitmap_table[i]); > + bswap64s(&bitmap_table[i]); > } Doesn't that swap even on big endian hosts, resulting incorrectly in a little endian table? The closest thing we have that I can see is the be_bswap() macro in bswap.h, but it's undefined again at the end of the header. > > @@ -1401,9 +1401,10 @@ static int store_bitmap(BlockDriverState *bs, > > Qcow2Bitmap *bm, Error **errp) > > goto fail; > > } > > - bitmap_table_to_be(tb, tb_size); > > + bitmap_table_bswap_be(tb, tb_size); > > ret = bdrv_pwrite(bs->file, tb_offset, tb_size * sizeof(tb[0]), tb, > > 0); > > if (ret < 0) { > > + bitmap_table_bswap_be(tb, tb_size); > > error_setg_errno(errp, -ret, "Failed to write bitmap '%s' to > > file", > > bm_name); > > goto fail; > > Pre-existing, but consider using g_autofree for 'tb'. > > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org> Thanks! Kevin