[libvirt] [patch 2/5] Instantiate comments in ip(6)tables rules

Eric Blake eblake at redhat.com
Fri Sep 24 22:16:35 UTC 2010


On 09/24/2010 02:22 PM, Stefan Berger wrote:
> I just tried the TCK test without and with double-escaping in libvirtd
> and double-escaping does seem to be necessary otherwise `ls` and $(ls)
> do get executed and their results end up in the comment. The spaces
> are preserved, though, so I can revert the change to IFS.

Hmm.

> "res=`eval \"$cmd\"" CMD_SEPARATOR

> +        virBufferVSprintf(buf,
> +                          " -m comment --comment \\\"%s\\\"",
> +                          cmt);

Thinking about it more:

Let's see why double-escaping helped you, by setting up a user comment 
with two spaces and a $.  You ultimately want to call:

iptables -m comment --comment 'user  $comment'

but you used a "" style, resulting in:

iptables -m comment --comment "user  \$comment"

Then, you are capturing that command in a shell variable $cmd (to avoid 
repetition when displaying a failed command) and the output in $res.  So 
you need one level of escaping for assigning to cmd within "" context:

cmd="iptables -m comment --comment \"user  \\\$comment\""
res=`$cmd`

Oops, that failed, because it splits $cmd at IFS boundaries, which 
breaks up the user comment because it looks roughly like

res=`'iptables' '-m' 'comment' '--comment' '"user' '\$comment"'`

But quoting $cmd isn't right either, because then you try to execute a 
single command with no arguments:

res=`"$cmd"`
res=`'iptables -m comment --comment "user  \$comment"'`

So you _do_ need eval to control where the word boundaries are, which 
means you will be removing a level of quoting from the contents of $cmd, 
and you also need quoting when assigning to cmd, so I now see why double 
escaping is needed.  Continuing on with your original example, your 
underquoted version with IFS cleared to avoid field splitting of $cmd 
looks like:

IFS=
cmd="iptables -m comment --comment \"user  \\\$comment\""
res=`eval $cmd`
res=`eval 'iptables -m comment --comment "user  \$comment"'`
res=`iptables -m comment --comment "user  $comment"`

oops - here, `` turned \$ into $ before the eval got run, which means 
eval will expand the (empty variable) $comment.  Even with two levels of 
quoting, you risk problems when mixing "" and ``.



My suggestion is to assign cmd using '' rather than "" (fewer things to 
quote), as well as moving the eval outside of the `` (so it becomes 
obvious which \ are interpreted by eval rather than by ``:

cmd='iptables -m comment --comment '\''user  $comment'\''
eval res=\`"$cmd"\`
res=`iptables -m comment --comment 'user  $comment'`

And the nice part of that is the implementation:

virBufferVSprintf(buf, " -m comment --comment '%s'",
   escapeSingleQuotes(user_comment));

virBufferVSprintf(cmd, "cmd='%s'\nres=\\`\"$cmd\"\\`",
   escapeSingleQuotes(buf));

still results in the needed double quoting (one for assignment to cmd, 
and one to be stripped by eval), but via two passes of a single function 
that only has to escape all instances of ', rather than a function that 
has to add escaping for four different bytes and pay attention to how 
many escape levels must be added.

>> Ouch.  That's probably 4x slower than the glibc version.  I'd much
>> rather see:
>>
>> #undef strchr
>>
>
> Yes, that does the trick. Thanks.
>
>> or
>>
>> (strchr)(a, b)

On further thought, gnulib might be doing:

#define strchr rpl_strchr

on platforms where strchr is broken, so using #undef strchr is too 
risky.  So I'd recommend sticking with (strchr)(a, b), which still works 
if gnulib has to replace a broken strchr.

(It's surprising how easy it is to have bugs in such a low-level 
function - until just this month, glibc 2.12 on Alpha hardware had a bug 
in memchr that gnulib has to work around).

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org




More information about the libvir-list mailing list