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

On Tue, May 31, 2022 at 08:40:57PM +0300, Nir Soffer wrote: > > > > @@ -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) Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m.

Then, later on when retiring the command, we note that 33m != 32m and fail the read with EIO (if it has not already failed for other reasons). > 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? Switching immediately to a dead state is also possible, but it's nice to try and keep the connection alive as long as we can with a nice diagnosis of a failed CMD_READ but still allow further commands, rather than an abrupt disconnect that takes out all other use of the server.