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