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

[Eric Blake]

On 9/27/18 11:42 AM, 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.
>
> This patch was produced with the following spatch script:
> @@
> expression E;
> @@
> -be16_to_cpus(&E);
> +E = be16_to_cpu(E);

I'm a bit confused. After applying your patch (and rebasing it to my 
pending pull request), I still found instances of be16_to_cpus() and 
others.  Were you only flipping instances that were members of a packed 
struct, while leaving other instances unchanged (in which case the 
commit message should be amended to mention post-filtering on the 
Coccinelle results)?  Can the Coccinelle script be tightened to only 
catch expressions of the form a.b or a->b, or where we guarantee a 
packed struct was involved?

> Signed-off-by: Peter Maydell <address@hidden>
> ---
> Disclaimer: tested only with "make check", but it is a mechanically
> generated patch...
>
>   nbd/client.c | 44 ++++++++++++++++++++++----------------------
>   nbd/server.c | 16 ++++++++--------
>   2 files changed, 30 insertions(+), 30 deletions(-)

I'm wondering if we need to squash this in (for complete conversion, per 
the listed Coccinelle script), or omit it (since these are not packed 
uses, in-place conversion still works):

diff --git i/nbd/server.c w/nbd/server.c
index 58f20ef34ee..98d0fa25158 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -486,7 +486,7 @@ static int nbd_negotiate_send_info(NBDClient *client,
     if (rc < 0) {
         return rc;
     }
-    cpu_to_be16s(&info);
+    info = cpu_to_be16(info);
     if (nbd_write(client->ioc, &info, sizeof(info), errp) < 0) {
         return -EIO;
     }
@@ -551,14 +551,14 @@ static int nbd_negotiate_handle_info(NBDClient 
*client, uint16_t myflags,
     if (rc <= 0) {
         return rc;
     }
-    be16_to_cpus(&requests);
+    requests = be16_to_cpu(requests);
     trace_nbd_negotiate_handle_info_requests(requests);
     while (requests--) {
         rc = nbd_opt_read(client, &request, sizeof(request), errp);
         if (rc <= 0) {
             return rc;
         }
-        be16_to_cpus(&request);
+        request = be16_to_cpu(request);
         trace_nbd_negotiate_handle_info_request(request,
                                                 nbd_info_lookup(request));
         /* We care about NBD_INFO_NAME and NBD_INFO_BLOCK_SIZE;


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org