[apparmor] [patch 01/26] Convert mount and dbus to be subclasses of a generic rule class

Steve Beattie steve at nxnw.org
Mon Mar 31 22:59:02 UTC 2014


On Thu, Mar 27, 2014 at 08:45:14AM -0700, john.johansen at canonical.com wrote:
> This will simplify add new features as most of the code can reside in
> its own class. There are still things to improve but its a start.
> 
> Signed-off-by: John Johansen <john.johansen at canonical.com>

Alright, I think I've got enough of a handle on this patch (although
I'll admit I glazed over a bit on the mount processing move to
the mount_rule class) to go ahead and say
Acked-by: Steve Beattie <steve at nxnw.org>, with some nits to follow:

>  parser/Makefile          |   13 -
>  parser/dbus.c            |  405 +++++++++++++++++++++++++---------
>  parser/dbus.h            |   32 ++
>  parser/mount.c           |  551 +++++++++++++++++++++++++++++++++++++----------
>  parser/mount.h           |   32 +-
>  parser/parser.h          |   22 +
>  parser/parser_misc.c     |  101 --------
>  parser/parser_policy.c   |   29 --
>  parser/parser_regex.c    |  489 -----------------------------------------
>  parser/parser_variable.c |   66 +----
>  parser/parser_yacc.y     |   51 ++--
>  parser/profile.cc        |    6 
>  parser/profile.h         |    7 
>  parser/rule.c            |   23 +
>  parser/rule.h            |   51 ++++
>  15 files changed, 940 insertions(+), 938 deletions(-)

A stylistic nit: presumably, dbus.c, mount.c and rule.c should get
renamed to .cc?

> --- 2.9-test.orig/parser/dbus.c
> +++ 2.9-test/parser/dbus.c
> @@ -20,23 +20,91 @@
>  #include <string.h>
>  #include <sys/apparmor.h>
>  
> +#include <iomanip>
> +#include <string>
> +#include <iostream>
> +#include <sstream>
> +
>  #include "parser.h"
>  #include "profile.h"
>  #include "parser_yacc.h"
>  #include "dbus.h"
>  
> -void free_dbus_entry(struct dbus_entry *ent)
> +#define _(s) gettext(s)

Nit: we really ought to move this define to a common header.

