Re: [Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place
On Mon, Sep 11, 2023 at 09:19:12AM -0500, Eric Blake wrote:
> On Mon, Sep 11, 2023 at 03:09:21PM +0100, Richard W.M. Jones wrote:
> > > > - case SERVICE_MODE_UNIXSOCKET:
> > > > -fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> > > > -if (export_name && strcmp (export_name, "") != 0) {
> > > > - putc ('/', fp);
> > > > - uri_quote (export_name, fp);
> > > > -}
> > > > -fprintf (fp, "\\?socket=");
> > > > -uri_quote (unixsocket, fp);
> > >
> > > Beforehand, we were manually shell-quoting the ? in the Unix URI...
> >
> > The shell quoting here was only marginally useful before this change.
> > In theory there might be a file in a subdirectory called
> > 'nbd+unix:/Xsocket=' which would match :-)
>
> > > > + switch (service_mode) {
> > > > + case SERVICE_MODE_UNIXSOCKET:
> > > > +fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> > > > +if (export_name && strcmp (export_name, "") != 0) {
> > > > + putc ('/', fp);
> > > > + uri_quote (export_name, fp);
> > > > +}
> > > > +fprintf (fp, "?socket=");
> > > > +uri_quote (unixsocket, fp);
> > >
> > > ...where the manual shell-quoting is no longer injected. Yes, this
> > > looks correct (the appearance of the quoting, using '' instead of \,
> > > may be different, but the resulting string as parsed by the shell is
> > > the same).
>
> My point was that pre-patch, we had:
>
> nbd+unix://\?socket=...
>
> and post-patch, we have:
>
> 'nbd+unix://?socket=...'
>
> but both forms are equally quoted; after the shell removes \ or ''
> quoting, we are left with:
>
> nbd+unix://?socket=...
>
> and neither form allowed the glob expansion of the (unlikely) file
> nbd+unix:/Xsocket=...
>
> My comment was more about why changing the fprintf("\\?socket=")
> pre-patch to fprintf("?socket=") post-patch was correct.
Agreed, 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
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place
On Mon, Sep 11, 2023 at 03:09:21PM +0100, Richard W.M. Jones wrote:
> > > - case SERVICE_MODE_UNIXSOCKET:
> > > -fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> > > -if (export_name && strcmp (export_name, "") != 0) {
> > > - putc ('/', fp);
> > > - uri_quote (export_name, fp);
> > > -}
> > > -fprintf (fp, "\\?socket=");
> > > -uri_quote (unixsocket, fp);
> >
> > Beforehand, we were manually shell-quoting the ? in the Unix URI...
>
> The shell quoting here was only marginally useful before this change.
> In theory there might be a file in a subdirectory called
> 'nbd+unix:/Xsocket=' which would match :-)
> > > + switch (service_mode) {
> > > + case SERVICE_MODE_UNIXSOCKET:
> > > +fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> > > +if (export_name && strcmp (export_name, "") != 0) {
> > > + putc ('/', fp);
> > > + uri_quote (export_name, fp);
> > > +}
> > > +fprintf (fp, "?socket=");
> > > +uri_quote (unixsocket, fp);
> >
> > ...where the manual shell-quoting is no longer injected. Yes, this
> > looks correct (the appearance of the quoting, using '' instead of \,
> > may be different, but the resulting string as parsed by the shell is
> > the same).
My point was that pre-patch, we had:
nbd+unix://\?socket=...
and post-patch, we have:
'nbd+unix://?socket=...'
but both forms are equally quoted; after the shell removes \ or ''
quoting, we are left with:
nbd+unix://?socket=...
and neither form allowed the glob expansion of the (unlikely) file
nbd+unix:/Xsocket=...
My comment was more about why changing the fprintf("\\?socket=")
pre-patch to fprintf("?socket=") post-patch was correct.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
Re: [Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place
On Mon, Sep 11, 2023 at 09:01:47AM -0500, Eric Blake wrote:
> On Sat, Sep 09, 2023 at 02:57:52PM +0100, Richard W.M. Jones wrote:
> > Move the calculation of $uri to the main function (well, inlined
> > there), and out of --run code.
> >
> > This is largely code motion. In theory it changes the content of $uri
> > since we now shell quote it after generating it, but this ought not to
> > have any practical effect.
> > ---
> > server/internal.h | 1 +
> > server/captive.c | 41 ++--
> > server/main.c | 79 +++
> > 3 files changed, 82 insertions(+), 39 deletions(-)
> >
>
> > +++ b/server/captive.c
> > @@ -75,45 +75,8 @@ run_command (void)
> >
> >/* Construct $uri. */
> >fprintf (fp, "uri=");
> > - switch (service_mode) {
> > - case SERVICE_MODE_SOCKET_ACTIVATION:
> > - case SERVICE_MODE_LISTEN_STDIN:
> > -break; /* can't form a URI, leave it blank */
> > - case SERVICE_MODE_UNIXSOCKET:
> > -fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> > -if (export_name && strcmp (export_name, "") != 0) {
> > - putc ('/', fp);
> > - uri_quote (export_name, fp);
> > -}
> > -fprintf (fp, "\\?socket=");
> > -uri_quote (unixsocket, fp);
>
> Beforehand, we were manually shell-quoting the ? in the Unix URI...
The shell quoting here was only marginally useful before this change.
In theory there might be a file in a subdirectory called
'nbd+unix:/Xsocket=' which would match :-)
> > -break;
> > - case SERVICE_MODE_VSOCK:
> > -/* 1 = VMADDR_CID_LOCAL */
> > -fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" : "");
> > -if (port) {
> > - putc (':', fp);
> > - shell_quote (port, fp);
> > -}
> > -if (export_name && strcmp (export_name, "") != 0) {
> > - putc ('/', fp);
> > - uri_quote (export_name, fp);
> > -}
> > -break;
> > - case SERVICE_MODE_TCPIP:
> > -fprintf (fp, "nbd%s://localhost", tls == 2 ? "s" : "");
> > -if (port) {
> > - putc (':', fp);
> > - shell_quote (port, fp);
> > -}
> > -if (export_name && strcmp (export_name, "") != 0) {
> > - putc ('/', fp);
> > - uri_quote (export_name, fp);
> > -}
> > -break;
> > - default:
> > -abort ();
> > - }
> > + if (uri)
> > +shell_quote (uri, fp);
>
> ...while here, we shell-quote the entire string...
Right, and '?' is not listed as a "safe char" so it should be quoted:
https://gitlab.com/nbdkit/nbdkit/-/blob/ff3c9eb0e2afb24def80950cbfc963c14b037ba5/common/utils/quote.c#L54
> >putc ('\n', fp);
> >
> >/* Since nbdkit 1.24, $nbd is a synonym for $uri. */
> > diff --git a/server/main.c b/server/main.c
> > index 2a332bfdd..54eb348ba 100644
> > --- a/server/main.c
>
> > +static char *
> > +make_uri (void)
>
> > +
> > + switch (service_mode) {
> > + case SERVICE_MODE_UNIXSOCKET:
> > +fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> > +if (export_name && strcmp (export_name, "") != 0) {
> > + putc ('/', fp);
> > + uri_quote (export_name, fp);
> > +}
> > +fprintf (fp, "?socket=");
> > +uri_quote (unixsocket, fp);
>
> ...where the manual shell-quoting is no longer injected. Yes, this
> looks correct (the appearance of the quoting, using '' instead of \,
> may be different, but the resulting string as parsed by the shell is
> the same).
>
> > +break;
> > + case SERVICE_MODE_VSOCK:
> > +/* 1 = VMADDR_CID_LOCAL */
> > +fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" : "");
> > +if (port) {
> > + putc (':', fp);
> > + fputs (port, fp);
> > +}
> > +if (export_name && strcmp (export_name, "") != 0) {
> > + putc ('/', fp);
> > + uri_quote (export_name, fp);
> > +}
> > +break;
> > + case SERVICE_MODE_TCPIP:
> > +fprintf (fp, "nbd%s://localhost", tls == 2 ? "s" : "");
> > +if (port) {
> > + putc (':', fp);
> > + fputs (port, fp);
> > +}
> > +if (export_name && strcmp (export_name, "") != 0) {
> > + putc ('/', fp);
> > + uri_quote (export_name, fp);
> > +}
> > +break;
> > +
> > + case SERVICE_MODE_SOCKET_ACTIVATION:
> > + case SERVICE_MODE_LISTEN_STDIN:
> > +abort (); /* see above */
> > + default:
> > +abort ();
>
> Could consolidate these labels, although I don't know if a compiler
> would be picky about:
>
> case ...:
> /* comment */
> default:
> abort ();
>
> so I'm also fine leaving it as-is.
>
> Reviewed-by: Eric Blake
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW
___
Libguestfs mailing list
Libguestfs@redhat.com
Re: [Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place
On Sat, Sep 09, 2023 at 02:57:52PM +0100, Richard W.M. Jones wrote:
> Move the calculation of $uri to the main function (well, inlined
> there), and out of --run code.
>
> This is largely code motion. In theory it changes the content of $uri
> since we now shell quote it after generating it, but this ought not to
> have any practical effect.
> ---
> server/internal.h | 1 +
> server/captive.c | 41 ++--
> server/main.c | 79 +++
> 3 files changed, 82 insertions(+), 39 deletions(-)
>
> +++ b/server/captive.c
> @@ -75,45 +75,8 @@ run_command (void)
>
>/* Construct $uri. */
>fprintf (fp, "uri=");
> - switch (service_mode) {
> - case SERVICE_MODE_SOCKET_ACTIVATION:
> - case SERVICE_MODE_LISTEN_STDIN:
> -break; /* can't form a URI, leave it blank */
> - case SERVICE_MODE_UNIXSOCKET:
> -fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> -if (export_name && strcmp (export_name, "") != 0) {
> - putc ('/', fp);
> - uri_quote (export_name, fp);
> -}
> -fprintf (fp, "\\?socket=");
> -uri_quote (unixsocket, fp);
Beforehand, we were manually shell-quoting the ? in the Unix URI...
> -break;
> - case SERVICE_MODE_VSOCK:
> -/* 1 = VMADDR_CID_LOCAL */
> -fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" : "");
> -if (port) {
> - putc (':', fp);
> - shell_quote (port, fp);
> -}
> -if (export_name && strcmp (export_name, "") != 0) {
> - putc ('/', fp);
> - uri_quote (export_name, fp);
> -}
> -break;
> - case SERVICE_MODE_TCPIP:
> -fprintf (fp, "nbd%s://localhost", tls == 2 ? "s" : "");
> -if (port) {
> - putc (':', fp);
> - shell_quote (port, fp);
> -}
> -if (export_name && strcmp (export_name, "") != 0) {
> - putc ('/', fp);
> - uri_quote (export_name, fp);
> -}
> -break;
> - default:
> -abort ();
> - }
> + if (uri)
> +shell_quote (uri, fp);
...while here, we shell-quote the entire string...
>putc ('\n', fp);
>
>/* Since nbdkit 1.24, $nbd is a synonym for $uri. */
> diff --git a/server/main.c b/server/main.c
> index 2a332bfdd..54eb348ba 100644
> --- a/server/main.c
> +static char *
> +make_uri (void)
> +
> + switch (service_mode) {
> + case SERVICE_MODE_UNIXSOCKET:
> +fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
> +if (export_name && strcmp (export_name, "") != 0) {
> + putc ('/', fp);
> + uri_quote (export_name, fp);
> +}
> +fprintf (fp, "?socket=");
> +uri_quote (unixsocket, fp);
...where the manual shell-quoting is no longer injected. Yes, this
looks correct (the appearance of the quoting, using '' instead of \,
may be different, but the resulting string as parsed by the shell is
the same).
> +break;
> + case SERVICE_MODE_VSOCK:
> +/* 1 = VMADDR_CID_LOCAL */
> +fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" : "");
> +if (port) {
> + putc (':', fp);
> + fputs (port, fp);
> +}
> +if (export_name && strcmp (export_name, "") != 0) {
> + putc ('/', fp);
> + uri_quote (export_name, fp);
> +}
> +break;
> + case SERVICE_MODE_TCPIP:
> +fprintf (fp, "nbd%s://localhost", tls == 2 ? "s" : "");
> +if (port) {
> + putc (':', fp);
> + fputs (port, fp);
> +}
> +if (export_name && strcmp (export_name, "") != 0) {
> + putc ('/', fp);
> + uri_quote (export_name, fp);
> +}
> +break;
> +
> + case SERVICE_MODE_SOCKET_ACTIVATION:
> + case SERVICE_MODE_LISTEN_STDIN:
> +abort (); /* see above */
> + default:
> +abort ();
Could consolidate these labels, although I don't know if a compiler
would be picky about:
case ...:
/* comment */
default:
abort ();
so I'm also fine leaving it as-is.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
___
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
[Libguestfs] [PATCH nbdkit 04/10] server: Calculate $uri in one place
Move the calculation of $uri to the main function (well, inlined
there), and out of --run code.
This is largely code motion. In theory it changes the content of $uri
since we now shell quote it after generating it, but this ought not to
have any practical effect.
---
server/internal.h | 1 +
server/captive.c | 41 ++--
server/main.c | 79 +++
3 files changed, 82 insertions(+), 39 deletions(-)
diff --git a/server/internal.h b/server/internal.h
index 0652f8a86..656b6d8ce 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -142,6 +142,7 @@ extern const char *user, *group;
extern bool verbose;
extern bool vsock;
extern enum service_mode service_mode;
+extern char *uri;
extern bool configured;
extern int saved_stdin;
extern int saved_stdout;
diff --git a/server/captive.c b/server/captive.c
index 40c4bb2ca..51dafca34 100644
--- a/server/captive.c
+++ b/server/captive.c
@@ -75,45 +75,8 @@ run_command (void)
/* Construct $uri. */
fprintf (fp, "uri=");
- switch (service_mode) {
- case SERVICE_MODE_SOCKET_ACTIVATION:
- case SERVICE_MODE_LISTEN_STDIN:
-break; /* can't form a URI, leave it blank */
- case SERVICE_MODE_UNIXSOCKET:
-fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
-if (export_name && strcmp (export_name, "") != 0) {
- putc ('/', fp);
- uri_quote (export_name, fp);
-}
-fprintf (fp, "\\?socket=");
-uri_quote (unixsocket, fp);
-break;
- case SERVICE_MODE_VSOCK:
-/* 1 = VMADDR_CID_LOCAL */
-fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" : "");
-if (port) {
- putc (':', fp);
- shell_quote (port, fp);
-}
-if (export_name && strcmp (export_name, "") != 0) {
- putc ('/', fp);
- uri_quote (export_name, fp);
-}
-break;
- case SERVICE_MODE_TCPIP:
-fprintf (fp, "nbd%s://localhost", tls == 2 ? "s" : "");
-if (port) {
- putc (':', fp);
- shell_quote (port, fp);
-}
-if (export_name && strcmp (export_name, "") != 0) {
- putc ('/', fp);
- uri_quote (export_name, fp);
-}
-break;
- default:
-abort ();
- }
+ if (uri)
+shell_quote (uri, fp);
putc ('\n', fp);
/* Since nbdkit 1.24, $nbd is a synonym for $uri. */
diff --git a/server/main.c b/server/main.c
index 2a332bfdd..54eb348ba 100644
--- a/server/main.c
+++ b/server/main.c
@@ -66,6 +66,7 @@
#include "ascii-string.h"
#include "exit-with-parent.h"
#include "nbd-protocol.h"
+#include "open_memstream.h"
#include "realpath.h"
#include "strndup.h"
#include "syslog.h"
@@ -79,6 +80,7 @@
#endif
static char *make_random_fifo (void);
+static char *make_uri (void);
static struct backend *open_plugin_so (size_t i, const char *filename,
int short_name);
static struct backend *open_filter_so (struct backend *next, size_t i,
@@ -118,6 +120,7 @@ bool verbose; /* -v */
bool vsock; /* --vsock */
unsigned int socket_activation; /* $LISTEN_FDS and $LISTEN_PID set */
enum service_mode service_mode; /* serving over TCP, Unix, etc */
+char *uri; /* NBD URI */
bool configured;/* .config_complete done */
int saved_stdin = -1; /* dup'd stdin during -s/--run */
int saved_stdout = -1; /* dup'd stdout during -s/--run */
@@ -630,6 +633,11 @@ main (int argc, char *argv[])
else
service_mode = SERVICE_MODE_TCPIP;
+ /* By the point we have enough information to calculate the NBD URI.
+ * Note this may be NULL.
+ */
+ uri = make_uri ();
+
/* The remaining command line arguments are the plugin name and
* parameters. If --help, --version or --dump-plugin were specified
* then we open the plugin so that we can display the per-plugin
@@ -799,6 +807,7 @@ main (int argc, char *argv[])
free (unixsocket);
free (pidfile);
+ free (uri);
if (random_fifo) {
unlink (random_fifo);
@@ -856,6 +865,76 @@ make_random_fifo (void)
return NULL;
}
+static char *
+make_uri (void)
+{
+ FILE *fp;
+ size_t len = 0;
+ char *r = NULL;
+
+ switch (service_mode) {
+ case SERVICE_MODE_SOCKET_ACTIVATION:
+ case SERVICE_MODE_LISTEN_STDIN:
+/* can't form a URI, uri will be NULL */
+return NULL;
+ default: ;
+ }
+
+ fp = open_memstream (, );
+ if (fp == NULL) {
+perror ("open_memstream");
+exit (EXIT_FAILURE);
+ }
+
+ switch (service_mode) {
+ case SERVICE_MODE_UNIXSOCKET:
+fprintf (fp, "nbd%s+unix://", tls == 2 ? "s" : "");
+if (export_name && strcmp (export_name, "") != 0) {
+ putc ('/', fp);
+ uri_quote (export_name, fp);
+}
+fprintf (fp, "?socket=");
+uri_quote (unixsocket, fp);
+break;
+ case SERVICE_MODE_VSOCK:
+/* 1 = VMADDR_CID_LOCAL */
+fprintf (fp, "nbd%s+vsock://1", tls == 2 ? "s" : "");
+if (port) {
+ putc (':', fp);
+ fputs (port, fp);
+}
+if (export_name && strcmp
