Libguestfs - May 2010 - [PATCH] guestfish -i and virt-inspector work on filenames containing spaces (RHBZ#507810).

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
New in Fedora 11: Fedora Windows cross-compiler. Compile Windows
programs, test, and build Windows installers. Over 70 libraries supprt'd
http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw
-------------- next part --------------
>From fb8ae842ef1195fe3896e3f83a97762d27364abb Mon Sep 17 00:00:00 2001
From: Richard Jones <rjones at redhat.com>
Date: Wed, 12 May 2010 15:58:00 +0100
Subject: [PATCH] guestfish -i and virt-inspector work on filenames containing
spaces (RHBZ#507810).

This commit fixes a long-standing bug which prevented guestfish -i
and virt-inspector from working on disk images which had a space
in the filename (or other unsafe characters).  It works by ensuring
that the strings passed between guestfish -i and virt-inspector are
quoted correctly in both directions.

Note that this commit adds a dependency from virt-inspector to
the perl module String::ShellQuote.  We have previously used this
module in virt-make-fs.
---
 fish/fish.c              |  111 ++++++++++++++++++++++++++++++++++++----------
 inspector/virt-inspector |    9 +++-
 2 files changed, 94 insertions(+), 26 deletions(-)

diff --git a/fish/fish.c b/fish/fish.c
index 75bc2cf..34188d4 100644
--- a/fish/fish.c
+++ b/fish/fish.c
@@ -1,5 +1,5 @@
 /* guestfish - the filesystem interactive shell
- * Copyright (C) 2009 Red Hat Inc.
+ * Copyright (C) 2009-2010 Red Hat Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -66,6 +66,7 @@ static void cmdline (char *argv[], int optind, int argc);
 static void initialize_readline (void);
 static void cleanup_readline (void);
 static void add_history_line (const char *);
+static int print_shell_quote (FILE *stream, const char *str);
 
 /* Currently open libguestfs handle. */
 guestfs_h *g;
@@ -362,9 +363,6 @@ main (int argc, char *argv[])
 
   /* Inspector mode invalidates most of the other arguments. */
   if (inspector) {
-    char cmd[1024];
-    int r;
-
     if (drvs || mps || remote_control_listen || remote_control ||
         guestfs_get_selinux (g)) {
       fprintf (stderr, _("%s: cannot use -i option with -a, -m, -N, "
@@ -379,44 +377,90 @@ main (int argc, char *argv[])
       exit (EXIT_FAILURE);
     }
 
-    strcpy (cmd, "a=`virt-inspector");
+    char *cmd;
+    size_t cmdlen;
+    FILE *fp = open_memstream (&cmd, &cmdlen);
+    if (fp == NULL) {
+      perror ("open_memstream");
+      exit (EXIT_FAILURE);
+    }
+
+    //fprintf (fp, "virt-inspector");
+    fprintf (fp, "inspector/run-inspector-locally");
     while (optind < argc) {
-      if (strlen (cmd) + strlen (argv[optind]) + strlen (real_argv0) + 60
-          >= sizeof cmd) {
-        fprintf (stderr,
-                 _("%s: virt-inspector command too long for fixed-size
buffer\n"),
-                 program_name);
-        exit (EXIT_FAILURE);
-      }
-      strcat (cmd, " '");
-      strcat (cmd, argv[optind]);
-      strcat (cmd, "'");
+      fputc (' ', fp);
+      print_shell_quote (fp, argv[optind]);
       optind++;
     }
 
     if (read_only)
-      strcat (cmd, " --ro-fish");
+      fprintf (fp, " --ro-fish");
     else
-      strcat (cmd, " --fish");
+      fprintf (fp, " --fish");
+
+    if (fclose (fp) == -1) {
+      perror ("fclose");
+      exit (EXIT_FAILURE);
+    }
+
+    if (verbose)
+      fprintf (stderr,
+               "%s -i: running: %s\n", program_name, cmd);
+
+    FILE *pp = popen (cmd, "r");
+    if (pp == NULL) {
+      perror (cmd);
+      exit (EXIT_FAILURE);
+    }
+
+    char *cmd2;
+    fp = open_memstream (&cmd2, &cmdlen);
+    if (fp == NULL) {
+      perror ("open_memstream");
+      exit (EXIT_FAILURE);
+    }
 
-    sprintf (&cmd[strlen(cmd)], "` && %s $a",
real_argv0);
+    fprintf (fp, "%s", real_argv0);
 
     if (guestfs_get_verbose (g))
-      strcat (cmd, " -v");
+      fprintf (fp, " -v");
     if (!guestfs_get_autosync (g))
-      strcat (cmd, " -n");
+      fprintf (fp, " -n");
     if (guestfs_get_trace (g))
-      strcat (cmd, " -x");
+      fprintf (fp, " -x");
+
+    char *insp = NULL;
+    size_t insplen;
+    if (getline (&insp, &insplen, pp) == -1) {
+      perror (cmd);
+      exit (EXIT_FAILURE);
+    }
+    fprintf (fp, " %s", insp);
+
+    if (pclose (pp) == -1) {
+      perror (cmd);
+      exit (EXIT_FAILURE);
+    }
+
+    if (fclose (fp) == -1) {
+      perror ("fclose");
+      exit (EXIT_FAILURE);
+    }
 
     if (verbose)
       fprintf (stderr,
-               "%s -i: running virt-inspector command:\n%s\n",
program_name, cmd);
+               "%s -i: running: %s\n", program_name, cmd2);
 
-    r = system (cmd);
+    int r = system (cmd2);
     if (r == -1) {
-      perror ("system");
+      perror (cmd2);
       exit (EXIT_FAILURE);
     }
+
+    free (cmd);
+    free (cmd2);
+    free (insp);
+
     exit (WEXITSTATUS (r));
   }
 
@@ -1616,3 +1660,22 @@ file_out (const char *arg)
   }
   return ret;
 }
+
+static int
+print_shell_quote (FILE *stream, const char *str)
+{
+#define SAFE(c) (c_isalnum((c)) ||					\
+                 (c) == '/' || (c) == '-' || (c) == '_'
|| (c) == '.')
+  int i, len;
+
+  for (i = len = 0; str[i]; ++i) {
+    if (!SAFE(str[i])) {
+      putc ('\\', stream);
+      len ++;
+    }
+    putc (str[i], stream);
+    len ++;
+  }
+
+  return len;
+}
diff --git a/inspector/virt-inspector b/inspector/virt-inspector
index 4428ecd..f62d21f 100755
--- a/inspector/virt-inspector
+++ b/inspector/virt-inspector
@@ -27,6 +27,7 @@ use Pod::Usage;
 use Getopt::Long;
 use Data::Dumper;
 use XML::Writer;
+use String::ShellQuote qw(shell_quote);
 use Locale::TextDomain 'libguestfs';
 
 # Optional:
@@ -298,13 +299,17 @@ if ($output eq "fish" || $output eq
"ro-fish") {
         print "--ro ";
     }
 
-    print "-a $_ " foreach @images;
+    foreach (@images) {
+        printf "-a %s ", shell_quote ($_);
+    }
 
     my $mounts = $oses->{$root_dev}->{mounts};
     # Have to mount / first.  Luckily '/' is early in the ASCII
     # character set, so this should be OK.
     foreach (sort keys %$mounts) {
-        print "-m $mounts->{$_}:$_ " if $_ ne "swap"
&& $_ ne "none";
+        if ($_ ne "swap" && $_ ne "none") {
+            printf "-m %s ", shell_quote
("$mounts->{$_}:$_");
+        }
     }
     print "\n"
 }