> --- 2.9-test.orig/parser/parser_variable.c
> +++ 2.9-test/parser/parser_variable.c
> @@ -15,6 +15,7 @@
>   *   along with this program; if not, contact Novell, Inc.
>   */
>  
> +#include <assert.h>
>  #include <ctype.h>
>  #include <stdlib.h>
>  #include <stdio.h>
> @@ -213,13 +214,15 @@
>  }
>  
>  /* doesn't handle variables in options atm */
> -static int expand_entry_variables(char **name, void *entry)
> +int expand_entry_variables(char **name)
>  {
>  	struct set_value *valuelist;
>  	struct var_string *split_var;
>  	int ret;
>  
> -	if (!entry) 		/* can happen when entry is optional */
> +	assert(name);
> +
> +	if (!*name) 		/* can happen when entry is optional */
>  		return 0;
>  
>  	while ((split_var = split_out_var(*name))) {
> @@ -251,7 +254,7 @@
>  	struct cod_entry *entry;
>  
>  	list_for_each(entry_list, entry) {
> -		error = expand_entry_variables(&entry->name, entry);
> +		error = expand_entry_variables(&entry->name);
>  		if (error)
>  			return error;
>  	}
> @@ -259,69 +262,24 @@
>  	return 0;
>  }
>  
> -/* does not currently support expansion of vars in options */
> -static int process_variables_in_mnt_entries(struct mnt_entry *entry_list)
> +static int process_variables_in_rules(Profile &prof)
>  {
> -	int error = 0;
> -	struct mnt_entry *entry;
> -
> -	list_for_each(entry_list, entry) {
> -		error = expand_entry_variables(&entry->mnt_point, entry);
> +	for(RuleList::iterator i = prof.rule_ents.begin(); i != prof.rule_ents.end(); i++) {

Another nit, can you put a space between the for and the open paren?

> +	  int error = (*i)->expand_variables();
>  		if (error)
>  			return error;
> -		error = expand_entry_variables(&entry->device, entry);
> -		if (error)
> -			return error;
> -		error = expand_entry_variables(&entry->trans, entry);
> -		if (error)
> -			return error;
> -
>  	}
>  
>  	return 0;
>  }
>  
> --- 2.9-test.orig/parser/parser_yacc.y
> +++ 2.9-test/parser/parser_yacc.y
> @@ -72,11 +72,11 @@
>  
>  struct cod_entry *do_file_rule(char *ns, char *id, int mode,
>  			       char *link_id, char *nt);
> -struct mnt_entry *do_mnt_rule(struct cond_entry *src_conds, char *src,
> -			      struct cond_entry *dst_conds, char *dst,
> -			      int mode);
> -struct mnt_entry *do_pivot_rule(struct cond_entry *old, char *root,
> -				char *transition);
> +mnt_rule *do_mnt_rule(struct cond_entry *src_conds, char *src,
> +		      struct cond_entry *dst_conds, char *dst,
> +		      int mode);
> +mnt_rule *do_pivot_rule(struct cond_entry *old, char *root,
> +			char *transition);
>  
>  void add_local_entry(Profile *prof);
>  
> @@ -162,6 +162,12 @@
>  /* debug flag values */
>  %token TOK_FLAGS
>  
> +%code requires {
> +	#include "profile.h"
> +	#include "mount.h"
> +	#include "dbus.h"
> +}
> +

Interesting, I'd not seen the %code stuff before.

>  %union {
>  	char *id;
>  	char *flag_id;

> --- /dev/null
> +++ 2.9-test/parser/rule.h
> @@ -0,0 +1,51 @@
> +/*
> + *   Copyright (c) 2014
> + *   Canonical Ltd. (All rights reserved)
> + *
> + *   This program is free software; you can redistribute it and/or
> + *   modify it under the terms of version 2 of the GNU General Public
> + *   License published by the Free Software Foundation.
> + *
> + *   This program is distributed in the hope that it will be useful,
> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *   GNU General Public License for more details.
> + *
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, contact Novell, Inc. or Canonical
> + *   Ltd.
> + */
> +#ifndef __AA_RULE_H
> +#define __AA_RULE_H
> +
> +#include <list>
> +#include <ostream>
> +
> +#include "policydb.h"
> +
> +class Profile;
> +
> +#define RULE_NOT_SUPPORTED 0
> +#define RULE_ERROR -1
> +#define RULE_OK 1
> +
> +class rule_t {
> +public:
> +	int aa_class;
> +
> +	rule_t(void): aa_class(AA_CLASS_UNKNOWN) { };

Another nit: aa_class is defined for the rule_t abstract class as well
as for the mnt_rule class, but not for the dbus_rule class, nor is it
used anywhere. So I'm guessing it's something that you initially thought
you needed, but turned out not to. Can you kill it if that's the case?
(It can be a separate patch.)

> +	virtual ~rule_t() { };
> +
> +	//virtual bool operator<(rule_t const &rhs)const = 0;
> +	virtual std::ostream &dump(std::ostream &os) = 0;
> +	virtual int expand_variables(void) = 0;
> +	virtual int gen_policy_re(Profile &prof) = 0;
> +	virtual void post_process(Profile &prof) = 0;
> +};
> +
> +std::ostream &operator<<(std::ostream &os, rule_t &rule);
> +
> +typedef std::list<rule_t *> RuleList;
> +
> +#endif /* __AA_RULE_H */
> +

Thanks. I really like the direction of this patch.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20140331/6ce982c2/attachment.pgp>


More information about the AppArmor mailing list