Libguestfs - Nov 2017 - [nbdkit PATCH] connections: Improve error responses

We had several inconsistencies from the NBD spec when diagnosing
bad client messages:
- FLUSH is not generally forbidden on a read-only export (so failing
with EPERM is wrong) [meanwhile, if we don't advertise flush because
plugin_can_flush() fails, then rejecting with EINVAL is still okay]
- returning EPERM (aka EROFS) for read-only exports should probably
take precedence over anything else
- out-of-bounds WRITE and WRITE_ZEROES should fail with ENOSPC,
not EIO
- out-of-bounds READ and TRIM should fail with EINVAL, not EIO

We also had dead code: valid_range() and validate_request() cannot
return -1.  Make these functions return bool instead.  And finally,
fix a comment typo.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 53 ++++++++++++++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 29 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index 8dc1925..264037d 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -666,7 +666,7 @@ negotiate_handshake (struct connection *conn)
   return r;
 }

-static int
+static bool
 valid_range (struct connection *conn, uint64_t offset, uint32_t count)
 {
   uint64_t exportsize = conn->exportsize;
@@ -674,27 +674,34 @@ valid_range (struct connection *conn, uint64_t offset,
uint32_t count)
   return count > 0 && offset <= exportsize && offset +
count <= exportsize;
 }

