From 0f2e9c84e9ff1071260770930068642ecc8ac0d9 Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Mon, 27 Jul 2009 22:27:45 +0100 Subject: [PATCH] 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); --- HACKING | 22 +++++++++++ daemon/configure.ac | 16 ++++++++ daemon/daemon.h | 18 ++++++++- daemon/file.c | 24 +++++------- daemon/find.c | 11 +----- daemon/grub.c | 9 ++--- daemon/guestfsd.c | 104 +++++++++++++++++++++++++++++++++++++++------------- daemon/initrd.c | 16 +++----- daemon/tar.c | 60 ++++++++++++------------------ 9 files changed, 176 insertions(+), 104 deletions(-) diff --git a/HACKING b/HACKING index 42f4f77..336fc7c 100644 --- a/HACKING +++ b/HACKING @@ -126,3 +126,25 @@ into the appliance. Debugging messages are never translated, since they are intended for the programmers. + +Extended printf +---------------------------------------------------------------------- + +In the daemon code we have created custom printf formatters %Q and %R, +which are used to do shell quoting. + +%Q => Simple shell quoted string. Any spaces or other shell characters + are escaped for you. + +%R => Same as %Q except the string is treated as a path which is prefixed + by the sysroot. + +eg. + +asprintf (&cmd, "cat %R", path); +==> "cat /sysroot/some\ path\ with\ spaces" + +Note: Do NOT use these when you are passing parameters to the +command{,r,v,rv}() functions. These parameters do NOT need to be +quoted because they are not passed via the shell (instead, straight to +exec). You probably want to use the sysroot_path() function however. diff --git a/daemon/configure.ac b/daemon/configure.ac index dc6936b..243f353 100644 --- a/daemon/configure.ac +++ b/daemon/configure.ac @@ -65,6 +65,22 @@ AC_CHECK_LIB([portablexdr],[xdrmem_create],[],[ dnl Functions which may not be available in older distributions. AC_CHECK_FUNCS([futimens listxattr llistxattr getxattr lgetxattr setxattr lsetxattr removexattr lremovexattr]) +dnl For modified printf, we need glibc either (old-style) +dnl register_printf_function or (new-style) register_printf_specifier. +AC_CHECK_FUNC([register_printf_specifier],[ + AC_DEFINE([HAVE_REGISTER_PRINTF_SPECIFIER],[1], + [Define to 1 if you have new-style register_printf_specifier]) + ],[ + AC_CHECK_FUNC([register_printf_function],[ + AC_DEFINE([HAVE_REGISTER_PRINTF_FUNCTION],[1], + [Define to 1 if you have old-style register_printf_function]) + ],[ + AC_MSG_FAILURE( +[No support for glibc-style extended printf formatters. + +This means you either have a very old glibc (pre-2.0) or you +are using some other libc where this is not supported.])])]) + dnl Headers. AC_CHECK_HEADERS([attr/xattr.h sys/xattr.h]) diff --git a/daemon/daemon.h b/daemon/daemon.h index c2bbf3e..5bf6f35 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -54,12 +54,26 @@ 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. See HACKING file for more + * info about these. + */ +static 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..7bd384e 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -36,6 +36,7 @@ #include #include #include +#include #include "daemon.h" @@ -47,6 +48,18 @@ 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); +#ifdef HAVE_REGISTER_PRINTF_SPECIFIER +static int print_arginfo (const struct printf_info *info, size_t n, int *argtypes, int *size); +#else +#ifdef HAVE_REGISTER_PRINTF_FUNCTION +static int print_arginfo (const struct printf_info *info, size_t n, int *argtypes); +#else +#error "HAVE_REGISTER_PRINTF_{SPECIFIER|FUNCTION} not defined" +#endif +#endif + /* Location to mount root device. */ const char *sysroot = "/sysroot"; /* No trailing slash. */ int sysroot_len = 8; @@ -76,6 +89,19 @@ main (int argc, char *argv[]) uint32_t len; struct sigaction sa; +#ifdef HAVE_REGISTER_PRINTF_SPECIFIER + /* 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); +#else +#ifdef HAVE_REGISTER_PRINTF_FUNCTION + register_printf_function ('Q', print_shell_quote, print_arginfo); + register_printf_function ('R', print_sysroot_shell_quote, print_arginfo); +#else +#error "HAVE_REGISTER_PRINTF_{SPECIFIER|FUNCTION} not defined" +#endif +#endif + for (;;) { c = getopt_long (argc, argv, options, long_options, NULL); if (c == -1) break; @@ -229,6 +255,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,42 +713,66 @@ split_lines (char *str) return lines; } -/* Quote 'in' for the shell, and write max len-1 bytes to out. The - * result will be NUL-terminated, even if it is truncated. - * - * Returns number of bytes needed, so if result >= len then the buffer - * should have been longer. - * - * XXX This doesn't quote \n correctly (but is still safe). +/* printf helper function so we can use %Q ("quoted") and %R to print + * shell-quoted strings. See HACKING file for more details. */ -int -shell_quote (char *out, int len, const char *in) +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, j; - int outlen = strlen (in); + int i, len; + const char *str = *((const char **) (args[0])); - /* Calculate how much output space this really needs. */ - for (i = 0; in[i]; ++i) - if (!SAFE (in[i])) outlen++; + for (i = len = 0; str[i]; ++i) { + if (!SAFE(str[i])) { + putc ('\\', stream); + len ++; + } + putc (str[i], stream); + len ++; + } - /* Now copy the string, but only up to len-1 bytes. */ - for (i = 0, j = 0; in[i]; ++i) { - int is_safe = SAFE (in[i]); + return len; +} - /* Enough space left to write this character? */ - if (j >= len-1 || (!is_safe && j >= len-2)) - break; +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); +} - if (!is_safe) out[j++] = '\\'; - out[j++] = in[i]; +#ifdef HAVE_REGISTER_PRINTF_SPECIFIER +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 *); } - - out[j] = '\0'; - - return outlen; + return 1; +} +#else +#ifdef HAVE_REGISTER_PRINTF_FUNCTION +static int +print_arginfo (const struct printf_info *info, size_t n, int *argtypes) +{ + if (n > 0) + argtypes[0] = PA_STRING; + return 1; } +#else +#error "HAVE_REGISTER_PRINTF_{SPECIFIER|FUNCTION} not defined" +#endif +#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/ | 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.8.3.1