[Qemu-devel] [PATCH] block/vdi: Don't take address of fields in packed structs

[Qemu-devel] [PATCH] block/vdi: Don't take address of fields in packed structs

[Peter Maydell]

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.

There are other places where we take the address of a packed member
in this file for other purposes than passing it to a byteswap
function (all the calls to qemu_uuid_*()); we leave those for now.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Another "tested with make check" auto-conversion patch. In this
case, as noted above, it doesn't fix all the warnings for the file,
but we might as well put the easy part of the fix in. I'm not sure
what to do with the qemu_uuid_*() calls. Something like
     QemuUUID uuid_link = header->uuid_link;
and then using "qemu_uuid_is_null(uuid_link)" rather than
"qemu_uuid_is_null(&header.uuid_link)" ?
Or we could macroise the relevant qemu_uuid functions (notably
qemu_uuid_bswap()) or turn them into functions taking a QemuUUID
rather than a QemuUUID*. Suggestions ?

 block/vdi.c | 64 ++++++++++++++++++++++++++---------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6555cffb886..0ff1ead736c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -187,22 +187,22 @@ typedef struct {
 
 static void vdi_header_to_cpu(VdiHeader *header)
 {
-    le32_to_cpus(&header->signature);
-    le32_to_cpus(&header->version);
-    le32_to_cpus(&header->header_size);
-    le32_to_cpus(&header->image_type);
-    le32_to_cpus(&header->image_flags);
-    le32_to_cpus(&header->offset_bmap);
-    le32_to_cpus(&header->offset_data);
-    le32_to_cpus(&header->cylinders);
-    le32_to_cpus(&header->heads);
-    le32_to_cpus(&header->sectors);
-    le32_to_cpus(&header->sector_size);
-    le64_to_cpus(&header->disk_size);
-    le32_to_cpus(&header->block_size);
-    le32_to_cpus(&header->block_extra);
-    le32_to_cpus(&header->blocks_in_image);
-    le32_to_cpus(&header->blocks_allocated);
+    header->signature = le32_to_cpu(header->signature);
+    header->version = le32_to_cpu(header->version);
+    header->header_size = le32_to_cpu(header->header_size);
+    header->image_type = le32_to_cpu(header->image_type);
+    header->image_flags = le32_to_cpu(header->image_flags);
+    header->offset_bmap = le32_to_cpu(header->offset_bmap);
+    header->offset_data = le32_to_cpu(header->offset_data);
+    header->cylinders = le32_to_cpu(header->cylinders);
+    header->heads = le32_to_cpu(header->heads);
+    header->sectors = le32_to_cpu(header->sectors);
+    header->sector_size = le32_to_cpu(header->sector_size);
+    header->disk_size = le64_to_cpu(header->disk_size);
+    header->block_size = le32_to_cpu(header->block_size);
+    header->block_extra = le32_to_cpu(header->block_extra);
+    header->blocks_in_image = le32_to_cpu(header->blocks_in_image);
+    header->blocks_allocated = le32_to_cpu(header->blocks_allocated);
     qemu_uuid_bswap(&header->uuid_image);
     qemu_uuid_bswap(&header->uuid_last_snap);
     qemu_uuid_bswap(&header->uuid_link);
@@ -211,22 +211,22 @@ static void vdi_header_to_cpu(VdiHeader *header)
 
 static void vdi_header_to_le(VdiHeader *header)
 {
-    cpu_to_le32s(&header->signature);
-    cpu_to_le32s(&header->version);
-    cpu_to_le32s(&header->header_size);
-    cpu_to_le32s(&header->image_type);
-    cpu_to_le32s(&header->image_flags);
-    cpu_to_le32s(&header->offset_bmap);
-    cpu_to_le32s(&header->offset_data);
-    cpu_to_le32s(&header->cylinders);
-    cpu_to_le32s(&header->heads);
-    cpu_to_le32s(&header->sectors);
-    cpu_to_le32s(&header->sector_size);
-    cpu_to_le64s(&header->disk_size);
-    cpu_to_le32s(&header->block_size);
-    cpu_to_le32s(&header->block_extra);
-    cpu_to_le32s(&header->blocks_in_image);
-    cpu_to_le32s(&header->blocks_allocated);
+    header->signature = cpu_to_le32(header->signature);
+    header->version = cpu_to_le32(header->version);
+    header->header_size = cpu_to_le32(header->header_size);
+    header->image_type = cpu_to_le32(header->image_type);
+    header->image_flags = cpu_to_le32(header->image_flags);
+    header->offset_bmap = cpu_to_le32(header->offset_bmap);
+    header->offset_data = cpu_to_le32(header->offset_data);
+    header->cylinders = cpu_to_le32(header->cylinders);
+    header->heads = cpu_to_le32(header->heads);
+    header->sectors = cpu_to_le32(header->sectors);
+    header->sector_size = cpu_to_le32(header->sector_size);
+    header->disk_size = cpu_to_le64(header->disk_size);
+    header->block_size = cpu_to_le32(header->block_size);
+    header->block_extra = cpu_to_le32(header->block_extra);
+    header->blocks_in_image = cpu_to_le32(header->blocks_in_image);
+    header->blocks_allocated = cpu_to_le32(header->blocks_allocated);
     qemu_uuid_bswap(&header->uuid_image);
     qemu_uuid_bswap(&header->uuid_last_snap);
     qemu_uuid_bswap(&header->uuid_link);
-- 
2.19.0

Re: [Qemu-devel] [Qemu-block] [PATCH] block/vdi: Don't take address of fields in packed structs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Tue, Oct 16, 2018 at 06:25:03PM +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.
> 
> There are other places where we take the address of a packed member
> in this file for other purposes than passing it to a byteswap
> function (all the calls to qemu_uuid_*()); we leave those for now.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Another "tested with make check" auto-conversion patch. In this
> case, as noted above, it doesn't fix all the warnings for the file,
> but we might as well put the easy part of the fix in. I'm not sure
> what to do with the qemu_uuid_*() calls. Something like
>      QemuUUID uuid_link = header->uuid_link;
> and then using "qemu_uuid_is_null(uuid_link)" rather than

I would take this route.  (I think you mean qemu_uuid_is_null(&uuid_link).)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] block/vdi: Don't take address of fields in packed structs

[Kevin Wolf]

Am 16.10.2018 um 19:25 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.
> 
> There are other places where we take the address of a packed member
> in this file for other purposes than passing it to a byteswap
> function (all the calls to qemu_uuid_*()); we leave those for now.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] block/vdi: Don't take address of fields in packed structs

[Peter Maydell]

On 17 October 2018 at 15:55, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 16.10.2018 um 19:25 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.
>>
>> There are other places where we take the address of a packed member
>> in this file for other purposes than passing it to a byteswap
>> function (all the calls to qemu_uuid_*()); we leave those for now.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Thanks, applied to the block branch.

This also hasn't made it into master.

thanks
-- PMM