On Thu, Jun 01, 2023 at 08:00:55AM -0500, Eric Blake wrote:
> > > @@ -83,13 +96,18 @@ REPLY.STRUCTURED_REPLY.CHECK:
> > > * not worth keeping the connection alive.
> > > */
> > > if (length > MAX_REQUEST_SIZE + sizeof
> > > h->sbuf.reply.payload.offset_data) {
> > > - set_error (0, "invalid server reply length %" PRIu32, length);
> > > + set_error (0, "invalid server reply length %" PRIu64, length);
> > > SET_NEXT_STATE (%.DEAD);
> > > return 0;
> > > }
> > > + /* For convenience, we now normalize extended replies into compact,
> > > + * doable since we validated that payload length fits in 32 bits.
> > > + */
> > > + h->sbuf.reply.hdr.structured.length = length;
> >
> > (8) I'm very confused by this. For an extended reply, this will
> > overwrite the "offset" field.
>
> At one point, I considered normalizing in the opposite direction (take
> structured replies and widen them into the extended header form); it
> required touching more lines of code, so I didn't pursue it at the
> time. But I can see how compressing things down (intentionally
> throwing away information we will no longer reference) can be
> confusing without good comments, so at a minimum, I will be adding
> comments, if not outright going with the widening rather than
> narrowing approach.
In fact, doing it in place is bad. Later code in this function can
trigger a transition to state RESYNC, which assumes
sbuf.reply.hdr.structured.length still holds the wire value, not the
normalized value. I'm fixing it up by creating h->payload_left,
initialized in START to the endian-corrected wire value, and then
decremented as various states peel off parts of the payload, so that
FINISH can then assert all payload has been accounted for.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
