On 12/05/10 16:01, Richard W.M. Jones wrote:

...

From fb8ae842ef1195fe3896e3f83a97762d27364abb Mon Sep 17 00:00:00 2001 From: Richard Jones <rjones@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