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

[Eric Blake]

On 09/27/2010 12:40 PM, Stefan Berger wrote:
> V2 changes:
>   following Eric's comments:
>    - commands in the script are assigned using cmd='...' rather than cmd="..."
>    - invoke commands using eval res=... rather than res=eval ...
>    - rewrote function escaping ` and single quotes (which became more tricky)
>
> In this patch I am extending the rule instantiator to create the comment
> node where supported, which is the case for iptables and ip6tables.
>
> Since commands are written in the format
>
> cmd='iptables ...-m comment --comment \"\" '
>
> certain characters ('`) in the comment need to be escaped to
> prevent comments from becoming commands themselves or cause other
> forms of (bash) substitutions. I have tested this with various input and in
> my tests the input made it straight into the comment. A test case for TCK
> will be provided separately that tests this.

At first, I failed to see how ` needs escaping inside '', unless you 
aren't uniformly using '' like you think you are.  Then it hit me - yuck 
- we aren't uniformly using '' - we really do have to escape ` inside 
``, or come up with an alternate solution.  That is:

ret=`iptables -m comment --comment '`'`

is indeed ill-formed.  But if you are going to escape `, then you also 
have to escape \ and $ for the same duration.  Yuck again.

But fear not - I have a slicker solution:

comment='comment with lone '\'', ", `, \, $x, and two  spaces'
cmd='iptables ...-m comment --comment "$comment"'
eval ret=\`"$cmd"\`

which at expansion time results in:

eval ret=\`"$cmd"\`
ret=`iptables ...-m comment --comment "$comment"`
iptables ...-m comment --comment \
  'comment with lone '\'', `, ", `, \, $x, and two  spaces'

That is, rather than trying to pass the comment literally through $cmd 
(and thus having to carefully escape $cmd for its eventual use inside 
``), it might be nicer to stick the comment in an intermediate variable 
(where you only need to escape ') and make $cmd reference the 
intermediate variable (where you won't have any problematic uses of ", 
`, or ' to contend with, and where your only $ is one where you 
intentionally want variable expansion).

> @@ -52,10 +53,10 @@
>
>
>  #define CMD_SEPARATOR "\n"
> -#define CMD_DEF_PRE  "cmd=\""
> -#define CMD_DEF_POST "\""
> +#define CMD_DEF_PRE  "cmd='"
> +#define CMD_DEF_POST "'"
>  #define CMD_DEF(X) CMD_DEF_PRE X CMD_DEF_POST
> -#define CMD_EXEC   "res=`${cmd}`" CMD_SEPARATOR
> +#define CMD_EXEC   "eval res=\\`\"${cmd}\"\\`" CMD_SEPARATOR

This part seems okay - the ` is quoted to protect it from evaluation 
until after eval has had a chance to collect its arguments.

> +static char *
> +escapeComment(const char *buf)
> +{
> +    char *res;
> +    size_t i, j, add = 0, len = strlen(buf);
> +
> +    static const char SINGLEQUOTE_REPLACEMENT[12] = "'\\'\\\"\\'\\\"\\''";

That seems rather long to me.  Broken into chunks with C-string escaping 
eliminated:

'    \'\"\'\"\'    '
end the current single quoting
      the 5 literal shell chars '"'"'
                    resume single quoting

I'm not following why we need 5 literal shell characters, instead of 1.

> +
> +    if (len > IPTABLES_MAX_COMMENT_SIZE)
> +        len = IPTABLES_MAX_COMMENT_SIZE;
> +
> +    for (i = 0; i < len; i++) {
> +        if (buf[i] == '`')
> +            add++;
> +        else if (buf[i] == '\'')
> +            add += sizeof(SINGLEQUOTE_REPLACEMENT);
> +    }

Back to my observation that this doesn't help for \ or $, the other two 
characters that need escaping inside ``.  It would be so much easier if 
we could rely on $() instead of ``.  Wait a minute - this is only done 
for Linux hosts, where we are guaranteed a sane shell (it's pretty much 
just Solaris where /bin/sh is too puny to do $()) - using $() instead of 
`` would solve a lot of escaping issues.

> @@ -993,6 +1034,16 @@ iptablesHandleIpHdr(virBufferPtr buf,
>          }
>      }
>
> +    if (HAS_ENTRY_ITEM(&ipHdr->dataComment)) {
> +        char *cmt = escapeComment(ipHdr->dataComment.u.string);
> +        if (!cmt)
> +            goto err_exit;
> +        virBufferVSprintf(buf,
> +                          " -m comment --comment '\\''%s'\\''",
> +                          cmt);
> +        VIR_FREE(cmt);

OK, maybe I see why your comment had such a long replacement for ', 
because you aren't adding any escaping to cmt here.  But I still think 
we can come up with a more elegant solution, by using the intermediate 
variable.  Thanks for forcing me to explain myself - it's an interesting 
process thinking about this.


[Do I even dare mention that at an even higher layer, it might be nicer 
to avoid /bin/sh in the first place, and instead put effort into my 
pending virCommand API patches to make it easier to directly invoke all 
the iptables commands from C?]

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