Libguestfs - Jun 2023 - [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 at redhat.com>
Signed-off-by: Eric Blake <eblake at redhat.com>
---
 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);
-- 
2.40.1

On 6/9/23 04:17, Eric Blake wrote:
> 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 at redhat.com>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  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);
Can you perhaps append

 ... &&
 sizeof h->sbuf.simple_reply.cookie = sizeof
h->sbuf.sr.structured_reply.cookie

(if you agree)?

Also, the commit message and the comment talk about the magic number as
well, not just the cookie, and the static assertion ignores magic.
However, I can see the magic handling changes in the next patch.

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

Thanks
Laszlo
>    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);