Re: [Libguestfs] [libnbd PATCH v4 1/4] states: Document our reliance on type overlaps

When I added structured replies to the NBD spec, I intentionally chose a wire layout where the magic number and cookie overlap, even while the middle member changes from uint32_t error to the pair uint16_t flags and type. Based only on a strict reading of C rules on effective types and compatible type prefixes, it's probably questionable on whether my reliance on type aliasing to reuse cookie from the same offset of a union, or even the fact that a structured reply is built by first reading bytes into sbuf.simple_reply then following up with only bytes into the tail of sbuf.sr.structured_reply is strictly portable. But since it works in practice, it's worth at least adding some compile- and run-time assertions that our (ab)use of aliasing is accessing the bytes we want under the types we expect. Upcoming patches will restructure part of the sbuf layout to hopefully be a little easier to tie back to strict C standards. Suggested-by: Laszlo Ersek <lersek(a)redhat.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- generator/states-reply.c | 17 +++++++++++++---- generator/states-reply-structured.c | 13 +++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 511e5cb1..2c77658b 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -17,6 +17,7 @@ */ #include <assert.h> +#include <stddef.h> /* State machine for receiving reply messages from the server. * @@ -63,9 +64,15 @@ REPLY.START: ssize_t r; /* We read all replies initially as if they are simple replies, but - * check the magic in CHECK_SIMPLE_OR_STRUCTURED_REPLY below. - * This works because the structured_reply header is larger. + * check the magic in CHECK_SIMPLE_OR_STRUCTURED_REPLY below. This + * works because the structured_reply header is larger, and because + * the last member of a simple reply, cookie, is coincident between + * the two structs (an intentional design decision in the NBD spec + * when structured replies were added). */ + STATIC_ASSERT (offsetof (struct nbd_handle, sbuf.simple_reply.cookie) == + offsetof (struct nbd_handle, sbuf.sr.structured_reply.cookie), + cookie_aliasing);

assert (h->reply_cmd == NULL); assert (h->rlen == 0); @@ -135,7 +142,8 @@ REPLY.CHECK_SIMPLE_OR_STRUCTURED_REPLY: } /* NB: This works for both simple and structured replies because the - * handle (our cookie) is stored at the same offset. + * handle (our cookie) is stored at the same offset. See the + * STATIC_ASSERT above in state REPLY.START that confirmed this. */ h->chunks_received++; cookie = be64toh (h->sbuf.simple_reply.cookie); @@ -155,7 +163,8 @@ REPLY.FINISH_COMMAND: bool retire; /* NB: This works for both simple and structured replies because the - * handle (our cookie) is stored at the same offset. + * handle (our cookie) is stored at the same offset. See the + * STATIC_ASSERT above in state REPLY.START that confirmed this. */ cookie = be64toh (h->sbuf.simple_reply.cookie); /* Find the command amongst the commands in flight. */ diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 5aca7262..205a236d 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -19,6 +19,7 @@ /* State machine for parsing structured replies from the server. */ #include <stdbool.h> +#include <stddef.h> #include <stdint.h> #include <inttypes.h> @@ -45,11 +46,15 @@ structured_reply_in_bounds (uint64_t offset, uint32_t length, STATE_MACHINE { REPLY.STRUCTURED_REPLY.START: - /* We've only read the simple_reply. The structured_reply is longer, - * so read the remaining part. + /* We've only read the bytes that fill simple_reply. The + * structured_reply is longer, so read the remaining part. We + * depend on the memory aliasing in union sbuf to overlay the two + * reply types. */ - h->rbuf = &h->sbuf; - h->rbuf = (char *)h->rbuf + sizeof h->sbuf.simple_reply; + STATIC_ASSERT (sizeof h->sbuf.simple_reply == + offsetof (struct nbd_structured_reply, length), + simple_structured_overlap); + assert (h->rbuf == (char *)&h->sbuf + sizeof h->sbuf.simple_reply); h->rlen = sizeof h->sbuf.sr.structured_reply; h->rlen -= sizeof h->sbuf.simple_reply; SET_NEXT_STATE (%RECV_REMAINING);