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