Re: [Qemu-devel] [PATCH v2 5/8] nbd: BLOCK_STATUS for standard get_block_status function: client part

[Vladimir Sementsov-Ogievskiy]

27.04.2018 15:50, Peter Maydell wrote:

On 12 March 2018 at 15:21, Vladimir Sementsov-Ogievskiy
 wrote:

Minimal realization: only one extent in server answer is supported.
Flag NBD_CMD_FLAG_REQ_ONE is used to force this behavior.

Hi; Coverity complains about a possible use of uninitialized
variables in this function (CID 1390607, 1390611):


+static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
+ const char *export,
+ const char *context,
+ uint32_t *context_id,
+ Error **errp)
+{
+int ret;
+NBDOptionReply reply;
+uint32_t received_id;
+bool received;

We declare received_id and received without initializing them...


+uint32_t export_len = strlen(export);
+uint32_t context_len = strlen(context);
+uint32_t data_len = sizeof(export_len) + export_len +
+sizeof(uint32_t) + /* number of queries */
+sizeof(context_len) + context_len;
+char *data = g_malloc(data_len);
+char *p = data;
+
+stl_be_p(p, export_len);
+memcpy(p += sizeof(export_len), export, export_len);
+stl_be_p(p += export_len, 1);
+stl_be_p(p += sizeof(uint32_t), context_len);
+memcpy(p += sizeof(context_len), context, context_len);
+
+ret = nbd_send_option_request(ioc, NBD_OPT_SET_META_CONTEXT, data_len, 
data,
+  errp);
+g_free(data);
+if (ret < 0) {
+return ret;
+}
+
+if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
+ errp) < 0)
+{
+return -1;
+}
+
+ret = nbd_handle_reply_err(ioc, , errp);
+if (ret <= 0) {
+return ret;
+}
+
+if (reply.type == NBD_REP_META_CONTEXT) {

...and if we don't take this code path we don't ever initialize
received...


+char *name;
+size_t len;
+
+if (nbd_read(ioc, _id, sizeof(received_id), errp) < 0) {
+return -EIO;
+}
+be32_to_cpus(_id);
+
+len = reply.length - sizeof(received_id);
+name = g_malloc(len + 1);
+if (nbd_read(ioc, name, len, errp) < 0) {
+g_free(name);
+return -EIO;
+}
+name[len] = '\0';
+if (strcmp(context, name)) {
+error_setg(errp, "Failed to negotiate meta context '%s', server "
+   "answered with different context '%s'", context,
+   name);
+g_free(name);
+return -1;
+}
+g_free(name);
+
+received = true;
+
+/* receive NBD_REP_ACK */
+if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
+ errp) < 0)
+{
+return -1;
+}
+
+ret = nbd_handle_reply_err(ioc, , errp);
+if (ret <= 0) {
+return ret;
+}
+}
+
+if (reply.type != NBD_REP_ACK) {
+error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
+   reply.type, NBD_REP_ACK);
+return -1;
+}
+
+if (received) {
+*context_id = received_id;

...so we might use both received and received_id uninitialized here.


+return 1;
+}
+
+return 0;
+}

My guess is that the correct fix is just to initialize received
with "bool received = false;". Coverity should then be able to figure
out that we don't touch received_id unless we initialized it.


Yes, looks like a bug. I'll send a patch.



thanks
-- PMM



--
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 5/8] nbd: BLOCK_STATUS for standard get_block_status function: client part

[Peter Maydell]

On 12 March 2018 at 15:21, Vladimir Sementsov-Ogievskiy
 wrote:
> Minimal realization: only one extent in server answer is supported.
> Flag NBD_CMD_FLAG_REQ_ONE is used to force this behavior.

Hi; Coverity complains about a possible use of uninitialized
variables in this function (CID 1390607, 1390611):

> +static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
> + const char *export,
> + const char *context,
> + uint32_t *context_id,
> + Error **errp)
> +{
> +int ret;
> +NBDOptionReply reply;
> +uint32_t received_id;
> +bool received;

We declare received_id and received without initializing them...

> +uint32_t export_len = strlen(export);
> +uint32_t context_len = strlen(context);
> +uint32_t data_len = sizeof(export_len) + export_len +
> +sizeof(uint32_t) + /* number of queries */
> +sizeof(context_len) + context_len;
> +char *data = g_malloc(data_len);
> +char *p = data;
> +
> +stl_be_p(p, export_len);
> +memcpy(p += sizeof(export_len), export, export_len);
> +stl_be_p(p += export_len, 1);
> +stl_be_p(p += sizeof(uint32_t), context_len);
> +memcpy(p += sizeof(context_len), context, context_len);
> +
> +ret = nbd_send_option_request(ioc, NBD_OPT_SET_META_CONTEXT, data_len, 
> data,
> +  errp);
> +g_free(data);
> +if (ret < 0) {
> +return ret;
> +}
> +
> +if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
> + errp) < 0)
> +{
> +return -1;
> +}
> +
> +ret = nbd_handle_reply_err(ioc, , errp);
> +if (ret <= 0) {
> +return ret;
> +}
> +
> +if (reply.type == NBD_REP_META_CONTEXT) {

...and if we don't take this code path we don't ever initialize
received...

> +char *name;
> +size_t len;
> +
> +if (nbd_read(ioc, _id, sizeof(received_id), errp) < 0) {
> +return -EIO;
> +}
> +be32_to_cpus(_id);
> +
> +len = reply.length - sizeof(received_id);
> +name = g_malloc(len + 1);
> +if (nbd_read(ioc, name, len, errp) < 0) {
> +g_free(name);
> +return -EIO;
> +}
> +name[len] = '\0';
> +if (strcmp(context, name)) {
> +error_setg(errp, "Failed to negotiate meta context '%s', server "
> +   "answered with different context '%s'", context,
> +   name);
> +g_free(name);
> +return -1;
> +}
> +g_free(name);
> +
> +received = true;
> +
> +/* receive NBD_REP_ACK */
> +if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, ,
> + errp) < 0)
> +{
> +return -1;
> +}
> +
> +ret = nbd_handle_reply_err(ioc, , errp);
> +if (ret <= 0) {
> +return ret;
> +}
> +}
> +
> +if (reply.type != NBD_REP_ACK) {
> +error_setg(errp, "Unexpected reply type %" PRIx32 " expected %x",
> +   reply.type, NBD_REP_ACK);
> +return -1;
> +}
> +
> +if (received) {
> +*context_id = received_id;

...so we might use both received and received_id uninitialized here.

> +return 1;
> +}
> +
> +return 0;
> +}

