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

[Stefan Berger]

Eric Blake <eblake at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100927/42d910e0/attachment-0001.htm>