[Qemu-devel] [PATCH for-2.9] tests/virtio-9p-test: Don't call le*_to_cpus on fields of packed struct

[Qemu-devel] [PATCH for-2.9] tests/virtio-9p-test: Don't call le*_to_cpus on fields of packed struct

[Peter Maydell]

For a packed struct like 'P9Hdr' the fields within it may not be
aligned as much as the natural alignment for their types.  This means
it is not valid to pass the address of such a field to a function
like le32_to_cpus() which operate on uint32_t* and assume alignment.
Doing this results in a SIGBUS on hosts like SPARC which have strict
alignment requirements.

Use ldl_le_p() instead, which is specified to correctly handle
unaligned pointers.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Sadly gcc doesn't warn about this:
 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
clang supposedly was fixed to do so:
 https://bugs.llvm.org//show_bug.cgi?id=22821
but I think that commit was reverted without the bug being
reopened; at least my clang doesn't have that warning flag.
---
 tests/virtio-9p-test.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
index 43a1ad8..ad33d96 100644
--- a/tests/virtio-9p-test.c
+++ b/tests/virtio-9p-test.c
@@ -256,8 +256,8 @@ static void v9fs_req_recv(P9Req *req, uint8_t id)
         qvirtio_wait_queue_isr(v9p->dev, v9p->vq, 1000 * 1000);
 
         v9fs_memread(req, &hdr, 7);
-        le32_to_cpus(&hdr.size);
-        le16_to_cpus(&hdr.tag);
+        hdr.size = ldl_le_p(&hdr.size);
+        hdr.tag = lduw_le_p(&hdr.tag);
         if (hdr.size >= 7) {
             break;
         }
-- 
2.7.4

Re: [Qemu-devel] [PATCH for-2.9] tests/virtio-9p-test: Don't call le*_to_cpus on fields of packed struct

[Greg Kurz]

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

On Mon, 27 Mar 2017 18:59:04 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> For a packed struct like 'P9Hdr' the fields within it may not be
> aligned as much as the natural alignment for their types.  This means
> it is not valid to pass the address of such a field to a function
> like le32_to_cpus() which operate on uint32_t* and assume alignment.
> Doing this results in a SIGBUS on hosts like SPARC which have strict
> alignment requirements.
> 
> Use ldl_le_p() instead, which is specified to correctly handle
> unaligned pointers.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

I'll apply this to my tree and send a pull request tomorrow.

Cheers.

--
Greg

> Sadly gcc doesn't warn about this:
>  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51628
> clang supposedly was fixed to do so:
>  https://bugs.llvm.org//show_bug.cgi?id=22821
> but I think that commit was reverted without the bug being
> reopened; at least my clang doesn't have that warning flag.
> ---
>  tests/virtio-9p-test.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> index 43a1ad8..ad33d96 100644
> --- a/tests/virtio-9p-test.c
> +++ b/tests/virtio-9p-test.c
> @@ -256,8 +256,8 @@ static void v9fs_req_recv(P9Req *req, uint8_t id)
>          qvirtio_wait_queue_isr(v9p->dev, v9p->vq, 1000 * 1000);
>  
>          v9fs_memread(req, &hdr, 7);
> -        le32_to_cpus(&hdr.size);
> -        le16_to_cpus(&hdr.tag);
> +        hdr.size = ldl_le_p(&hdr.size);
> +        hdr.tag = lduw_le_p(&hdr.tag);
>          if (hdr.size >= 7) {
>              break;
>          }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]