Eric Blake <eblake@redhat.com> wrote on 09/27/2010 04:52:31 PM:
V2 changes: following Eric's comments: - commands in the script are assigned using cmd='...' rather
On 09/27/2010 12:40 PM, Stefan Berger wrote: 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