[Libguestfs] [nbdkit PATCH] offset: Better handling of parameters

[Richard W.M. Jones]

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 at 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 at 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/