[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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.


"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\""

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=`'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:

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'",

virBufferVSprintf(cmd, "cmd='%s'\nres=\\`\"$cmd\"\\`",

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.


(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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]