[libvirt] [PATCH 2/4] libvirt-guest.init: quoting variables

[Philipp Hahn]

Hello Eric,

On Friday Mar 11th 2011 21:48:03 Eric Blake wrote:
> On 03/09/2011 01:54 AM, Philipp Hahn wrote:
> > At least protect the $uri variable against further expansion by properly
> > quoting it. While doing that, also quote all other variables to protect
> > against shell meta characters.
>
> Meanwhile, $URIS is indeed a user-settable variable, via
> /etc/sysconfig/libvirt-guests,

I would call it root-configurable instead of user-settable, because that file 
is normally owned by root and has mod 0644. A sane root hopefully does not 
put a 'rm -rf /' in there ;-)
It would be something else if that file was sourced by a program running with 
more privileges than the user, who is allowed to edit that file.
But yes, even root should expect decent bahavior and should not be surprised 
by the shell expanding string containing wildcards.

> Meanwhile, I'm not a fan of blindly quoting everything; there are
> documented cases where you can trigger shell bugs by doing too much
> quoting.  For example:
>
> foo=`some_command "with quotes"`
>
> is portable, but
>
> foo="`some_command "with quotes"`"
>
> is not.  So I prefer to quote variables that come from external source,
> but not internal variables that are obviously safe (that said, quoting
> generally doesn't hurt, so even if something is overkill does not meant
> that I am rejecting the patch).

I've seen to many scripts not doing any quoting at all or all wrong, which 
fail in interesting to horrible ways when given arguments containing shell 
meta characters. I personally consider them a much greater risk than some 
ancient shell not being able to parse properly quoted strings.

> > @@ -89,7 +87,7 @@ list_guests() {
> >
> >      uuids=
> >      for id in $(echo "$list" | awk 'NR > 2 {print $1}'); do
> > -        uuid=$(run_virsh_c $uri dominfo $id | awk '/^UUID:/{print $2}')
> > +        uuid=$(run_virsh_c "$uri" dominfo "$id" | awk '/^UUID:/{print
> > $2}')
>
> Quoting "$id" is overkill; we know it's numeric.  But it probably
> doesn't hurt.

My rule of thumb is this: if the command dominfo is expecting one argument, I 
make sure it gets exactly on argument by properly quoting it. I don't care 
that I know that it will normally not contain blanks or wildcard, so even 
when someone other later changes the variable in some stupid way, it will 
still pass one required argument to the function. I prefer virsh to report an 
error like "'foo bar' is invalid" instead of "'foo' not found" or 'unexpected 
additional parameter "bar"'.

> This one's overkill - we explicitly set $configured to either 'true' or
> 'false'; no external data possible.

I'm waiting for the day when someone dictates, that the UNIXroot-directory is 
no longer called '/' but '/ /'. That will give some lovely firefork ;-)

> Overkill, since $guest_running is under our control.
> > +        "$1"
>
> Overkill, since we know that $1 is one of three safe values.
>
> I think I'll apply your patch as-is, in spite of the overkill, then
> worry about how to safely split $URIS as a separate patch.

Only #1 was the really important fix, the others were done because I was 
already looking for whitespace problems. To thanks for applying this one two.

Sincerely
Philipp
-- 
Philipp Hahn           Open Source Software Engineer      hahn at univention.de
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  D-28359 Bremen                 fax: +49 421 22 232-99
                                                   http://www.univention.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110314/329f0ccf/attachment-0001.sig>