[libvirt] [PATCH 2/2] virsh: support VISUAL, and allow metacharacters in EDITOR

[Eric Blake]

Common Unix practice is to prefer VISUAL over EDITOR, particularly if
the editor of choice spawns a new window.  Thus, it is also common to
see settings like EDITOR='emacs -nw', with the expectation that the
shell will parse this as an argument to 'emacs' and not try to invoke
a file containing a space.

If a user puts junk in EDITOR, they deserve what they get (much more
than virsh will misbehave); furthermore, sudo scrubs EDITOR by
default.  So the blind use of metacharacters in EDITOR should not be
considered too much of a security issue.

* tools/virsh.c (editFile): Prefer VISUAL over EDITOR.  Don't
reject shell metacharacters in EDITOR.
* tools/virsh.pod (edit, net-edit, ENVIRONMENT): Document VISUAL.
---
 tools/virsh.c   |   19 ++++++++-----------
 tools/virsh.pod |   15 ++++++++++-----
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index aa85ee6..9f3b197 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -7381,20 +7381,17 @@ editFile (vshControl *ctl, const char *filename)
     char *command;
     int command_ret;

-    editor = getenv ("EDITOR");
+    editor = getenv ("VISUAL");
+    if (!editor) editor = getenv ("EDITOR");
     if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */

-    /* Check the editor doesn't contain shell meta-characters, and if
-     * it does, refuse to run.
+    /* Check that filename doesn't contain shell meta-characters, and
+     * if it does, refuse to run.  Follow the Unix conventions for
+     * EDITOR: the user can intentionally specify command options, so
+     * we don't protect any shell metacharacters there.  Lots more
+     * than virsh will misbehave if EDITOR has bogus contents (which
+     * is why sudo scrubs it by default).
      */
-    if (strspn (editor, ACCEPTED_CHARS) != strlen (editor)) {
-        vshError(ctl,
-                 _("%s: $EDITOR environment variable contains shell meta or "
-                   "other unacceptable characters"),
-                 editor);
-        return -1;
-    }
-    /* Same for the filename. */
     if (strspn (filename, ACCEPTED_CHARS) != strlen (filename)) {
         vshError(ctl,
                  _("%s: temporary filename contains shell meta or other "
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 302de18..3707916 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -332,8 +332,8 @@ This is equivalent to:
  virsh define domain.xml
 except that it does some error checking.

-The editor used can be supplied by the C<$EDITOR> environment
-variable, or if that is not defined defaults to C<vi>.
+The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment
+variables, and defaults to C<vi>.

 =item B<migrate> optional I<--live> I<--suspend> I<domain-id> I<desturi> I<migrateuri>

@@ -557,8 +557,8 @@ This is equivalent to:
  virsh define network.xml
 except that it does some error checking.

-The editor used can be supplied by the C<$EDITOR> environment
-variable, or if that is not defined defaults to C<vi>.
+The editor used can be supplied by the C<$VISUAL> or C<$EDITOR> environment
+variables, and defaults to C<vi>.

 =item B<net-list> optional I<--inactive> or I<--all>

@@ -639,10 +639,15 @@ of C<virsh>
 The hypervisor to connect to by default. Set this to a URI, in the same
 format as accepted by the B<connect> option.

-=item EDITOR
+=item VISUAL

 The editor to use by the B<edit> and B<net-edit> options.

+=item EDITOR
+
+The editor to use by the B<edit> and B<net-edit> options, if C<VISUAL>
+is not set.
+
 =item LIBVIRT_DEBUG=LEVEL

 Turn on verbose debugging of all libvirt API calls. Valid levels are
-- 
1.6.6.1