-static int
+static bool
 validate_request (struct connection *conn,
                   uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t
count,
                   uint32_t *error)
 {
   int r;

+  /* Readonly connection? */
+  if (conn->readonly &&
+      (cmd == NBD_CMD_WRITE ||
+       cmd == NBD_CMD_TRIM || cmd == NBD_CMD_WRITE_ZEROES)) {
+    nbdkit_error ("invalid request: write request on readonly
connection");
+    *error = EROFS;
+    return false;
+  }
+
   /* Validate cmd, offset, count. */
   switch (cmd) {
   case NBD_CMD_READ:
   case NBD_CMD_WRITE:
   case NBD_CMD_TRIM:
   case NBD_CMD_WRITE_ZEROES:
-    r = valid_range (conn, offset, count);
-    if (r == -1)
-      return -1;
-    if (r == 0) {
+    if (!valid_range (conn, offset, count)) {
       /* XXX Allow writes to extend the disk? */
       nbdkit_error ("invalid request: offset and length are out of
range");
-      *error = EIO;
-      return 0;
+      *error = (cmd == NBD_CMD_WRITE ||
+                cmd == NBD_CMD_WRITE_ZEROES) ? ENOSPC : EINVAL;
+      return false;
     }
     break;

@@ -702,7 +709,7 @@ validate_request (struct connection *conn,
     if (offset != 0 || count != 0) {
       nbdkit_error ("invalid flush request: expecting offset and length ==
0");
       *error = EINVAL;
-      return 0;
+      return false;
     }
     break;

@@ -710,20 +717,20 @@ validate_request (struct connection *conn,
     nbdkit_error ("invalid request: unknown command (%" PRIu32
") ignored",
                   cmd);
     *error = EINVAL;
-    return 0;
+    return false;
   }

   /* Validate flags */
   if (flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
     nbdkit_error ("invalid request: unknown flag (0x%x)", flags);
     *error = EINVAL;
-    return 0;
+    return false;
   }
   if ((flags & NBD_CMD_FLAG_NO_HOLE) &&
       cmd != NBD_CMD_WRITE_ZEROES) {
     nbdkit_error ("invalid request: NO_HOLE flag needs WRITE_ZEROES
request");
     *error = EINVAL;
-    return 0;
+    return false;
   }

   /* Refuse over-large read and write requests. */
@@ -732,33 +739,24 @@ validate_request (struct connection *conn,
     nbdkit_error ("invalid request: data request is too large (%"
PRIu32
                   " > %d)", count, MAX_REQUEST_SIZE);
     *error = ENOMEM;
-    return 0;
-  }
-
-  /* Readonly connection? */
-  if (conn->readonly &&
-      (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_FLUSH ||
-       cmd == NBD_CMD_TRIM || cmd == NBD_CMD_WRITE_ZEROES)) {
-    nbdkit_error ("invalid request: write request on readonly
connection");
-    *error = EROFS;
-    return 0;
+    return false;
   }

   /* Flush allowed? */
   if (!conn->can_flush && cmd == NBD_CMD_FLUSH) {
     nbdkit_error ("invalid request: flush operation not supported");
     *error = EINVAL;
-    return 0;
+    return false;
   }

   /* Trim allowed? */
   if (!conn->can_trim && cmd == NBD_CMD_TRIM) {
     nbdkit_error ("invalid request: trim operation not supported");
     *error = EINVAL;
-    return 0;
+    return false;
   }

-  return 1;                     /* Commands validates. */
+  return true;                     /* Command validates. */
 }

 /* Grab the appropriate error value.
@@ -970,10 +968,7 @@ recv_request_send_reply (struct connection *conn)
   }

   /* Validate the request. */
-  r = validate_request (conn, cmd, flags, offset, count, &error);
-  if (r == -1)
-    return -1;
-  if (r == 0) {                 /* request not valid */
+  if (!validate_request (conn, cmd, flags, offset, count, &error)) {
     if (cmd == NBD_CMD_WRITE &&
         skip_over_write_buffer (conn->sockin, count) < 0)
       return -1;
-- 
2.13.6

On Wed, Nov 15, 2017 at 04:16:49PM -0600, Eric Blake
wrote:
> We had several inconsistencies from the NBD spec when diagnosing
> bad client messages:
> - FLUSH is not generally forbidden on a read-only export (so failing
> with EPERM is wrong) [meanwhile, if we don't advertise flush because
> plugin_can_flush() fails, then rejecting with EINVAL is still okay]
> - returning EPERM (aka EROFS) for read-only exports should probably
> take precedence over anything else
> - out-of-bounds WRITE and WRITE_ZEROES should fail with ENOSPC,
> not EIO
> - out-of-bounds READ and TRIM should fail with EINVAL, not EIO
> 
> We also had dead code: valid_range() and validate_request() cannot
> return -1.  Make these functions return bool instead.  And finally,
> fix a comment typo.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  src/connections.c | 53
++++++++++++++++++++++++-----------------------------
>  1 file changed, 24 insertions(+), 29 deletions(-)
> 
> diff --git a/src/connections.c b/src/connections.c
> index 8dc1925..264037d 100644
> --- a/src/connections.c
> +++ b/src/connections.c
> @@ -666,7 +666,7 @@ negotiate_handshake (struct connection *conn)
>    return r;
>  }
> 
> -static int
> +static bool
>  valid_range (struct connection *conn, uint64_t offset, uint32_t count)
>  {
>    uint64_t exportsize = conn->exportsize;
> @@ -674,27 +674,34 @@ valid_range (struct connection *conn, uint64_t
offset, uint32_t count)
>    return count > 0 && offset <= exportsize && offset
+ count <= exportsize;
>  }
> 
> -static int
> +static bool
>  validate_request (struct connection *conn,
>                    uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t
count,
>                    uint32_t *error)
>  {
>    int r;
> 
> +  /* Readonly connection? */
> +  if (conn->readonly &&
> +      (cmd == NBD_CMD_WRITE ||
> +       cmd == NBD_CMD_TRIM || cmd == NBD_CMD_WRITE_ZEROES)) {
> +    nbdkit_error ("invalid request: write request on readonly
connection");
> +    *error = EROFS;
> +    return false;
> +  }
> +
>    /* Validate cmd, offset, count. */
>    switch (cmd) {
>    case NBD_CMD_READ:
>    case NBD_CMD_WRITE:
>    case NBD_CMD_TRIM:
>    case NBD_CMD_WRITE_ZEROES:
> -    r = valid_range (conn, offset, count);
> -    if (r == -1)
> -      return -1;
> -    if (r == 0) {
> +    if (!valid_range (conn, offset, count)) {
>        /* XXX Allow writes to extend the disk? */
>        nbdkit_error ("invalid request: offset and length are out of
range");
> -      *error = EIO;
> -      return 0;
> +      *error = (cmd == NBD_CMD_WRITE ||
> +                cmd == NBD_CMD_WRITE_ZEROES) ? ENOSPC : EINVAL;
> +      return false;
>      }
>      break;
> 
> @@ -702,7 +709,7 @@ validate_request (struct connection *conn,
>      if (offset != 0 || count != 0) {
>        nbdkit_error ("invalid flush request: expecting offset and
length == 0");
>        *error = EINVAL;
> -      return 0;
> +      return false;
>      }
>      break;
> 
> @@ -710,20 +717,20 @@ validate_request (struct connection *conn,
>      nbdkit_error ("invalid request: unknown command (%" PRIu32
") ignored",
>                    cmd);
>      *error = EINVAL;
> -    return 0;
> +    return false;
>    }
> 
>    /* Validate flags */
>    if (flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE)) {
>      nbdkit_error ("invalid request: unknown flag (0x%x)",
flags);
>      *error = EINVAL;
> -    return 0;
> +    return false;
>    }
>    if ((flags & NBD_CMD_FLAG_NO_HOLE) &&
>        cmd != NBD_CMD_WRITE_ZEROES) {
>      nbdkit_error ("invalid request: NO_HOLE flag needs WRITE_ZEROES
request");
>      *error = EINVAL;
> -    return 0;
> +    return false;
>    }
> 
>    /* Refuse over-large read and write requests. */
> @@ -732,33 +739,24 @@ validate_request (struct connection *conn,
>      nbdkit_error ("invalid request: data request is too large
(%" PRIu32
>                    " > %d)", count, MAX_REQUEST_SIZE);
>      *error = ENOMEM;
> -    return 0;
> -  }
> -
> -  /* Readonly connection? */
> -  if (conn->readonly &&
> -      (cmd == NBD_CMD_WRITE || cmd == NBD_CMD_FLUSH ||
> -       cmd == NBD_CMD_TRIM || cmd == NBD_CMD_WRITE_ZEROES)) {
> -    nbdkit_error ("invalid request: write request on readonly
connection");
> -    *error = EROFS;
> -    return 0;
> +    return false;
>    }
> 
>    /* Flush allowed? */
>    if (!conn->can_flush && cmd == NBD_CMD_FLUSH) {
>      nbdkit_error ("invalid request: flush operation not
supported");
>      *error = EINVAL;
> -    return 0;
> +    return false;
>    }
> 
>    /* Trim allowed? */
>    if (!conn->can_trim && cmd == NBD_CMD_TRIM) {
>      nbdkit_error ("invalid request: trim operation not
supported");
>      *error = EINVAL;
> -    return 0;
> +    return false;
>    }
> 
> -  return 1;                     /* Commands validates. */
> +  return true;                     /* Command validates. */
>  }
> 
>  /* Grab the appropriate error value.
> @@ -970,10 +968,7 @@ recv_request_send_reply (struct connection *conn)
>    }
> 
>    /* Validate the request. */
> -  r = validate_request (conn, cmd, flags, offset, count, &error);
> -  if (r == -1)
> -    return -1;
> -  if (r == 0) {                 /* request not valid */
> +  if (!validate_request (conn, cmd, flags, offset, count, &error)) {
>      if (cmd == NBD_CMD_WRITE &&
>          skip_over_write_buffer (conn->sockin, count) < 0)
>        return -1;
ACK.

I added you to the "collaborators" section of the project, so
hopefully you should be able to push this.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v