My guess is that the correct fix is just to initialize received
with "bool received = false;". Coverity should then be able to figure
out that we don't touch received_id unless we initialized it.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 5/8] nbd: BLOCK_STATUS for standard get_block_status function: client part

[Eric Blake]

On 03/12/2018 10:21 AM, Vladimir Sementsov-Ogievskiy wrote:

Minimal realization: only one extent in server answer is supported.
Flag NBD_CMD_FLAG_REQ_ONE is used to force this behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---

v2: - drop iotests changes, as server is fixed in 03
 - rebase to byte-based block status
 - use payload_advance32
 - check extent->length for zero and for alignment (not sure about
   zero, but, we do not send block status with zero-length, so
   reply should not be zero-length too)


The NBD spec needs to be clarified that a zero-length request is bogus; 
once that is done, then the server can be required to make progress (if 
it succeeds, at least one non-zero extent was reported per namespace), 
as that is the most useful behavior (if a server replies with 0 extents 
or 0-length extents, the client could go into an inf-loop re-requesting 
the same status).



 - handle max_block in nbd_client_co_block_status
 - handle zero-length request in nbd_client_co_block_status
 - do not use magic numbers in nbd_negotiate_simple_meta_context

 ? Hm, don't remember, what we decided about DATA/HOLE flags mapping..


At this point, it's still up in the air for me to fix the complaints 
Kevin had, but those are bug fixes on top of this series (and thus okay 
during soft freeze), so your initial implementation is adequate for a 
first commit.



+++ b/block/nbd-client.c
@@ -228,6 +228,47 @@ static int 
nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
  return 0;
  }
  
+/* nbd_parse_blockstatus_payload

+ * support only one extent in reply and only for
+ * base:allocation context
+ */
+static int nbd_parse_blockstatus_payload(NBDClientSession *client,
+ NBDStructuredReplyChunk *chunk,
+ uint8_t *payload, uint64_t 
orig_length,
+ NBDExtent *extent, Error **errp)
+{
+uint32_t context_id;
+
+if (chunk->length != sizeof(context_id) + sizeof(extent)) {
+error_setg(errp, "Protocol error: invalid payload for "
+ "NBD_REPLY_TYPE_BLOCK_STATUS");
+return -EINVAL;
+}
+
+context_id = payload_advance32();
+if (client->info.meta_base_allocation_id != context_id) {
+error_setg(errp, "Protocol error: unexpected context id: %d for "


s/id:/id/


+ "NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated context 
"
+ "id is %d", context_id,
+ client->info.meta_base_allocation_id);
+return -EINVAL;
+}
+
+extent->length = payload_advance32();
+extent->flags = payload_advance32();
+
+if (extent->length == 0 ||
+extent->length % client->info.min_block != 0 ||
+extent->length > orig_length)
+{
+/* TODO: clarify in NBD spec the second requirement about min_block */


Yeah, the spec wording can be tightened, but the intent is obvious: the 
server better not be reporting status on anything smaller than what you 
can address with read or write.  But I think we can address that on the 
NBD list without a TODO here.


However, you do have a bug: the server doesn't have to report min_block, 
so the value can still be zero (see nbd_refresh_limits, for example) - 
and %0 is a bad idea.  I'll do the obvious cleanup of checking for a 
non-zero min_block.



+error_setg(errp, "Protocol error: server sent chunk of invalid 
length");


Maybe insert 'status' in there?

  
+static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,

+uint64_t handle, uint64_t length,
+NBDExtent *extent, Error **errp)
+{
+NBDReplyChunkIter iter;
+NBDReply reply;
+void *payload = NULL;
+Error *local_err = NULL;
+bool received = false;
+
+NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
+NULL, , )
+{
+int ret;
+NBDStructuredReplyChunk *chunk = 
+
+assert(nbd_reply_is_structured());
+
+switch (chunk->type) {
+case NBD_REPLY_TYPE_BLOCK_STATUS:
+if (received) {
+s->quit = true;
+error_setg(_err, "Several BLOCK_STATUS chunks in reply");


Not necessarily an error later on when we request more than one 
namespace, but fine for the initial implementation where we really do 
expect exactly one status.



+nbd_iter_error(, true, -EINVAL, _err);
+}
+received = true;
+
+ret = nbd_parse_blockstatus_payload(s, ,
+payload, length, extent,
+_err);
+if (ret < 0) {
+s->quit = true;
+nbd_iter_error(, true, ret, _err);
+}
+