[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Eric Blake
Subject: Re: [Qemu-devel] [PATCH] nbd: Don't take address of fields in packed structs
Date: Thu, 27 Sep 2018 12:30:11 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0

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;
+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);
     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);
         /* 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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]