* [Qemu-devel] [PATCH] block/vhdx: Don't take address of fields in packed structs
@ 2018-10-16 17:09 Peter Maydell
2018-10-17 9:32 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2018-10-17 14:36 ` [Qemu-devel] " Kevin Wolf
0 siblings, 2 replies; 4+ messages in thread
From: Peter Maydell @ 2018-10-16 17:09 UTC (permalink / raw)
To: qemu-devel; +Cc: patches, Jeff Cody, Kevin Wolf, Max Reitz, qemu-block
Taking the address of a field in a packed struct is a bad idea, because
it might not be actually aligned enough for that pointer type (and
thus cause a crash on dereference on some host architectures). Newer
versions of clang warn about this. Avoid the bug by not using the
"modify in place" byte swapping functions.
There are a few places where the in-place swap function is
used on something other than a packed struct field; we convert
those anyway, for consistency.
Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Usual disclaimer: produced with "make check" only, but purely
automated conversion should be safe.
block/vhdx.h | 12 ++---
block/vhdx-endian.c | 118 ++++++++++++++++++++++----------------------
block/vhdx-log.c | 4 +-
block/vhdx.c | 18 +++----
4 files changed, 76 insertions(+), 76 deletions(-)
diff --git a/block/vhdx.h b/block/vhdx.h
index 7003ab7a795..3a5f5293adc 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -420,16 +420,16 @@ int vhdx_log_write_and_flush(BlockDriverState *bs, BDRVVHDXState *s,
static inline void leguid_to_cpus(MSGUID *guid)
{
- le32_to_cpus(&guid->data1);
- le16_to_cpus(&guid->data2);
- le16_to_cpus(&guid->data3);
+ guid->data1 = le32_to_cpu(guid->data1);
+ guid->data2 = le16_to_cpu(guid->data2);
+ guid->data3 = le16_to_cpu(guid->data3);
}
static inline void cpu_to_leguids(MSGUID *guid)
{
- cpu_to_le32s(&guid->data1);
- cpu_to_le16s(&guid->data2);
- cpu_to_le16s(&guid->data3);
+ guid->data1 = cpu_to_le32(guid->data1);
+ guid->data2 = cpu_to_le16(guid->data2);
+ guid->data3 = cpu_to_le16(guid->data3);
}
void vhdx_header_le_import(VHDXHeader *h);
diff --git a/block/vhdx-endian.c b/block/vhdx-endian.c
index 41fbdd2b8f0..ebfa33cb8a4 100644
--- a/block/vhdx-endian.c
+++ b/block/vhdx-endian.c
@@ -35,18 +35,18 @@ void vhdx_header_le_import(VHDXHeader *h)
{
assert(h != NULL);
- le32_to_cpus(&h->signature);
- le32_to_cpus(&h->checksum);
- le64_to_cpus(&h->sequence_number);
+ h->signature = le32_to_cpu(h->signature);
+ h->checksum = le32_to_cpu(h->checksum);
+ h->sequence_number = le64_to_cpu(h->sequence_number);
leguid_to_cpus(&h->file_write_guid);
leguid_to_cpus(&h->data_write_guid);
leguid_to_cpus(&h->log_guid);
- le16_to_cpus(&h->log_version);
- le16_to_cpus(&h->version);
- le32_to_cpus(&h->log_length);
- le64_to_cpus(&h->log_offset);
+ h->log_version = le16_to_cpu(h->log_version);
+ h->version = le16_to_cpu(h->version);
+ h->log_length = le32_to_cpu(h->log_length);
+ h->log_offset = le64_to_cpu(h->log_offset);
}
void vhdx_header_le_export(VHDXHeader *orig_h, VHDXHeader *new_h)
@@ -80,68 +80,68 @@ void vhdx_log_desc_le_import(VHDXLogDescriptor *d)
{
assert(d != NULL);
- le32_to_cpus(&d->signature);
- le64_to_cpus(&d->file_offset);
- le64_to_cpus(&d->sequence_number);
+ d->signature = le32_to_cpu(d->signature);
+ d->file_offset = le64_to_cpu(d->file_offset);
+ d->sequence_number = le64_to_cpu(d->sequence_number);
}
void vhdx_log_desc_le_export(VHDXLogDescriptor *d)
{
assert(d != NULL);
- cpu_to_le32s(&d->signature);
- cpu_to_le32s(&d->trailing_bytes);
- cpu_to_le64s(&d->leading_bytes);
- cpu_to_le64s(&d->file_offset);
- cpu_to_le64s(&d->sequence_number);
+ d->signature = cpu_to_le32(d->signature);
+ d->trailing_bytes = cpu_to_le32(d->trailing_bytes);
+ d->leading_bytes = cpu_to_le64(d->leading_bytes);
+ d->file_offset = cpu_to_le64(d->file_offset);
+ d->sequence_number = cpu_to_le64(d->sequence_number);
}
void vhdx_log_data_le_import(VHDXLogDataSector *d)
{
assert(d != NULL);
- le32_to_cpus(&d->data_signature);
- le32_to_cpus(&d->sequence_high);
- le32_to_cpus(&d->sequence_low);
+ d->data_signature = le32_to_cpu(d->data_signature);
+ d->sequence_high = le32_to_cpu(d->sequence_high);
+ d->sequence_low = le32_to_cpu(d->sequence_low);
}
void vhdx_log_data_le_export(VHDXLogDataSector *d)
{
assert(d != NULL);
- cpu_to_le32s(&d->data_signature);
- cpu_to_le32s(&d->sequence_high);
- cpu_to_le32s(&d->sequence_low);
+ d->data_signature = cpu_to_le32(d->data_signature);
+ d->sequence_high = cpu_to_le32(d->sequence_high);
+ d->sequence_low = cpu_to_le32(d->sequence_low);
}
void vhdx_log_entry_hdr_le_import(VHDXLogEntryHeader *hdr)
{
assert(hdr != NULL);
- le32_to_cpus(&hdr->signature);
- le32_to_cpus(&hdr->checksum);
- le32_to_cpus(&hdr->entry_length);
- le32_to_cpus(&hdr->tail);
- le64_to_cpus(&hdr->sequence_number);
- le32_to_cpus(&hdr->descriptor_count);
+ hdr->signature = le32_to_cpu(hdr->signature);
+ hdr->checksum = le32_to_cpu(hdr->checksum);
+ hdr->entry_length = le32_to_cpu(hdr->entry_length);
+ hdr->tail = le32_to_cpu(hdr->tail);
+ hdr->sequence_number = le64_to_cpu(hdr->sequence_number);
+ hdr->descriptor_count = le32_to_cpu(hdr->descriptor_count);
leguid_to_cpus(&hdr->log_guid);
- le64_to_cpus(&hdr->flushed_file_offset);
- le64_to_cpus(&hdr->last_file_offset);
+ hdr->flushed_file_offset = le64_to_cpu(hdr->flushed_file_offset);
+ hdr->last_file_offset = le64_to_cpu(hdr->last_file_offset);
}
void vhdx_log_entry_hdr_le_export(VHDXLogEntryHeader *hdr)
{
assert(hdr != NULL);
- cpu_to_le32s(&hdr->signature);
- cpu_to_le32s(&hdr->checksum);
- cpu_to_le32s(&hdr->entry_length);
- cpu_to_le32s(&hdr->tail);
- cpu_to_le64s(&hdr->sequence_number);
- cpu_to_le32s(&hdr->descriptor_count);
+ hdr->signature = cpu_to_le32(hdr->signature);
+ hdr->checksum = cpu_to_le32(hdr->checksum);
+ hdr->entry_length = cpu_to_le32(hdr->entry_length);
+ hdr->tail = cpu_to_le32(hdr->tail);
+ hdr->sequence_number = cpu_to_le64(hdr->sequence_number);
+ hdr->descriptor_count = cpu_to_le32(hdr->descriptor_count);
cpu_to_leguids(&hdr->log_guid);
- cpu_to_le64s(&hdr->flushed_file_offset);
- cpu_to_le64s(&hdr->last_file_offset);
+ hdr->flushed_file_offset = cpu_to_le64(hdr->flushed_file_offset);
+ hdr->last_file_offset = cpu_to_le64(hdr->last_file_offset);
}
@@ -150,18 +150,18 @@ void vhdx_region_header_le_import(VHDXRegionTableHeader *hdr)
{
assert(hdr != NULL);
- le32_to_cpus(&hdr->signature);
- le32_to_cpus(&hdr->checksum);
- le32_to_cpus(&hdr->entry_count);
+ hdr->signature = le32_to_cpu(hdr->signature);
+ hdr->checksum = le32_to_cpu(hdr->checksum);
+ hdr->entry_count = le32_to_cpu(hdr->entry_count);
}
void vhdx_region_header_le_export(VHDXRegionTableHeader *hdr)
{
assert(hdr != NULL);
- cpu_to_le32s(&hdr->signature);
- cpu_to_le32s(&hdr->checksum);
- cpu_to_le32s(&hdr->entry_count);
+ hdr->signature = cpu_to_le32(hdr->signature);
+ hdr->checksum = cpu_to_le32(hdr->checksum);
+ hdr->entry_count = cpu_to_le32(hdr->entry_count);
}
void vhdx_region_entry_le_import(VHDXRegionTableEntry *e)
@@ -169,9 +169,9 @@ void vhdx_region_entry_le_import(VHDXRegionTableEntry *e)
assert(e != NULL);
leguid_to_cpus(&e->guid);
- le64_to_cpus(&e->file_offset);
- le32_to_cpus(&e->length);
- le32_to_cpus(&e->data_bits);
+ e->file_offset = le64_to_cpu(e->file_offset);
+ e->length = le32_to_cpu(e->length);
+ e->data_bits = le32_to_cpu(e->data_bits);
}
void vhdx_region_entry_le_export(VHDXRegionTableEntry *e)
@@ -179,9 +179,9 @@ void vhdx_region_entry_le_export(VHDXRegionTableEntry *e)
assert(e != NULL);
cpu_to_leguids(&e->guid);
- cpu_to_le64s(&e->file_offset);
- cpu_to_le32s(&e->length);
- cpu_to_le32s(&e->data_bits);
+ e->file_offset = cpu_to_le64(e->file_offset);
+ e->length = cpu_to_le32(e->length);
+ e->data_bits = cpu_to_le32(e->data_bits);
}
@@ -190,16 +190,16 @@ void vhdx_metadata_header_le_import(VHDXMetadataTableHeader *hdr)
{
assert(hdr != NULL);
- le64_to_cpus(&hdr->signature);
- le16_to_cpus(&hdr->entry_count);
+ hdr->signature = le64_to_cpu(hdr->signature);
+ hdr->entry_count = le16_to_cpu(hdr->entry_count);
}
void vhdx_metadata_header_le_export(VHDXMetadataTableHeader *hdr)
{
assert(hdr != NULL);
- cpu_to_le64s(&hdr->signature);
- cpu_to_le16s(&hdr->entry_count);
+ hdr->signature = cpu_to_le64(hdr->signature);
+ hdr->entry_count = cpu_to_le16(hdr->entry_count);
}
void vhdx_metadata_entry_le_import(VHDXMetadataTableEntry *e)
@@ -207,16 +207,16 @@ void vhdx_metadata_entry_le_import(VHDXMetadataTableEntry *e)
assert(e != NULL);
leguid_to_cpus(&e->item_id);
- le32_to_cpus(&e->offset);
- le32_to_cpus(&e->length);
- le32_to_cpus(&e->data_bits);
+ e->offset = le32_to_cpu(e->offset);
+ e->length = le32_to_cpu(e->length);
+ e->data_bits = le32_to_cpu(e->data_bits);
}
void vhdx_metadata_entry_le_export(VHDXMetadataTableEntry *e)
{
assert(e != NULL);
cpu_to_leguids(&e->item_id);
- cpu_to_le32s(&e->offset);
- cpu_to_le32s(&e->length);
- cpu_to_le32s(&e->data_bits);
+ e->offset = cpu_to_le32(e->offset);
+ e->length = cpu_to_le32(e->length);
+ e->data_bits = cpu_to_le32(e->data_bits);
}
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index d2f1b98199c..ecd64266c5e 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -835,11 +835,11 @@ static void vhdx_log_raw_to_le_sector(VHDXLogDescriptor *desc,
/* 8 + 4084 + 4 = 4096, 1 log sector */
memcpy(&desc->leading_bytes, data, 8);
data += 8;
- cpu_to_le64s(&desc->leading_bytes);
+ desc->leading_bytes = cpu_to_le64(desc->leading_bytes);
memcpy(sector->data, data, 4084);
data += 4084;
memcpy(&desc->trailing_bytes, data, 4);
- cpu_to_le32s(&desc->trailing_bytes);
+ desc->trailing_bytes = cpu_to_le32(desc->trailing_bytes);
data += 4;
sector->sequence_high = (uint32_t) (seq >> 32);
diff --git a/block/vhdx.c b/block/vhdx.c
index 0795ca19857..b785aef4b7b 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -156,7 +156,7 @@ uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset)
memset(buf + crc_offset, 0, sizeof(crc));
crc = crc32c(0xffffffff, buf, size);
- cpu_to_le32s(&crc);
+ crc = cpu_to_le32(crc);
memcpy(buf + crc_offset, &crc, sizeof(crc));
return crc;
@@ -753,8 +753,8 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
goto exit;
}
- le32_to_cpus(&s->params.block_size);
- le32_to_cpus(&s->params.data_bits);
+ s->params.block_size = le32_to_cpu(s->params.block_size);
+ s->params.data_bits = le32_to_cpu(s->params.data_bits);
/* We now have the file parameters, so we can tell if this is a
@@ -803,9 +803,9 @@ static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
goto exit;
}
- le64_to_cpus(&s->virtual_disk_size);
- le32_to_cpus(&s->logical_sector_size);
- le32_to_cpus(&s->physical_sector_size);
+ s->virtual_disk_size = le64_to_cpu(s->virtual_disk_size);
+ s->logical_sector_size = le32_to_cpu(s->logical_sector_size);
+ s->physical_sector_size = le32_to_cpu(s->physical_sector_size);
if (s->params.block_size < VHDX_BLOCK_SIZE_MIN ||
s->params.block_size > VHDX_BLOCK_SIZE_MAX) {
@@ -985,7 +985,7 @@ static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
/* endian convert, and verify populated BAT field file offsets against
* region table and log entries */
for (i = 0; i < s->bat_entries; i++) {
- le64_to_cpus(&s->bat[i]);
+ s->bat[i] = le64_to_cpu(s->bat[i]);
if (payblocks--) {
/* payload bat entries */
if ((s->bat[i] & VHDX_BAT_STATE_BIT_MASK) ==
@@ -1509,7 +1509,7 @@ static int vhdx_create_new_metadata(BlockBackend *blk,
mt_file_params->block_size = cpu_to_le32(block_size);
if (type == VHDX_TYPE_FIXED) {
mt_file_params->data_bits |= VHDX_PARAMS_LEAVE_BLOCKS_ALLOCED;
- cpu_to_le32s(&mt_file_params->data_bits);
+ mt_file_params->data_bits = cpu_to_le32(mt_file_params->data_bits);
}
vhdx_guid_generate(&mt_page83->page_83_data);
@@ -1656,7 +1656,7 @@ static int vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s,
sinfo.file_offset = ROUND_UP(sinfo.file_offset, MiB);
vhdx_update_bat_table_entry(blk_bs(blk), s, &sinfo, &unused, &unused,
block_state);
- cpu_to_le64s(&s->bat[sinfo.bat_idx]);
+ s->bat[sinfo.bat_idx] = cpu_to_le64(s->bat[sinfo.bat_idx]);
sector_num += s->sectors_per_block;
}
ret = blk_pwrite(blk, file_offset, s->bat, length, 0);
--
2.19.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block/vhdx: Don't take address of fields in packed structs
2018-10-16 17:09 [Qemu-devel] [PATCH] block/vhdx: Don't take address of fields in packed structs Peter Maydell
@ 2018-10-17 9:32 ` Stefan Hajnoczi
2018-10-17 14:36 ` [Qemu-devel] " Kevin Wolf
1 sibling, 0 replies; 4+ messages in thread
From: Stefan Hajnoczi @ 2018-10-17 9:32 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, Kevin Wolf, Max Reitz, qemu-block, patches
[-- Attachment #1: Type: text/plain, Size: 1103 bytes --]
On Tue, Oct 16, 2018 at 06:09:38PM +0100, Peter Maydell wrote:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
>
> There are a few places where the in-place swap function is
> used on something other than a packed struct field; we convert
> those anyway, for consistency.
>
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Usual disclaimer: produced with "make check" only, but purely
> automated conversion should be safe.
>
> block/vhdx.h | 12 ++---
> block/vhdx-endian.c | 118 ++++++++++++++++++++++----------------------
> block/vhdx-log.c | 4 +-
> block/vhdx.c | 18 +++----
> 4 files changed, 76 insertions(+), 76 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vhdx: Don't take address of fields in packed structs
2018-10-16 17:09 [Qemu-devel] [PATCH] block/vhdx: Don't take address of fields in packed structs Peter Maydell
2018-10-17 9:32 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2018-10-17 14:36 ` Kevin Wolf
2018-11-05 14:44 ` Peter Maydell
1 sibling, 1 reply; 4+ messages in thread
From: Kevin Wolf @ 2018-10-17 14:36 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, patches, Jeff Cody, Max Reitz, qemu-block
Am 16.10.2018 um 19:09 hat Peter Maydell geschrieben:
> Taking the address of a field in a packed struct is a bad idea, because
> it might not be actually aligned enough for that pointer type (and
> thus cause a crash on dereference on some host architectures). Newer
> versions of clang warn about this. Avoid the bug by not using the
> "modify in place" byte swapping functions.
>
> There are a few places where the in-place swap function is
> used on something other than a packed struct field; we convert
> those anyway, for consistency.
>
> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Usual disclaimer: produced with "make check" only, but purely
> automated conversion should be safe.
More relevant, qemu-iotests for vhdx passes, too. I don't think "make
check" would run any of the modified code.
Thanks, applied to the block branch
Kevin
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] block/vhdx: Don't take address of fields in packed structs
2018-10-17 14:36 ` [Qemu-devel] " Kevin Wolf
@ 2018-11-05 14:44 ` Peter Maydell
0 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2018-11-05 14:44 UTC (permalink / raw)
To: Kevin Wolf; +Cc: QEMU Developers, patches, Jeff Cody, Max Reitz, Qemu-block
On 17 October 2018 at 15:36, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 16.10.2018 um 19:09 hat Peter Maydell geschrieben:
>> Taking the address of a field in a packed struct is a bad idea, because
>> it might not be actually aligned enough for that pointer type (and
>> thus cause a crash on dereference on some host architectures). Newer
>> versions of clang warn about this. Avoid the bug by not using the
>> "modify in place" byte swapping functions.
>>
>> There are a few places where the in-place swap function is
>> used on something other than a packed struct field; we convert
>> those anyway, for consistency.
>>
>> Patch produced with scripts/coccinelle/inplace-byteswaps.cocci.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Usual disclaimer: produced with "make check" only, but purely
>> automated conversion should be safe.
>
> More relevant, qemu-iotests for vhdx passes, too. I don't think "make
> check" would run any of the modified code.
>
> Thanks, applied to the block branch
Ping? This doesn't seem to have reached master.
thanks
-- PMM
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-11-05 14:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-16 17:09 [Qemu-devel] [PATCH] block/vhdx: Don't take address of fields in packed structs Peter Maydell
2018-10-17 9:32 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2018-10-17 14:36 ` [Qemu-devel] " Kevin Wolf
2018-11-05 14:44 ` Peter Maydell
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.