| Message ID | 20180925161924.7832-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | migration/ram.c: Avoid taking address of fields in packed MultiFDInit_t struct | expand |
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
* 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
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",
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(-) -- 2.19.0