migration/ram.c: Avoid taking address of fields in packed MultiFDInit_t struct

[Qemu-devel] [PATCH] migration/ram.c: Avoid taking address of fields in packed MultiFDInit_t struct

Posted by Peter Maydell 5 years, 6 months ago

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:

migration/ram.c:651:19: warning: taking address of packed member 'magic' of class or structure 'MultiFDInit_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
migration/ram.c:652:19: warning: taking address of packed member 'version' of class or structure 'MultiFDInit_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
migration/ram.c:737:19: warning: taking address of packed member 'magic' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
migration/ram.c:745:19: warning: taking address of packed member 'version' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
migration/ram.c:755:19: warning: taking address of packed member 'size' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]

Avoid the bug by not using the "modify in place" byteswapping
functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Now I have a machine with the version of clang that emits these warnings
I'm starting to care a little more about them :-)
---
 migration/ram.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index f6fd8e5e096..4fc3bc36816 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -648,8 +648,8 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
         return -1;
     }
 
-    be32_to_cpus(&msg.magic);
-    be32_to_cpus(&msg.version);
+    msg.magic = be32_to_cpu(msg.magic);
+    msg.version = be32_to_cpu(msg.version);
 
     if (msg.magic != MULTIFD_MAGIC) {
         error_setg(errp, "multifd: received packet magic %x "
@@ -734,7 +734,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
     RAMBlock *block;
     int i;
 
-    be32_to_cpus(&packet->magic);
+    packet->magic = be32_to_cpu(packet->magic);
     if (packet->magic != MULTIFD_MAGIC) {
         error_setg(errp, "multifd: received packet "
                    "magic %x and expected magic %x",
@@ -742,7 +742,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
         return -1;
     }
 
-    be32_to_cpus(&packet->version);
+    packet->version = be32_to_cpu(packet->version);
     if (packet->version != MULTIFD_VERSION) {
         error_setg(errp, "multifd: received packet "
                    "version %d and expected version %d",
@@ -752,7 +752,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
 
     p->flags = be32_to_cpu(packet->flags);
 
-    be32_to_cpus(&packet->size);
+    packet->size = be32_to_cpu(packet->size);
     if (packet->size > migrate_multifd_page_count()) {
         error_setg(errp, "multifd: received packet "
                    "with size %d and expected maximum size %d",
-- 
2.19.0

Re: [Qemu-devel] [PATCH] migration/ram.c: Avoid taking address of fields in packed MultiFDInit_t struct

Posted by Marc-André Lureau 5 years, 6 months ago

On Tue, Sep 25, 2018 at 8:24 PM Peter Maydell <peter.maydell@linaro.org> 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:
>
> migration/ram.c:651:19: warning: taking address of packed member 'magic' of class or structure 'MultiFDInit_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:652:19: warning: taking address of packed member 'version' of class or structure 'MultiFDInit_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:737:19: warning: taking address of packed member 'magic' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:745:19: warning: taking address of packed member 'version' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:755:19: warning: taking address of packed member 'size' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
>
> Avoid the bug by not using the "modify in place" byteswapping
> functions.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
> Now I have a machine with the version of clang that emits these warnings
> I'm starting to care a little more about them :-)
> ---
>  migration/ram.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index f6fd8e5e096..4fc3bc36816 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -648,8 +648,8 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
>          return -1;
>      }
>
> -    be32_to_cpus(&msg.magic);
> -    be32_to_cpus(&msg.version);
> +    msg.magic = be32_to_cpu(msg.magic);
> +    msg.version = be32_to_cpu(msg.version);
>
>      if (msg.magic != MULTIFD_MAGIC) {
>          error_setg(errp, "multifd: received packet magic %x "
> @@ -734,7 +734,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>      RAMBlock *block;
>      int i;
>
> -    be32_to_cpus(&packet->magic);
> +    packet->magic = be32_to_cpu(packet->magic);
>      if (packet->magic != MULTIFD_MAGIC) {
>          error_setg(errp, "multifd: received packet "
>                     "magic %x and expected magic %x",
> @@ -742,7 +742,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>          return -1;
>      }
>
> -    be32_to_cpus(&packet->version);
> +    packet->version = be32_to_cpu(packet->version);
>      if (packet->version != MULTIFD_VERSION) {
>          error_setg(errp, "multifd: received packet "
>                     "version %d and expected version %d",
> @@ -752,7 +752,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>
>      p->flags = be32_to_cpu(packet->flags);
>
> -    be32_to_cpus(&packet->size);
> +    packet->size = be32_to_cpu(packet->size);
>      if (packet->size > migrate_multifd_page_count()) {
>          error_setg(errp, "multifd: received packet "
>                     "with size %d and expected maximum size %d",
> --
> 2.19.0
>
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] migration/ram.c: Avoid taking address of fields in packed MultiFDInit_t struct

Posted by Dr. David Alan Gilbert 5 years, 6 months ago

* Peter Maydell (peter.maydell@linaro.org) 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:
> 
> migration/ram.c:651:19: warning: taking address of packed member 'magic' of class or structure 'MultiFDInit_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:652:19: warning: taking address of packed member 'version' of class or structure 'MultiFDInit_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:737:19: warning: taking address of packed member 'magic' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:745:19: warning: taking address of packed member 'version' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> migration/ram.c:755:19: warning: taking address of packed member 'size' of class or structure 'MultiFDPacket_t' may result in an unaligned pointer value [-Waddress-of-packed-member]
> 
> Avoid the bug by not using the "modify in place" byteswapping
> functions.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Queued

> ---
> Now I have a machine with the version of clang that emits these warnings
> I'm starting to care a little more about them :-)
> ---
>  migration/ram.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index f6fd8e5e096..4fc3bc36816 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -648,8 +648,8 @@ static int multifd_recv_initial_packet(QIOChannel *c, Error **errp)
>          return -1;
>      }
>  
> -    be32_to_cpus(&msg.magic);
> -    be32_to_cpus(&msg.version);
> +    msg.magic = be32_to_cpu(msg.magic);
> +    msg.version = be32_to_cpu(msg.version);
>  
>      if (msg.magic != MULTIFD_MAGIC) {
>          error_setg(errp, "multifd: received packet magic %x "
> @@ -734,7 +734,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>      RAMBlock *block;
>      int i;
>  
> -    be32_to_cpus(&packet->magic);
> +    packet->magic = be32_to_cpu(packet->magic);
>      if (packet->magic != MULTIFD_MAGIC) {
>          error_setg(errp, "multifd: received packet "
>                     "magic %x and expected magic %x",
> @@ -742,7 +742,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>          return -1;
>      }
>  
> -    be32_to_cpus(&packet->version);
> +    packet->version = be32_to_cpu(packet->version);
>      if (packet->version != MULTIFD_VERSION) {
>          error_setg(errp, "multifd: received packet "
>                     "version %d and expected version %d",
> @@ -752,7 +752,7 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>  
>      p->flags = be32_to_cpu(packet->flags);
>  
> -    be32_to_cpus(&packet->size);
> +    packet->size = be32_to_cpu(packet->size);
>      if (packet->size > migrate_multifd_page_count()) {
>          error_setg(errp, "multifd: received packet "
>                     "with size %d and expected maximum size %d",
> -- 
> 2.19.0
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK