[apparmor] [patch 11/11] utils: add simple parsing of multi-line rules

[Christian Boltz]

Hello,

Am Donnerstag, 6. März 2014 schrieb Steve Beattie:
> On Thu, Mar 06, 2014 at 10:10:16PM +0100, Christian Boltz wrote:
> > Am Mittwoch, 5. März 2014 schrieb Steve Beattie:
> > > D-Bus rules in particular seem to get written as multi-line rules.
> > > This patch adds very simple hackish support for multiple lines.
> > > Essentially, what it does is if the parsing of a line doesn't
> > > match
> > > anything and falls all the way through, it saves the line and
> > > prepends it to the next line that occurs in the profile, but
> > > *only*
> > > if the line does not have a trailing comma to indicate the end of
> > > a
> > > rule. If the trailing comma exists, then it assumes that it's a
> > > rule
> > > that it doesn't understand and aborts.
> > 
> > Just for the records: this works for all rule types, not only for
> > dbus rules.
> 
> Correct.
> 
> > Is it intentional that RE_RULE_HAS_COMMA does not start with  ^  ?
> 
> Let's just say that RE_RULE_HAS_COMMA was a bit over-engineered. And
> yeah, it should have been anchored at the beginning of the line.
> I've updated the regex in the attached patch, as well as added a
> second to detect trailing comments (that need to filtered).
> Hopefully, the way I split them up is more readable.

Yes :-)

> > > +        # we're dealing with a multiline statement
> > > +        if lastline:
> > > +            line = '%s %s' % (lastline, line)
> > 
> > You can probably (untested ;-) also set "lastline = None" here
> > (since
> > you added it to line, it isn't needed anymore). This also makes the
> > following >10 "lastline = None" superfluous.
> 
> Yeah, adjusted patch.

:-)

> > > Index: b/utils/test/test-dbus_parse.py
> > > ==================================================================
> > > =
> > > --- a/utils/test/test-dbus_parse.py
> > > +++ b/utils/test/test-dbus_parse.py
> > 
> > Hmm, that's a strange filename for testing a regex that works for
> > all
> > rule types ;-)
> 
> Heh, okay, hint taken, split out into a separate test script, which
> exercises both new regexs. More tests can be added here for
> other/additional test cases.

;-)

> > > +    def test_without_comma_curly_brace_03(self):
> > > +        '''simple no comma check w/empty curly braces'''
> > > +        self._check('member={} ', False)
> > > +
> > > 
> > >  if __name__ == '__main__':
> > >      unittest.main()
> > 
> > I'd merge the testcases into arrays, so that you have one array for
> > "with comma" and one for "without comma", and then loop over the
> > arrays. This should keep the code more readable, even if we add 50
> > additional test rules.
> 
> I created an array, but I use each string in a 'no comma' and 'with
> comma' situation, using %s replacement with either a comma or a space.
> Kinda overkill, but works. I also used an array with generator for
> the new split comment out regex.

Sounds even better ;-)

> > Speaking about additional test rules - please also add testcases 
for:
> >     audit "/tmp/foo, # bar"
> >     
> >     audit "/tmp/foo, # bar" rw,
> >     
> >     audit "/tmp/foo, # bar" rw, # comment
> 
> Good catches, I've added cases for the above.
> 
> >     member={Hello,AddMatch,RemoveMatch,
> >     
> >     dbus (r, w,
> 
> So... the former is invalid syntax under the parser (though it
> is if you encapsulate the alternation in "s), while the latter is
> accepted (in the context of additional text to make a complete rule
> on additional lines). However, given that until this patch set, the
> tools wouldn't parse multi-line rules *at all* and the complexity of
> getting the above situations correct, at this point I'm inclined to
> say that you probably shouldn't organize your rules like so, similar
> to how we asserted you shouldn't use multi line rules if you wanted
> to use the tools until now.

Ah, you learned fast what "we'll see how many things will explode with 
those evil testcases" means ;-)

I found another evil testcase: 
parser/tst/simple_tests/vars/vars_alternation_3.sd
(and I'm slightly ;-) surprised about EXRESULT PASS - I wouldn't be 
surprised if it breaks the tools)

    #=DESCRIPTION variable w/part of an alternation included
    #=EXRESULT PASS

    @{BAR}={bar,baz,blort

    /does/not/exist {
      /does/not/@{BAR},exist,notexist} r,
    }

Please add the two lines containing @{BAR} to the testcase (if they 
fail, as a comment)

> Anyway. Updated patch attached.

Much better :-)

With the above two @{BAR} lines added to the tests,
Acked-by: Christian Boltz <apparmor at cboltz.de>


Regards,

Christian Boltz
-- 
> By getting back to the bugs you are high-jacking the thread
> and drawing away attention of what is asked.
I'm ready to try this wonderfull new car with no engine nor 
wheels...   [> houghi and jdd in opensuse-factory]