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

[Daniel P. Berrange]

On Fri, Mar 12, 2010 at 09:50:01AM -0700, Eric Blake wrote:
> 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
> -- 

ACK

This fixes this bug too https://bugzilla.redhat.com/show_bug.cgi?id=487738

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|