Richard W.M. Jones
2009-Jul-27 21:42 UTC
[Libguestfs] [PATCH] Replace shell_quote function with %Q and %R printf specifiers.
At the moment the daemon code contains an incredibly hairy function called shell_quote for safely quoting strings passed to the shell. The patch replaces that with a glibc custom printf format (actually two, but very closely related), %Q and %R. %Q is like %s but it safely shell quotes the string. %R is like %Q but it prefixes the path with /sysroot. Example usage (w/o error checks): asprintf (&cmd, "zcat %R | tar xvf -", path); ==> "zcat /sysroot/path\ with\ spaces | tar xvf -" Rich. [One of the nice things about OCaml is this sort of safe shell quoting is built in to the stdlib functions]. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones Read my programming blog: http://rwmj.wordpress.com Fedora now supports 75 OCaml packages (the OPEN alternative to F#) http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora -------------- next part -------------->From fcc190a856a537bb1ca429714e54468ca3514f33 Mon Sep 17 00:00:00 2001From: Richard Jones <rjones at trick.home.annexia.org> Date: Mon, 27 Jul 2009 22:27:45 +0100 Subject: [PATCH 1/2] Replace shell_quote function with %Q and %R printf specifiers. %Q => simple shell quoted string %R => path will be prefixed by /sysroot eg. snprintf (cmd, sizeof cmd, "cat %R", path); system (cmd); --- daemon/daemon.h | 17 ++++++++++++- daemon/file.c | 24 ++++++++----------- daemon/find.c | 11 +------- daemon/grub.c | 9 ++----- daemon/guestfsd.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++ daemon/initrd.c | 16 ++++--------- daemon/tar.c | 60 +++++++++++++++++++----------------------------- 7 files changed, 124 insertions(+), 78 deletions(-) diff --git a/daemon/daemon.h b/daemon/daemon.h index c2bbf3e..2ab1574 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -54,12 +54,25 @@ extern int commandrv (char **stdoutput, char **stderror, extern char **split_lines (char *str); -extern int shell_quote (char *out, int len, const char *in); - extern int device_name_translation (char *device, const char *func); extern void udev_settle (void); +/* This just stops gcc from giving a warning about our custom + * printf formatters %Q and %R. + */ +static inline int +asprintf_nowarn (char **strp, const char *fmt, ...) +{ + int r; + va_list args; + + va_start (args, fmt); + r = vasprintf (strp, fmt, args); + va_end (args); + return r; +} + /*-- in names.c (auto-generated) --*/ extern const char *function_names[]; diff --git a/daemon/file.c b/daemon/file.c index 7c46722..6062c50 100644 --- a/daemon/file.c +++ b/daemon/file.c @@ -440,6 +440,7 @@ char * do_zfile (char *method, char *path) { int len; + const char *zcat; char *cmd; FILE *fp; char line[256]; @@ -447,27 +448,22 @@ do_zfile (char *method, char *path) NEED_ROOT (NULL); ABS_PATH (path, NULL); - len = 2 * strlen (path) + sysroot_len + 64; - cmd = malloc (len); - if (!cmd) { - reply_with_perror ("malloc"); - return NULL; - } - if (strcmp (method, "gzip") == 0 || strcmp (method, "compress") == 0) - strcpy (cmd, "zcat"); + zcat = "zcat"; else if (strcmp (method, "bzip2") == 0) - strcpy (cmd, "bzcat"); + zcat = "bzcat"; else { - free (cmd); reply_with_error ("zfile: unknown method"); return NULL; } - strcat (cmd, " "); - strcat (cmd, sysroot); - shell_quote (cmd + strlen (cmd), len - strlen (cmd), path); - strcat (cmd, " | file -bsL -"); + if (asprintf_nowarn (&cmd, "%s %R | file -bsL -", zcat, path) == -1) { + reply_with_perror ("asprintf"); + return NULL; + } + + if (verbose) + fprintf (stderr, "%s\n", cmd); fp = popen (cmd, "r"); if (fp == NULL) { diff --git a/daemon/find.c b/daemon/find.c index d882953..40f1b3b 100644 --- a/daemon/find.c +++ b/daemon/find.c @@ -83,21 +83,14 @@ do_find (char *dir) sysrootdirlen = strlen (sysrootdir); /* Assemble the external find command. */ - len = 2 * sysrootdirlen + 32; - cmd = malloc (len); - if (!cmd) { + if (asprintf_nowarn (&cmd, "find %Q -print0", sysrootdir) == -1) { reply_with_perror ("malloc"); free (sysrootdir); return NULL; } - strcpy (cmd, "find "); - shell_quote (cmd+5, len-5, sysrootdir); - free (sysrootdir); - strcat (cmd, " -print0"); - if (verbose) - printf ("%s\n", cmd); + fprintf (stderr, "%s\n", cmd); fp = popen (cmd, "r"); if (fp == NULL) { diff --git a/daemon/grub.c b/daemon/grub.c index 3b1768f..86e812e 100644 --- a/daemon/grub.c +++ b/daemon/grub.c @@ -28,7 +28,7 @@ int do_grub_install (char *root, char *device) { - int r, len; + int r; char *err; char *buf; @@ -36,13 +36,10 @@ do_grub_install (char *root, char *device) ABS_PATH (root, -1); IS_DEVICE (device, -1); - len = strlen (root) + sysroot_len + 64; - buf = malloc (len); - if (!buf) { - reply_with_perror ("malloc"); + if (asprintf_nowarn (&buf, "--root-directory=%R", root) == -1) { + reply_with_perror ("asprintf"); return -1; } - snprintf (buf, len, "--root-directory=%s%s", sysroot, root); r = command (NULL, &err, "/sbin/grub-install", buf, device, NULL); free (buf); diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 5c250f0..6c044b6 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -36,6 +36,7 @@ #include <fcntl.h> #include <ctype.h> #include <signal.h> +#include <printf.h> #include "daemon.h" @@ -47,6 +48,10 @@ static void usage (void); int verbose = 0; +static int print_shell_quote (FILE *stream, const struct printf_info *info, const void *const *args); +static int print_sysroot_shell_quote (FILE *stream, const struct printf_info *info, const void *const *args); +static int print_arginfo (const struct printf_info *info, size_t n, int *argtypes, int *size); + /* Location to mount root device. */ const char *sysroot = "/sysroot"; /* No trailing slash. */ int sysroot_len = 8; @@ -76,6 +81,10 @@ main (int argc, char *argv[]) uint32_t len; struct sigaction sa; + /* http://udrepper.livejournal.com/20948.html */ + register_printf_specifier ('Q', print_shell_quote, print_arginfo); + register_printf_specifier ('R', print_sysroot_shell_quote, print_arginfo); + for (;;) { c = getopt_long (argc, argv, options, long_options, NULL); if (c == -1) break; @@ -229,6 +238,8 @@ main (int argc, char *argv[]) * * Caller must check for NULL and call reply_with_perror ("malloc") * if it is. Caller must also free the string. + * + * See also the custom %R printf formatter which does shell quoting too. */ char * sysroot_path (const char *path) @@ -685,6 +696,59 @@ split_lines (char *str) return lines; } +/* printf helper function so we can use %Q ("quoted") and %R to print + * shell-quoted strings. + * + * %Q => simple shell quoted string + * %R => path will be prefixed by /sysroot + * + * eg. snprintf (cmd, sizeof cmd, "cat %R", path); system (cmd); + */ +static int +print_shell_quote (FILE *stream, + const struct printf_info *info, + const void *const *args) +{ +#define SAFE(c) (isalnum((c)) || \ + (c) == '/' || (c) == '-' || (c) == '_' || (c) == '.') + int i, len; + const char *str = *((const char **) (args[0])); + + for (i = len = 0; str[i]; ++i) { + if (!SAFE(str[i])) { + putc ('\\', stream); + len ++; + } + putc (str[i], stream); + len ++; + } + + return len; +} + +static int +print_sysroot_shell_quote (FILE *stream, + const struct printf_info *info, + const void *const *args) +{ +#define SAFE(c) (isalnum((c)) || \ + (c) == '/' || (c) == '-' || (c) == '_' || (c) == '.') + fputs (sysroot, stream); + return sysroot_len + print_shell_quote (stream, info, args); +} + +static int +print_arginfo (const struct printf_info *info, + size_t n, int *argtypes, int *size) +{ + if (n > 0) { + argtypes[0] = PA_STRING; + size[0] = sizeof (const char *); + } + return 1; +} + +#if 0 /* Quote 'in' for the shell, and write max len-1 bytes to out. The * result will be NUL-terminated, even if it is truncated. * @@ -721,6 +785,7 @@ shell_quote (char *out, int len, const char *in) return outlen; } +#endif /* Perform device name translation. Don't call this directly - * use the IS_DEVICE macro. diff --git a/daemon/initrd.c b/daemon/initrd.c index 392b811..59749bb 100644 --- a/daemon/initrd.c +++ b/daemon/initrd.c @@ -31,29 +31,23 @@ char ** do_initrd_list (char *path) { FILE *fp; - int len; char *cmd; char filename[PATH_MAX]; char **filenames = NULL; int size = 0, alloc = 0; + size_t len; NEED_ROOT (NULL); ABS_PATH (path, NULL); /* "zcat /sysroot/<path> | cpio --quiet -it", but path must be quoted. */ - len = 64 + sysroot_len + 2 * strlen (path); - cmd = malloc (len); - if (!cmd) { - reply_with_perror ("malloc"); + if (asprintf_nowarn (&cmd, "zcat %R | cpio --quiet -it", path) == -1) { + reply_with_perror ("asprintf"); return NULL; } - strcpy (cmd, "zcat "); - strcat (cmd, sysroot); - shell_quote (cmd+13, len-13, path); - strcat (cmd, " | cpio --quiet -it"); - - fprintf (stderr, "%s\n", cmd); + if (verbose) + fprintf (stderr, "%s\n", cmd); fp = popen (cmd, "r"); if (fp == NULL) { diff --git a/daemon/tar.c b/daemon/tar.c index 3008574..39b983c 100644 --- a/daemon/tar.c +++ b/daemon/tar.c @@ -38,7 +38,7 @@ fwrite_cb (void *fp_ptr, const void *buf, int len) int do_tar_in (char *dir) { - int err, r, len; + int err, r; FILE *fp; char *cmd; @@ -49,19 +49,16 @@ do_tar_in (char *dir) } /* "tar -C /sysroot%s -xf -" but we have to quote the dir. */ - len = 2 * strlen (dir) + sysroot_len + 32; - cmd = malloc (len); - if (!cmd) { + if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) { err = errno; cancel_receive (); errno = err; - reply_with_perror ("malloc"); + reply_with_perror ("asprintf"); return -1; } - strcpy (cmd, "tar -C "); - strcat (cmd, sysroot); - shell_quote (cmd+15, len-15, dir); - strcat (cmd, " -xf -"); + + if (verbose) + fprintf (stderr, "%s\n", cmd); fp = popen (cmd, "w"); if (fp == NULL) { @@ -104,7 +101,7 @@ do_tar_in (char *dir) int do_tar_out (char *dir) { - int r, len; + int r; FILE *fp; char *cmd; char buf[GUESTFS_MAX_CHUNK_SIZE]; @@ -113,16 +110,13 @@ do_tar_out (char *dir) ABS_PATH (dir, -1); /* "tar -C /sysroot%s -cf - ." but we have to quote the dir. */ - len = 2 * strlen (dir) + sysroot_len + 32; - cmd = malloc (len); - if (!cmd) { - reply_with_perror ("malloc"); + if (asprintf_nowarn (&cmd, "tar -C %R -cf - .", dir) == -1) { + reply_with_perror ("asprintf"); return -1; } - strcpy (cmd, "tar -C "); - strcat (cmd, sysroot); - shell_quote (cmd+15, len-15, dir); - strcat (cmd, " -cf - ."); + + if (verbose) + fprintf (stderr, "%s\n", cmd); fp = popen (cmd, "r"); if (fp == NULL) { @@ -166,7 +160,7 @@ do_tar_out (char *dir) int do_tgz_in (char *dir) { - int err, r, len; + int err, r; FILE *fp; char *cmd; @@ -177,19 +171,16 @@ do_tgz_in (char *dir) } /* "tar -C /sysroot%s -zxf -" but we have to quote the dir. */ - len = 2 * strlen (dir) + sysroot_len + 32; - cmd = malloc (len); - if (!cmd) { + if (asprintf_nowarn (&cmd, "tar -C %R -zxf -", dir) == -1) { err = errno; cancel_receive (); errno = err; - reply_with_perror ("malloc"); + reply_with_perror ("asprintf"); return -1; } - strcpy (cmd, "tar -C "); - strcat (cmd, sysroot); - shell_quote (cmd+15, len-15, dir); - strcat (cmd, " -zxf -"); + + if (verbose) + fprintf (stderr, "%s\n", cmd); fp = popen (cmd, "w"); if (fp == NULL) { @@ -232,7 +223,7 @@ do_tgz_in (char *dir) int do_tgz_out (char *dir) { - int r, len; + int r; FILE *fp; char *cmd; char buf[GUESTFS_MAX_CHUNK_SIZE]; @@ -241,16 +232,13 @@ do_tgz_out (char *dir) ABS_PATH (dir, -1); /* "tar -C /sysroot%s -zcf - ." but we have to quote the dir. */ - len = 2 * strlen (dir) + sysroot_len + 32; - cmd = malloc (len); - if (!cmd) { - reply_with_perror ("malloc"); + if (asprintf_nowarn (&cmd, "tar -C %R -zcf - .", dir) == -1) { + reply_with_perror ("asprintf"); return -1; } - strcpy (cmd, "tar -C "); - strcat (cmd, sysroot); - shell_quote (cmd+15, len-15, dir); - strcat (cmd, " -zcf - ."); + + if (verbose) + fprintf (stderr, "%s\n", cmd); fp = popen (cmd, "r"); if (fp == NULL) { -- 1.6.2.5
Daniel P. Berrange
2009-Jul-27 22:09 UTC
[Libguestfs] [PATCH] Replace shell_quote function with %Q and %R printf specifiers.
On Mon, Jul 27, 2009 at 10:42:17PM +0100, Richard W.M. Jones wrote:> > At the moment the daemon code contains an incredibly hairy function > called shell_quote for safely quoting strings passed to the shell. > > The patch replaces that with a glibc custom printf format (actually > two, but very closely related), %Q and %R.Wow, I didn't know you could register custom printf formats !> + /* http://udrepper.livejournal.com/20948.html */ > + register_printf_specifier ('Q', print_shell_quote, print_arginfo); > + register_printf_specifier ('R', print_sysroot_shell_quote, print_arginfo);Perhaps want to see if you can also make it work with /* Obsolete interface similar to register_printf_specifier. It can only handle basic data types because the ARGINFO callback does not return information on the size of the user-defined type. */ extern int register_printf_function (int __spec, printf_function __func, printf_arginfo_function __arginfo) so that libguestfs still compiles on older distros like RHEL-5/EPEL5, unless they're explicitly out of scope as targets now ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Matthew Booth
2009-Jul-27 22:19 UTC
[Libguestfs] [PATCH] Replace shell_quote function with %Q and %R printf specifiers.
On 27/07/09 22:42, Richard W.M. Jones wrote:> At the moment the daemon code contains an incredibly hairy function > called shell_quote for safely quoting strings passed to the shell. > > The patch replaces that with a glibc custom printf format (actually > two, but very closely related), %Q and %R. > > %Q is like %s but it safely shell quotes the string. > > %R is like %Q but it prefixes the path with /sysroot. >Nice. There's a danger here, though, of increasing the barrier to understandability. I can imagine looking at a printf containing %Q or %R and wasting a huge amount of time on google fruitlessly trying to work out what on earth it's on about. With that in mind I'd definitely add something to HACKING. If it weren't for the nowarn wrapper happening to provide a weirdness hook, I might also put add a comment above every printf which uses it. I would, however, add a much bigger comment to asprintf_nowarn, pointing to the implementations. I also don't think %R is what this feature is about. It seems too trivial for a fairly off-the-wall trick. You can just as easily write: #define SYSROOT "/sysroot" snprintf (cmd, sizeof cmd, "cat " SYSROOT "%s", path); It's only slightly longer, and a whole lot more readable to the vast majority of C programmers. Lastly, avoid using inline. The compiler will inline a function if it makes sense. Matt -- Matthew Booth, RHCA, RHCSS Red Hat Engineering, Virtualisation Team M: +44 (0)7977 267231 GPG ID: D33C3490 GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490