Libguestfs - Aug 2019 - [nbdkit PATCH] offset: Better handling of parameters

The man page claims both offset and range are optional (matching the
code), but the --help text claims offset is mandatory, and the comment
to the no-op offset_config_complete claims we require both parameters.
We did not check for an offset larger than the underlying size when
there was no range, and even when there is a range, we were not
careful about integer overflow (offset=5E range=5E happily claims to
export a 5E image; but all bets are off if you later try to access
beyond the real underlying size).  And in several cases, such as if
the plugin's get_size fails but range was not provided, we are not
returning -1 for failure.

Fixes: 3db69f56
Signed-off-by: Eric Blake <eblake@redhat.com>
---

The first post-1.14 bugfix.  Too bad I didn't spot it earlier today.

 filters/offset/offset.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/filters/offset/offset.c b/filters/offset/offset.c
index 7df1ed13..1039a577 100644
--- a/filters/offset/offset.c
+++ b/filters/offset/offset.c
@@ -64,16 +64,9 @@ offset_config (nbdkit_next_config *next, void *nxdata,
     return next (nxdata, key, value);
 }

-/* Check the user did pass both parameters. */
-static int
-offset_config_complete (nbdkit_next_config_complete *next, void *nxdata)
-{
-  return next (nxdata);
-}
-
 #define offset_config_help \
-  "offset=<OFFSET>     (required) The start offset to serve.\n"
\
-  "range=<LENGTH>                 The total size to serve."
+  "offset=<OFFSET>            The start offset to serve (default
0).\n" \
+  "range=<LENGTH>             The total size to serve (default rest
of file)."

 /* Get the file size. */
 static int64_t
@@ -82,16 +75,23 @@ offset_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata,
 {
   int64_t real_size = next_ops->get_size (nxdata);

+  if (real_size == -1)
+    return -1;
+
   if (range >= 0) {
-    if (offset + range > real_size) {
+    if (offset > real_size - range) {
       nbdkit_error ("offset+range is larger than the real size "
                     "of the underlying file or device");
       return -1;
     }
     return range;
   }
-  else
-    return real_size - offset;
+  else if (offset > real_size) {
+    nbdkit_error ("offset is larger than the real size "
+                  "of the underlying file or device");
+    return -1;
+  }
+  return real_size - offset;
 }

 /* Read data. */
@@ -176,7 +176,6 @@ static struct nbdkit_filter filter = {
   .longname          = "nbdkit offset filter",
   .version           = PACKAGE_VERSION,
   .config            = offset_config,
-  .config_complete   = offset_config_complete,
   .config_help       = offset_config_help,
   .get_size          = offset_get_size,
   .pread             = offset_pread,
-- 
2.21.0

On Wed, Aug 28, 2019 at 04:09:24PM -0500, Eric Blake
wrote:
> The man page claims both offset and range are optional (matching the
> code), but the --help text claims offset is mandatory, and the comment
> to the no-op offset_config_complete claims we require both parameters.
> We did not check for an offset larger than the underlying size when
> there was no range, and even when there is a range, we were not
> careful about integer overflow (offset=5E range=5E happily claims to
> export a 5E image; but all bets are off if you later try to access
> beyond the real underlying size).  And in several cases, such as if
> the plugin's get_size fails but range was not provided, we are not
> returning -1 for failure.
> 
> Fixes: 3db69f56
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> The first post-1.14 bugfix.  Too bad I didn't spot it earlier today.
The patch looks good, ACK.

We can backport these fixes easily to 1.14.1, but let's
give it a few days as I'm sure there will be several more
bugs that we find.

However do please push this today, because I will do a
final [probably] 1.12 stable branch release later today.

Thanks,

Rich.
>  filters/offset/offset.c | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/filters/offset/offset.c b/filters/offset/offset.c
> index 7df1ed13..1039a577 100644
> --- a/filters/offset/offset.c
> +++ b/filters/offset/offset.c
> @@ -64,16 +64,9 @@ offset_config (nbdkit_next_config *next, void *nxdata,
>      return next (nxdata, key, value);
>  }
> 
> -/* Check the user did pass both parameters. */
> -static int
> -offset_config_complete (nbdkit_next_config_complete *next, void *nxdata)
> -{
> -  return next (nxdata);
> -}
> -
>  #define offset_config_help \
> -  "offset=<OFFSET>     (required) The start offset to
serve.\n" \
> -  "range=<LENGTH>                 The total size to
serve."
> +  "offset=<OFFSET>            The start offset to serve
(default 0).\n" \
> +  "range=<LENGTH>             The total size to serve (default
rest of file)."
> 
>  /* Get the file size. */
>  static int64_t
> @@ -82,16 +75,23 @@ offset_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata,
>  {
>    int64_t real_size = next_ops->get_size (nxdata);
> 
> +  if (real_size == -1)
> +    return -1;
> +
>    if (range >= 0) {
> -    if (offset + range > real_size) {
> +    if (offset > real_size - range) {
>        nbdkit_error ("offset+range is larger than the real size "
>                      "of the underlying file or device");
>        return -1;
>      }
>      return range;
>    }
> -  else
> -    return real_size - offset;
> +  else if (offset > real_size) {
> +    nbdkit_error ("offset is larger than the real size "
> +                  "of the underlying file or device");
> +    return -1;
> +  }
> +  return real_size - offset;
>  }
> 
>  /* Read data. */
> @@ -176,7 +176,6 @@ static struct nbdkit_filter filter = {
>    .longname          = "nbdkit offset filter",
>    .version           = PACKAGE_VERSION,
>    .config            = offset_config,
> -  .config_complete   = offset_config_complete,
>    .config_help       = offset_config_help,
>    .get_size          = offset_get_size,
>    .pread             = offset_pread,
> -- 
> 2.21.0
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/