Hello, Am Donnerstag, 22. September 2016, 10:38:36 CEST schrieb Steve Beattie: > On Fri, Aug 12, 2016 at 10:54:09PM +0200, Christian Boltz wrote: > > aa-logprof needs to check if an exec rule for a given path exists. > > > > This patch adds a __FileAnyExec class to FileRule, as well as > > ANY_EXEC (which should be used externally, similar to ALL), and > > adjusts several checks to allow it as a special execute mode. > > > > This will allow to use is_covered() (or aa.py is_known_rule()) to > > find out if execute is permitted, which replaces aa.py > > profile_known_exec() in one of the following patches. > > > > As usual, also add some tests. > > > > [ 13-FileRule-add-ANY_EXEC.diff ] > > > > === modified file ./utils/apparmor/rule/file.py > > --- utils/apparmor/rule/file.py 2016-02-21 15:43:58.009985520 +0100 > > +++ utils/apparmor/rule/file.py 2016-02-21 16:05:39.673508607 +0100 > > @@ -235,12 +242,14 @@ > > > > return False > > > > # TODO: handle fallback modes? > > > > - if other_rule.exec_perms and self.exec_perms != > > other_rule.exec_perms: > > + if other_rule.exec_perms == self.ANY_EXEC and > > self.exec_perms: > > + pass # avoid hitting the 'elif' branch > > > > + elif other_rule.exec_perms and self.exec_perms != > > other_rule.exec_perms: > > return False > > Could you give a more explanatory comment than merely wanting to skip > the elif: test? Or restructure the conditionals that make it clear in > what situations we're returning False here for?
Basically the if condition is better than any comment. The "pass" gets
hit if other_rule.exec_perms is ANY_EXEC and self.exec_perms is set
(which means != None, so Px, Cx, ix, Ux etc. are valid).
Restructuring the code (doing the check from "elif" first doesn't look
like a good idea because it would result in a more complicated condition
(self.exec_perms != None + a check for ANY_EXEC). (Yes, I thought about
that because I'm not the biggest fan of "pass" - but in this case, it
really gives us easier to understand code.)
I agree that the comment could be better.
What about this:
pass # other_rule has ANY_EXEC and self has an exec rule set -> covered, so
avoid hitting the 'elif' branch
If nobody objects, I'll adjust the patch.
> That said, that's not enough of a criticism to block my
> Acked-by: Steve Beattie <[email protected]>. Thanks.
Thanks for reviewing the patches!
Regards,
Christian Boltz
--
> what do I need to avoid?
* Belgian "Beer". At any cost.
[> Richard Brown and Henne Vogelsang in opensuse-project]
signature.asc
Description: This is a digitally signed message part.
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
