7:32 p.m.
Eric Blake <eblake(a)redhat.com> wrote on 09/27/2010 04:52:31 PM:
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.
I had tested these also and it doesn't seem to be the case. I looked like
really only ` and ' needed special treatment.
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"\`
I changed this now for v3.
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).
It's nicer, true, just a little more changes need for building the final
command where
a 'prefix', here the comment='' is stuck in front of the line with the
iptables command.
> @@ -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.
That's because I needed to place the ' in between "". Without it
would not
work. The first ' was to close the string in front and the final ' was to
start another string.
> +
> + 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?]
I do also generate some scripts with 'if .. then' statements that won't be
able to executed via your virCommand API...
Stefan