Libguestfs - Jan 2019 - [PATCH nbdkit] xz: Do not pass can_write through to the plugin.

I'm not sure that this fix is really correct.

An alternate way I can think to fix this would be for the core server
to maintain a readonly flag for each layer (instead of just per-
server).

You could also argue that our readonly test in
server/connections.c:compute_eflags is wrong and/or that the
implementations of server/plugins.c:plugin_can_write and
server/filters.c:filter_can_write have the wrong default - they should
consult the readonly flag which was passed to that layer's open call.

However the fix only affects one filter, and we're not maintaining an
API for filters, so maybe we can defer the problem.

Rich.

Using ‘nbdkit --filter=xz file disk.xz’ and loop mounting eventually
results in this error:

nbdkit: file[1]: debug: xz: pwrite count=4096 offset=1048576 flags=0x0
nbdkit: file[1]: debug: pwrite count=4096 offset=1048576 fua=0
nbdkit: file[1]: error: pwrite: Bad file descriptor
nbdkit: file[1]: debug: sending error reply: Bad file descriptor

Since the filter did not intercept can_write, it was passed through to
the layer below (the file plugin).  The filter opens the file plugin
passing readonly=1, but in the case where there are no filters, nbdkit
core would never call can_write() (assuming that because we opened the
server readonly, it would return false).  However with the filter,
can_write is called and returns the default (true).  This caused
writes to be generated which were also passed through to the plugin
which generates "Bad file descriptor" because:

 EBADF  fd is not a valid file descriptor or is not open for writing.

This change also disables zero and trim because the server logic
disables those when the top backend->can_write returns false.
---
 filters/xz/xz.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/filters/xz/xz.c b/filters/xz/xz.c
index 28a6a81..5ff791e 100644
--- a/filters/xz/xz.c
+++ b/filters/xz/xz.c
@@ -173,6 +173,13 @@ xz_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata, void *handle)
   return xzfile_get_size (h->xz);
 }
 
+static int
+xz_can_write (struct nbdkit_next_ops *next_ops, void *nxdata,
+              void *handle)
+{
+  return 0;
+}
+
 /* Read data from the file. */
 static int
 xz_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
@@ -225,6 +232,7 @@ static struct nbdkit_filter filter = {
   .close             = xz_close,
   .prepare           = xz_prepare,
   .get_size          = xz_get_size,
+  .can_write         = xz_can_write,
   .pread             = xz_pread,
 };
 
-- 
2.20.1

On 1/30/19 8:36 AM, Richard W.M. Jones wrote:
> Using ‘nbdkit --filter=xz file disk.xz’ and loop mounting eventually
> results in this error:
> 
> nbdkit: file[1]: debug: xz: pwrite count=4096 offset=1048576 flags=0x0
> nbdkit: file[1]: debug: pwrite count=4096 offset=1048576 fua=0
> nbdkit: file[1]: error: pwrite: Bad file descriptor
> nbdkit: file[1]: debug: sending error reply: Bad file descriptor
> 
> Since the filter did not intercept can_write, it was passed through to
> the layer below (the file plugin).  The filter opens the file plugin
> passing readonly=1, but in the case where there are no filters, nbdkit
> core would never call can_write() (assuming that because we opened the
> server readonly, it would return false).  However with the filter,
> can_write is called and returns the default (true).  This caused
> writes to be generated which were also passed through to the plugin
> which generates "Bad file descriptor" because:
> 
>  EBADF  fd is not a valid file descriptor or is not open for writing.
> 
> This change also disables zero and trim because the server logic
> disables those when the top backend->can_write returns false.
ACK.
> ---
>  filters/xz/xz.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/filters/xz/xz.c b/filters/xz/xz.c
> index 28a6a81..5ff791e 100644
> --- a/filters/xz/xz.c
> +++ b/filters/xz/xz.c
> @@ -173,6 +173,13 @@ xz_get_size (struct nbdkit_next_ops *next_ops, void
*nxdata, void *handle)
>    return xzfile_get_size (h->xz);
>  }
>  
> +static int
> +xz_can_write (struct nbdkit_next_ops *next_ops, void *nxdata,
> +              void *handle)
Perhaps worth a comment in the code, similar to cow.c's comment about
the override returning a constant because the filter enforces a
particular behavior that might be different from the backend's default
behavior.
> +{
> +  return 0;
> +}
> +
>  /* Read data from the file. */
>  static int
>  xz_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
> @@ -225,6 +232,7 @@ static struct nbdkit_filter filter = {
>    .close             = xz_close,
>    .prepare           = xz_prepare,
>    .get_size          = xz_get_size,
> +  .can_write         = xz_can_write,
>    .pread             = xz_pread,
>  };
>  
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 1/30/19 8:36 AM, Richard W.M. Jones wrote:
> I'm not sure that this fix is really correct.
The fix is correct in the sense that yes, the xz filter cannot allow
writes, and must therefore override write support from leaking through
from the plugin.
> 
> An alternate way I can think to fix this would be for the core server
> to maintain a readonly flag for each layer (instead of just per-
> server).
> 
> You could also argue that our readonly test in
> server/connections.c:compute_eflags is wrong and/or that the
> implementations of server/plugins.c:plugin_can_write and
> server/filters.c:filter_can_write have the wrong default - they should
> consult the readonly flag which was passed to that layer's open call.
Arguing that filters.c picks the wrong defaults makes a bit of intuitive
sense, but it trickier to implement correctly.

We have several examples of filters that override .pwrite but not
.can_write (e.g. truncate), some that override .can_write but not
.pwrite (xz), some that override both (cow), some that override neither
(nozero).  When .can_write is overridden, it is obviously correct. But
when .can_write is absent, whether or not .pwrite is also absent, I can
see your idea of defaulting to "if I passed readonly=true to the
plugin's open(), synthesize .can_write of false no matter what; if I
passed readonly=false to the plugin's open(), then call the plugin's
.can_write() which may return true or false based on other aspects up to
the plugin".

And we can track what the filter passes to the plugin's .open, since a
filter's .open MUST call the next(nxdata) parameters it was given. Since
next(nxdata) is implemented in filters.c, THAT could be the place where
we stash off whether the filter is passing true/false to the underling
plugin's .open (even if that value differs from the value of readonly
passed to the filter's .open), such that we can then have filters.c in
charge of remembering the per-connection state of whether the underlying
plugin is in forced-readonly mode and therefore the filter must
synthesize a .can_read that forces false.
> 
> However the fix only affects one filter, and we're not maintaining an
> API for filters, so maybe we can defer the problem.
I agree that our filter API is not stable, so I'm okay with your patch
going in now, regardless of whether we also change filters.c to have
saner defaults.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org