[Libguestfs] [libnbd PATCH 1/2] api: Tighter checking of structured read replies

[Nir Soffer]

On Tue, May 31, 2022 at 7:13 PM Eric Blake <eblake at redhat.com> wrote:
>
> On Tue, May 31, 2022 at 06:59:25PM +0300, Nir Soffer wrote:
> > On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake at redhat.com> wrote:
> > >
> > > Now that we allow clients to bypass buffer pre-initialization, it
> > > becomes more important to detect when a buggy server using structured
> > > replies does not send us enough bytes to cover the requested read
> > > size.  Our check is not perfect (a server that duplicates reply chunks
> > > for byte 0 and omits byte 1 gets past our check), but this is a
> > > tighter sanity check so that we are less likely to report a successful
> > > read containing uninitialized memory on a buggy server.
> >
> > Nice!
> >
> > > Because we have a maximum read buffer size of 64M, and first check
> > > that the server's chunk fits bounds, we don't have to worry about
> > > overflowing a uint32_t, even if a server sends enough duplicate
> > > responses that an actual sum would overflow.
> > > ---
>
> > > +++ b/generator/states-reply-structured.c
> > > @@ -354,7 +354,6 @@ STATE_MACHINE {
> > >      assert (cmd); /* guaranteed by CHECK */
> > >
> > >      assert (cmd->data && cmd->type == NBD_CMD_READ);
> > > -    cmd->data_seen = true;
> > >
> > >      /* Length of the data following. */
> > >      length -= 8;
> > > @@ -364,6 +363,8 @@ STATE_MACHINE {
> > >        SET_NEXT_STATE (%.DEAD);
> > >        return 0;
> > >      }
> > > +    if (cmd->data_seen <= cmd->count)
> > > +      cmd->data_seen += length;
> >
> > This does not feel right. if you received more data, it should be counted,
> > and if this causes data_seen to be bigger than cmd->count, isn't this a fatal
> > error?
>
> cmd->count is at most 64M; it represents how much we asked the server
> to provide.  length was just validated (in the elided statement
> between these two hunks) to be <= cmd->count (otherwise, the server is
> known-buggy for replying with more than we asked, and we've already
> moved to %.DEAD state).  cmd->data_seen is a cumulative count of all
> bytes seen in prior chunks, plus the current chunk.  If we have not
> yet passed cmd->count, then this chunk counts towards the cumulative
> limit (and there is no overflow, since 64M*2 < UINT32_MAX).  If we
> have already passed cmd->count (in a previous chunk), then we don't
> further increase cmd->count, but we already know that we will fail the
> overall read later.  In other words, we can stop incrementing
> cmd->data_seen as soon as we know it exceeds cmd->count, and by
> stopping early, we still detect server bugs without overflowing
> uint32_t.

But we can have this case:

1. ask for 32m
2. server sends 16m (data_seen increase to 16m)
3. server sends 16m (data_seen increase to 32m)
4. server sends 1m (data_seen does not increase)
5. entire request succeeds

Shouldn't we fail if server sends unexpected data?

If we detected that all data was received, and we get
unexpected data, why not fail immediately?

    cmd->data_seen += length
    if (cmd->data_seen > cmd->count)
        switch to dead state?

Nir