-- 
1.6.6.1

On 12/05/10 16:01, Richard W.M. Jones wrote:
>>From fb8ae842ef1195fe3896e3f83a97762d27364abb Mon Sep 17 00:00:00 2001
> From: Richard Jones <rjones at redhat.com>
> Date: Wed, 12 May 2010 15:58:00 +0100
> Subject: [PATCH] guestfish -i and virt-inspector work on filenames
containing spaces (RHBZ#507810).
> 
> This commit fixes a long-standing bug which prevented guestfish -i
> and virt-inspector from working on disk images which had a space
> in the filename (or other unsafe characters).  It works by ensuring
> that the strings passed between guestfish -i and virt-inspector are
> quoted correctly in both directions.
> 
> Note that this commit adds a dependency from virt-inspector to
> the perl module String::ShellQuote.  We have previously used this
> module in virt-make-fs.
> ---
>  fish/fish.c              |  111
++++++++++++++++++++++++++++++++++++----------
>  inspector/virt-inspector |    9 +++-
>  2 files changed, 94 insertions(+), 26 deletions(-)
> 
> diff --git a/fish/fish.c b/fish/fish.c
> index 75bc2cf..34188d4 100644
> --- a/fish/fish.c
> +++ b/fish/fish.c
> @@ -1,5 +1,5 @@
>  /* guestfish - the filesystem interactive shell
> - * Copyright (C) 2009 Red Hat Inc.
> + * Copyright (C) 2009-2010 Red Hat Inc.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -66,6 +66,7 @@ static void cmdline (char *argv[], int optind, int argc);
>  static void initialize_readline (void);
>  static void cleanup_readline (void);
>  static void add_history_line (const char *);
> +static int print_shell_quote (FILE *stream, const char *str);
>  
>  /* Currently open libguestfs handle. */
>  guestfs_h *g;
> @@ -362,9 +363,6 @@ main (int argc, char *argv[])
>  
>    /* Inspector mode invalidates most of the other arguments. */
>    if (inspector) {
> -    char cmd[1024];
> -    int r;
> -
>      if (drvs || mps || remote_control_listen || remote_control ||
>          guestfs_get_selinux (g)) {
>        fprintf (stderr, _("%s: cannot use -i option with -a, -m, -N,
"
> @@ -379,44 +377,90 @@ main (int argc, char *argv[])
>        exit (EXIT_FAILURE);
>      }
>  
> -    strcpy (cmd, "a=`virt-inspector");
> +    char *cmd;
> +    size_t cmdlen;
> +    FILE *fp = open_memstream (&cmd, &cmdlen);
> +    if (fp == NULL) {
> +      perror ("open_memstream");
> +      exit (EXIT_FAILURE);
> +    }
> +
> +    //fprintf (fp, "virt-inspector");
> +    fprintf (fp, "inspector/run-inspector-locally");
I don't think ^^^ is still supposed to be there.
>      while (optind < argc) {
> -      if (strlen (cmd) + strlen (argv[optind]) + strlen (real_argv0) + 60
> -          >= sizeof cmd) {
> -        fprintf (stderr,
> -                 _("%s: virt-inspector command too long for
fixed-size buffer\n"),
> -                 program_name);
> -        exit (EXIT_FAILURE);
> -      }
> -      strcat (cmd, " '");
> -      strcat (cmd, argv[optind]);
> -      strcat (cmd, "'");
> +      fputc (' ', fp);
> +      print_shell_quote (fp, argv[optind]);
>        optind++;
>      }
>  
>      if (read_only)
> -      strcat (cmd, " --ro-fish");
> +      fprintf (fp, " --ro-fish");
>      else
> -      strcat (cmd, " --fish");
> +      fprintf (fp, " --fish");
> +
> +    if (fclose (fp) == -1) {
> +      perror ("fclose");
> +      exit (EXIT_FAILURE);
> +    }
> +
> +    if (verbose)
> +      fprintf (stderr,
> +               "%s -i: running: %s\n", program_name, cmd);
> +
> +    FILE *pp = popen (cmd, "r");
> +    if (pp == NULL) {
> +      perror (cmd);
> +      exit (EXIT_FAILURE);
> +    }
> +
> +    char *cmd2;
> +    fp = open_memstream (&cmd2, &cmdlen);
> +    if (fp == NULL) {
> +      perror ("open_memstream");
> +      exit (EXIT_FAILURE);
> +    }
>  
> -    sprintf (&cmd[strlen(cmd)], "` && %s $a",
real_argv0);
> +    fprintf (fp, "%s", real_argv0);
>  
>      if (guestfs_get_verbose (g))
> -      strcat (cmd, " -v");
> +      fprintf (fp, " -v");
>      if (!guestfs_get_autosync (g))
> -      strcat (cmd, " -n");
> +      fprintf (fp, " -n");
>      if (guestfs_get_trace (g))
> -      strcat (cmd, " -x");
> +      fprintf (fp, " -x");
> +
> +    char *insp = NULL;
> +    size_t insplen;
> +    if (getline (&insp, &insplen, pp) == -1) {
> +      perror (cmd);
> +      exit (EXIT_FAILURE);
> +    }
> +    fprintf (fp, " %s", insp);
> +
> +    if (pclose (pp) == -1) {
> +      perror (cmd);
> +      exit (EXIT_FAILURE);
> +    }
> +
> +    if (fclose (fp) == -1) {
> +      perror ("fclose");
> +      exit (EXIT_FAILURE);
> +    }
>  
>      if (verbose)
>        fprintf (stderr,
> -               "%s -i: running virt-inspector command:\n%s\n",
program_name, cmd);
> +               "%s -i: running: %s\n", program_name, cmd2);
>  
> -    r = system (cmd);
> +    int r = system (cmd2);
Incidentally, why don't we just exec() here?
>      if (r == -1) {
> -      perror ("system");
> +      perror (cmd2);
>        exit (EXIT_FAILURE);
>      }
> +
> +    free (cmd);
> +    free (cmd2);
> +    free (insp);
> +
>      exit (WEXITSTATUS (r));
>    }
>  
> @@ -1616,3 +1660,22 @@ file_out (const char *arg)
>    }
>    return ret;
>  }
> +
> +static int
> +print_shell_quote (FILE *stream, const char *str)
> +{
> +#define SAFE(c) (c_isalnum((c)) ||					\
> +                 (c) == '/' || (c) == '-' || (c) ==
'_' || (c) == '.')
> +  int i, len;
> +
> +  for (i = len = 0; str[i]; ++i) {
> +    if (!SAFE(str[i])) {
> +      putc ('\\', stream);
> +      len ++;
> +    }
> +    putc (str[i], stream);
> +    len ++;
> +  }
> +
> +  return len;
> +}
The return value of print_shell_quote isn't used. No need to track len.
> diff --git a/inspector/virt-inspector b/inspector/virt-inspector
> index 4428ecd..f62d21f 100755
> --- a/inspector/virt-inspector
> +++ b/inspector/virt-inspector
> @@ -27,6 +27,7 @@ use Pod::Usage;
>  use Getopt::Long;
>  use Data::Dumper;
>  use XML::Writer;
> +use String::ShellQuote qw(shell_quote);
String::ShellQuote isn't in RHEL 5. Are you really sure you want this?
>  use Locale::TextDomain 'libguestfs';
>  
>  # Optional:
> @@ -298,13 +299,17 @@ if ($output eq "fish" || $output eq
"ro-fish") {
>          print "--ro ";
>      }
>  
> -    print "-a $_ " foreach @images;
> +    foreach (@images) {
> +        printf "-a %s ", shell_quote ($_);
> +    }
>  
>      my $mounts = $oses->{$root_dev}->{mounts};
>      # Have to mount / first.  Luckily '/' is early in the ASCII
>      # character set, so this should be OK.
>      foreach (sort keys %$mounts) {
> -        print "-m $mounts->{$_}:$_ " if $_ ne
"swap" && $_ ne "none";
> +        if ($_ ne "swap" && $_ ne "none") {
> +            printf "-m %s ", shell_quote
("$mounts->{$_}:$_");
> +        }
>      }
>      print "\n"
>  }
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