On 2013-07-30 00:14:28, John Johansen wrote:
> On 07/29/2013 03:40 PM, Seth Arnold wrote:
> > On Sat, Jul 27, 2013 at 02:45:16AM -0700, Tyler Hicks wrote:
> >> This patch implements the parsing of DBus rules.
> >>
>
> << snip >>
>
> >> +}
> >> +
> >> +static void copy_conditionals(struct dbus_entry *ent, struct cond_entry
> >> *conds)
> >> +{
> >> + struct cond_entry *cond_ent;
> >> +
> >> + list_for_each(conds, cond_ent) {
> >> + char **ent_member = NULL;
> >> +
> >> + /* for now disallow keyword 'in' (list) */
> >> + if (!cond_ent->eq)
> >> + yyerror("keyword \"in\" is not allowed in dbus
> >> rules\n");
> >> + if (list_len(cond_ent->vals) > 1)
> >> + yyerror("dbus conditional \"%s\" only supports a single
> >> value\n",
> >> + cond_ent->name);
> >> +
> >> + if (strcmp(cond_ent->name, "bus") == 0) {
> >> + ent_member = &ent->bus;
> >> + } else if (strcmp(cond_ent->name, "name") == 0) {
> >> + ent_member = &ent->name;
> >> + } else if (strcmp(cond_ent->name, "label") == 0) {
> >> + ent_member = &ent->peer_label;
> >> + } else if (strcmp(cond_ent->name, "path") == 0) {
> >> + ent_member = &ent->path;
> >> + } else if (strcmp(cond_ent->name, "interface") == 0) {
> >> + ent_member = &ent->interface;
> >> + } else if (strcmp(cond_ent->name, "member") == 0) {
> >> + ent_member = &ent->member;
> >> + } else {
> >> + yyerror("invalid dbus conditional \"%s\"\n",
> >> + cond_ent->name);
> >> + }
> >> +
> >> + if (ent_member) {
> >> + if (*ent_member) {
> >> + yyerror("dbus conditional \"%s\" can only be
> >> specified once\n",
> >> + cond_ent->name);
> >> + }
> >> +
> >> + (*ent_member) = cond_ent->vals->value;
> >> + }
> >> +
> >> + cond_ent->vals->value = NULL;
> >> + }
> >> +}
> >
> > This is a bit intricate :) I've read through it a few times and I'm
> > not convinced I understand it. It seems a strange place to put the 'in'
> > check, and the pointer handling seems more complicated than ideal. I'm
> > sorry though, I don't have a concrete suggestion for improvement.
> >
>
> The 'in' check is handled here because the code is using a common parsing
> pattern routine, to pick up the entries. So either this needs to be
> handled in the common parsing code by setting up/passing in context,
> or in post.
>
> This is just handling the semantic check post rule match. And the check
> is here instead of have a large code block hanging off of the yacc rule.
>
> As for pushing the semantics down into the generic pattern, this is
> difficult to do with yacc/bison. You can pass context, but we currently
> don't, and the context can only be updated once a certain match is
> achieved and reduced. So handling setting up meaningful context for
> this to be pushed into the generic pattern is probably more work than
> its worth.
>
> There will be some patches to start push profile/block context down but
> this is will be less work and a lot more useful.
Thanks for replying, John.
I unfortunately didn't make it clear before, but John wrote a good
amount of this patch (and some of the others) and then I've
updated/refined/changed them over time. This 'in' check came from his
original patch, so I'm glad he jumped in. :)
>
> As for the pointer handling sure its more complicated than you might
> like but isn't that always the case with pointers? I don't see a better
> way without standard templates to achieve the abstraction here
This goofy pointer handling was one of my changes. I wanted a way to do
a single check to see if we'd be clobbering an existing conditional
value (as with '/t { dbus bus=b1 bus=b2, }') and error out when that
happens.
Seth is right, it looks more complicated than it should. I've come up
with a better way of doing this. I'll reply with a v2 patch.
Tyler
>
>
> << snip >>
>
> >> +dbus_perm: TOK_VALUE
> >> + {
> >> + if (strcmp($1, "bind") == 0)
> >> + $$ = AA_DBUS_BIND;
> >> + else if (strcmp($1, "send") == 0 || strcmp($1, "write") == 0)
> >> + $$ = AA_DBUS_SEND;
> >> + else if (strcmp($1, "receive") == 0 || strcmp($1, "read") == 0)
> >> + $$ = AA_DBUS_RECEIVE;
> >
> > Is this code still needed? You've got explicit tokens lower down...
> >
> sadly yes, though perhaps we could abstract this more.
>
> This is the difference between a bare keyword
>
> dbus send blah...,
>
> and the keyword showing up in a permission list
>
> dbus (send receive) blah....
>
> >> + else if ($1) {
> >> + parse_dbus_mode($1, &$$, 1);
> >> + } else
> >> + $$ = 0;
> >> +
> >> + if ($1)
> >> + free($1);
> >> + }
> >> + | TOK_BIND { $$ = AA_DBUS_BIND; }
> >> + | TOK_SEND { $$ = AA_DBUS_SEND; }
> >> + | TOK_RECEIVE { $$ = AA_DBUS_RECEIVE; }
> >> + | TOK_READ { $$ = AA_DBUS_RECEIVE; }
> >> + | TOK_WRITE { $$ = AA_DBUS_SEND; }
> >> + | TOK_MODE
> >> + {
> >> + parse_dbus_mode($1, &$$, 1);
> >> + }
> >> +
> >> +dbus_perms: { /* nothing */ $$ = 0; }
> >> + | dbus_perms dbus_perm { $$ = $1 | $2; }
> >> + | dbus_perms TOK_COMMA dbus_perm { $$ = $1 | $3; }
> >> +
> >> +opt_dbus_perm: { /* nothing */ $$ = 0; }
> >> + | dbus_perm { $$ = $1; }
> >> + | TOK_OPENPAREN dbus_perms TOK_CLOSEPAREN { $$ = $2; }
> >> +
> >> +dbus_rule: TOK_DBUS opt_dbus_perm opt_conds opt_cond_list TOK_END_OF_RULE
> >> + {
> >> + struct dbus_entry *ent;
> >> +
> >> + ent = new_dbus_entry($2, $3, $4);
> >> + if (!ent) {
> >> + yyerror(_("Memory allocation error."));
> >> + }
> >> + $$ = ent;
> >> + }
> >> +
> >> hat_start: TOK_CARET {}
> >> | TOK_HAT {}
> >>
> >
> > Thanks!
> >
> >
> >
>
>
> --
> AppArmor mailing list
> [email protected]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
