On 7/21/23 18:08, Eric Blake wrote:
> While working on later patches, I noticed that I was frequently
> referring to the wire contents of h->sbuf.reply.hdr.structured.length,
> byte-swapping it, then repeating open-coded math for how many bytes
> were consumed in earlier states. It's easier to normalize the
> remaining bytes to be parsed into a variable that persists across the
> life of the reply, and also makes it possible to track that by the end
> of any structured reply, we have dealt with all of the bytes that the
> header advertised. Some of the worst implicit assumptions in block
> status code will be fixed in the next patch with the help of this new
> variable.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
>
> v4: new patch, replacing part of v3's 5/22
> ---
> lib/internal.h | 3 ++
> generator/states-reply-chunk.c | 59 ++++++++++++++++------------------
> 2 files changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/lib/internal.h b/lib/internal.h
> index 4b0043b3..b38ae524 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -304,6 +304,9 @@ struct nbd_handle {
> string_vector querylist;
> size_t querynum;
>
> + /* Chunk payload length remaining to be parsed */
> + size_t payload_left;
> +
> /* When receiving block status, this is used. */
> uint32_t *bs_entries;
>
> diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c
> index 26b8a6b0..da5fc426 100644
> --- a/generator/states-reply-chunk.c
> +++ b/generator/states-reply-chunk.c
> @@ -69,6 +69,7 @@ REPLY.CHUNK_REPLY.START:
> /* Skip an unexpected structured reply, including to an unknown cookie. */
> if (cmd == NULL || !h->structured_replies)
> goto resync;
> + h->payload_left = length;
>
> switch (type) {
> case NBD_REPLY_TYPE_NONE:
> @@ -87,6 +88,7 @@ REPLY.CHUNK_REPLY.START:
> goto resync;
> h->rbuf = &h->sbuf.reply.payload.offset_data;
> h->rlen = sizeof h->sbuf.reply.payload.offset_data;
> + h->payload_left -= h->rlen;
> SET_NEXT_STATE (%RECV_OFFSET_DATA);
> break;
>
> @@ -96,6 +98,7 @@ REPLY.CHUNK_REPLY.START:
> goto resync;
> h->rbuf = &h->sbuf.reply.payload.offset_hole;
> h->rlen = sizeof h->sbuf.reply.payload.offset_hole;
> + h->payload_left -= h->rlen;
> SET_NEXT_STATE (%RECV_OFFSET_HOLE);
> break;
>
> @@ -141,7 +144,8 @@ REPLY.CHUNK_REPLY.START:
>
> resync:
> h->rbuf = NULL;
> - h->rlen = length;
> + h->rlen = h->payload_left;
> + h->payload_left = 0;
> SET_NEXT_STATE (%RESYNC);
> return 0;
>
> @@ -156,7 +160,8 @@ REPLY.CHUNK_REPLY.RECV_ERROR:
> SET_NEXT_STATE (%.READY);
> return 0;
> case 0:
> - length = be32toh (h->sbuf.reply.hdr.structured.length);
> + length = h->payload_left;
> + h->payload_left -= MIN (length, sizeof
> h->sbuf.reply.payload.error.error);
> assert (length >= sizeof h->sbuf.reply.payload.error.error.error);
> assert (cmd);
>
> @@ -164,12 +169,13 @@ REPLY.CHUNK_REPLY.RECV_ERROR:
> goto resync;
>
> msglen = be16toh (h->sbuf.reply.payload.error.error.len);
> - if (msglen > length - sizeof h->sbuf.reply.payload.error.error ||
> + if (msglen > h->payload_left ||
> msglen > sizeof h->sbuf.reply.payload.error.msg)
> goto resync;
>
> h->rbuf = h->sbuf.reply.payload.error.msg;
> h->rlen = msglen;
> + h->payload_left -= h->rlen;
> SET_NEXT_STATE (%RECV_ERROR_MESSAGE);
> }
> return 0;
> @@ -180,12 +186,13 @@ REPLY.CHUNK_REPLY.RECV_ERROR:
> if (cmd->error == 0)
> cmd->error = nbd_internal_errno_of_nbd_error (error);
> h->rbuf = NULL;
> - h->rlen = length - MIN (length, sizeof h->sbuf.reply.payload.error.error);
> + h->rlen = h->payload_left;
> + h->payload_left = 0;
> SET_NEXT_STATE (%RESYNC);
> return 0;
>
> REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE:
> - uint32_t length, msglen;
> + uint32_t msglen;
> uint16_t type;
>
> switch (recv_into_rbuf (h)) {
> @@ -195,33 +202,31 @@ REPLY.CHUNK_REPLY.RECV_ERROR_MESSAGE:
> SET_NEXT_STATE (%.READY);
> return 0;
> case 0:
> - length = be32toh (h->sbuf.reply.hdr.structured.length);
> msglen = be16toh (h->sbuf.reply.payload.error.error.len);
> type = be16toh (h->sbuf.reply.hdr.structured.type);
>
> - length -= sizeof h->sbuf.reply.payload.error.error + msglen;
> -
> if (msglen)
> debug (h, "structured error server message: %.*s", (int)msglen,
> h->sbuf.reply.payload.error.msg);
>
> /* Special case two specific errors; silently ignore tail for all others
> */
> h->rbuf = NULL;
> - h->rlen = length;
> + h->rlen = h->payload_left;
> switch (type) {
> case NBD_REPLY_TYPE_ERROR:
> - if (length != 0)
> + if (h->payload_left != 0)
> debug (h, "ignoring unexpected slop after error message, "
> "the server may have a bug");
> break;
> case NBD_REPLY_TYPE_ERROR_OFFSET:
> - if (length != sizeof h->sbuf.reply.payload.error.offset)
> + if (h->payload_left != sizeof h->sbuf.reply.payload.error.offset)
> debug (h, "unable to safely extract error offset, "
> "the server may have a bug");
> else
> h->rbuf = &h->sbuf.reply.payload.error.offset;
> break;
> }
> + h->payload_left = 0;
> SET_NEXT_STATE (%RECV_ERROR_TAIL);
> }
> return 0;
> @@ -286,7 +291,6 @@ REPLY.CHUNK_REPLY.RECV_ERROR_TAIL:
> REPLY.CHUNK_REPLY.RECV_OFFSET_DATA:
> struct command *cmd = h->reply_cmd;
> uint64_t offset;
> - uint32_t length;
>
> switch (recv_into_rbuf (h)) {
> case -1: SET_NEXT_STATE (%.DEAD); return 0;
> @@ -295,29 +299,24 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA:
> SET_NEXT_STATE (%.READY);
> return 0;
> case 0:
> - length = be32toh (h->sbuf.reply.hdr.structured.length);
> offset = be64toh (h->sbuf.reply.payload.offset_data.offset);
>
> assert (cmd); /* guaranteed by CHECK */
> -
> assert (cmd->data && cmd->type == NBD_CMD_READ);
>
> - /* Length of the data following. */
> - length -= 8;
> -
> /* Is the data within bounds? */
> - if (! structured_reply_in_bounds (offset, length, cmd)) {
> + if (! structured_reply_in_bounds (offset, h->payload_left, cmd)) {
> SET_NEXT_STATE (%.DEAD);
> return 0;
> }
> if (cmd->data_seen <= cmd->count)
> - cmd->data_seen += length;
> + cmd->data_seen += h->payload_left;
> /* Now this is the byte offset in the read buffer. */
> offset -= cmd->offset;
>
> /* Set up to receive the data directly to the user buffer. */
> h->rbuf = (char *)cmd->data + offset;
> - h->rlen = length;
> + h->rlen = h->payload_left;
> SET_NEXT_STATE (%RECV_OFFSET_DATA_DATA);
> }
> return 0;
> @@ -325,7 +324,6 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA:
> REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA:
> struct command *cmd = h->reply_cmd;
> uint64_t offset;
> - uint32_t length;
>
> switch (recv_into_rbuf (h)) {
> case -1: SET_NEXT_STATE (%.DEAD); return 0;
> @@ -334,7 +332,6 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA:
> SET_NEXT_STATE (%.READY);
> return 0;
> case 0:
> - length = be32toh (h->sbuf.reply.hdr.structured.length);
> offset = be64toh (h->sbuf.reply.payload.offset_data.offset);
>
> assert (cmd); /* guaranteed by CHECK */
> @@ -343,11 +340,12 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_DATA_DATA:
>
> if (CALL_CALLBACK (cmd->cb.fn.chunk,
> (char *)cmd->data + (offset - cmd->offset),
> - length - sizeof offset, offset,
> + h->payload_left, offset,
> LIBNBD_READ_DATA, &error) == -1)
> if (cmd->error == 0)
> cmd->error = error ? error : EPROTO;
> }
> + h->payload_left = 0;
>
> SET_NEXT_STATE (%FINISH);
> }
> @@ -369,7 +367,6 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE:
> length = be32toh (h->sbuf.reply.payload.offset_hole.length);
>
> assert (cmd); /* guaranteed by CHECK */
> -
> assert (cmd->data && cmd->type == NBD_CMD_READ);
>
> /* Is the data within bounds? */
> @@ -405,8 +402,8 @@ REPLY.CHUNK_REPLY.RECV_OFFSET_HOLE:
>
> REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> struct command *cmd = h->reply_cmd;
> - uint32_t length;
> size_t i;
> + size_t count;
> uint32_t context_id;
>
> switch (recv_into_rbuf (h)) {
> @@ -416,20 +413,20 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
> SET_NEXT_STATE (%.READY);
> return 0;
> case 0:
> - length = be32toh (h->sbuf.reply.hdr.structured.length);
> -
> assert (cmd); /* guaranteed by CHECK */
> assert (cmd->type == NBD_CMD_BLOCK_STATUS);
> assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent));
> assert (h->bs_entries);
> - assert (length >= 12);
> + assert (h->payload_left >= 12);
> assert (h->meta_valid);
>
> /* Need to byte-swap the entries returned, but apart from that we
> * don't validate them.
> */
> - for (i = 0; i < length/4; ++i)
> + for (i = 0; i < h->payload_left / sizeof *h->bs_entries; ++i)
> h->bs_entries[i] = be32toh (h->bs_entries[i]);
> + count = (h->payload_left / sizeof *h->bs_entries) - 1;
> + h->payload_left = 0;
>
> /* Look up the context ID. */
> context_id = h->bs_entries[0];
> @@ -443,8 +440,7 @@ REPLY.CHUNK_REPLY.RECV_BS_ENTRIES:
>
> if (CALL_CALLBACK (cmd->cb.fn.extent,
> h->meta_contexts.ptr[i].name, cmd->offset,
> - &h->bs_entries[1], (length-4) / 4,
> - &error) == -1)
> + &h->bs_entries[1], count, &error) == -1)
> if (cmd->error == 0)
> cmd->error = error ? error : EPROTO;
> }
> @@ -494,6 +490,7 @@ REPLY.CHUNK_REPLY.RESYNC:
> REPLY.CHUNK_REPLY.FINISH:
> uint16_t flags;
>
> + assert (h->payload_left == 0);
> flags = be16toh (h->sbuf.reply.hdr.structured.flags);
> if (flags & NBD_REPLY_FLAG_DONE) {
> SET_NEXT_STATE (%^FINISH_COMMAND);
Acked-by: Laszlo Ersek <ler...@redhat.com>
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
