parser: finish basic infrastructure for rule merging

Currently only file rules get merged. Finish adding basic support
for rule merging and make the default the behavior to dedup
merge rules that are exact matches.

Signed-off-by: John Johansen <john.johansen@canonical.com>

parser/parser.h

@@ -99,6 +99,8 @@ struct value_list {
 	struct value_list *next;
 };
 
+int cmp_value_list(value_list *lhs, value_list *rhs);
+
 struct cond_entry {
 	char *name;
 	int eq;			/* where equals was used in specifying list */
@@ -454,6 +456,8 @@ extern struct cod_entry *new_entry(char *id, perms_t perms, char *link_id);
 
 /* returns -1 if value != true or false, otherwise 0 == false, 1 == true */
 extern int str_to_boolean(const char* str);
+extern int null_strcmp(const char *s1, const char *s2);
+extern bool strcomp (const char *lhs, const char *rhs);
 extern struct cod_entry *copy_cod_entry(struct cod_entry *cod);
 extern void free_cod_entries(struct cod_entry *list);
 void debug_cod_entries(struct cod_entry *list);

parser/parser_merge.c

@@ -72,7 +72,7 @@ static int process_file_entries(Profile *prof)
 	table = (struct cod_entry **) malloc(sizeof(struct cod_entry *) * (count + 1));
 	if (!table) {
 		PERROR(_("Couldn't merge entries. Out of Memory\n"));
-		return ENOMEM;
+		return -ENOMEM;
 	}
 
 	for (cur = prof->entries, n = 0; cur; cur = cur->next, n++)
@@ -84,6 +84,7 @@ static int process_file_entries(Profile *prof)
 	prof->entries = table[0];
 	free(table);
 
+	count = 0;
 	/* walk the sorted table merging similar entries */
 	for (cur = prof->entries, next = cur->next; next; next = cur->next) {
 		if (file_comp(&cur, &next) != 0) {
@@ -102,12 +103,20 @@ static int process_file_entries(Profile *prof)
 
 		next->next = NULL;
 		free_cod_entries(next);
+		count++;
 	}
 
-	return 0;
+	return count;
 }
 
 int profile_merge_rules(Profile *prof)
 {
-  return process_file_entries(prof);
+	int res, tmp = process_file_entries(prof);
+	if (tmp < 0)
+		return -tmp;
+	res = prof->merge_rules();
+	if (res < 0)
+		return -res;
+	// TODO: output message eliminated rules res + tmp;
+	return 0;
 }

parser/parser_misc.c

@@ -34,6 +34,8 @@
 #include <sys/apparmor.h>
 #include <sys/apparmor_private.h>
 
+#include <algorithm>
+
 #include "capability.h"
 #include "lib.h"
 #include "parser.h"
@@ -271,6 +273,25 @@ static const char *strn_token(const char *str, size_t &len)
 	return start;
 }
 
+int null_strcmp(const char *s1, const char *s2)
+{
+	if (s1) {
+		if (s2)
+			return strcmp(s1, s2);
+		return 1;
+	} else if (s2) {
+		return -1;
+	}
+
+	// both null
+	return 0;
+}
+
+bool strcomp (const char *lhs, const char *rhs)
+{
+	return null_strcmp(lhs, rhs) < 0;
+}
+
 /*
  * Returns: -1: error
  *           0: no change - capability already in table
@@ -1065,6 +1086,50 @@ void debug_cod_entries(struct cod_entry *list)
 	}
 }
 
+// these need to move to stl
+int ordered_cmp_value_list(value_list *lhs, value_list *rhs)
+{
+	std::vector<const char *> lhstable;
+	std::vector<const char *> rhstable;
+
+	struct value_list *entry;
+	list_for_each(lhs, entry) {
+		lhstable.push_back(entry->value);
+	}
+	list_for_each(rhs, entry) {
+		rhstable.push_back(entry->value);
+	}
+
+	int res = lhstable.size() - rhstable.size();
+	if (res)
+		return res;
+
+	std::sort(lhstable.begin(), lhstable.end(), strcomp);
+	std::sort(rhstable.begin(), rhstable.end(), strcomp);
+
+	for (unsigned long i = 0; i < lhstable.size(); i++) {
+		res = null_strcmp(lhstable[i], rhstable[i]);
+		if (res)
+			return res;
+	}
+
+	return 0;
+}
+
+int cmp_value_list(value_list *lhs, value_list *rhs)
+{
+	if (lhs) {
+		if (rhs) {
+			return ordered_cmp_value_list(lhs, rhs);
+		}
+		return 1;
+	} else if (rhs) {
+		return -1;
+	}
+
+	return 0;
+}
+
 struct value_list *new_value_list(char *value)
 {
 	struct value_list *val = (struct value_list *) calloc(1, sizeof(struct value_list));

parser/parser_regex.c

@@ -848,7 +848,7 @@ int clear_and_convert_entry(std::string& buffer, char *entry)
 int post_process_policydb_ents(Profile *prof)
 {
 	for (RuleList::iterator i = prof->rule_ents.begin(); i != prof->rule_ents.end(); i++) {
-		if ((*i)->skip_processing())
+		if ((*i)->skip())
 			continue;
 		if ((*i)->gen_policy_re(*prof) == RULE_ERROR)
 			return FALSE;

parser/parser_variable.c

@@ -267,7 +267,7 @@ static int process_variables_in_entries(struct cod_entry *entry_list)
 static int process_variables_in_rules(Profile &prof)
 {
 	for (RuleList::iterator i = prof.rule_ents.begin(); i != prof.rule_ents.end(); i++) {
-		if ((*i)->skip_processing())
+		if ((*i)->skip())
 			continue;
 		int error = (*i)->expand_variables();
 		if (error)

parser/profile.cc

@@ -121,38 +121,39 @@ Profile::~Profile()
 		free(net.quiet);
 }
 
-static bool comp (rule_t *lhs, rule_t *rhs) { return (*lhs < *rhs); }
+static bool comp (rule_t *lhs, rule_t *rhs)
+{
+	return (*lhs < *rhs);
+}
 
-bool Profile::merge_rules(void)
+// TODO: move to block rule
+// returns number of rules merged
+// returns negative number on error
+int Profile::merge_rules(void)
 {
 	int count = 0;
+	std::vector<rule_t *> table;
 
-	for (RuleList::iterator i = rule_ents.begin(); i != rule_ents.end(); ) {
-		if ((*i)->is_mergeable())
-			count++;
+	for (RuleList::iterator i = rule_ents.begin(); i != rule_ents.end(); i++) {
+		if ((*i)->is_mergeable() && !(*i)->skip())
+			table.push_back(*i);
 	}
-	if (count < 2)
+	if (table.size() < 2)
 		return 0;
-
-	std::vector<rule_t *> table(count);
-	int n = 0;
-	for (RuleList::iterator i = rule_ents.begin(); i != rule_ents.end(); ) {
-		if ((*i)->is_mergeable())
-			table[n++] = *i;
-	}
-
 	std::sort(table.begin(), table.end(), comp);
-
-	for (int i = 0, j = 1; j < count; j++) {
+	unsigned long n = table.size();
+	for (unsigned long i = 0, j = 1; j < n; j++) {
+		if (table[j]->skip())
+			continue;
 		if (table[i]->cmp(*table[j]) == 0) {
-			if (!table[i]->merge(*table[j]))
-				return false;
+			if (table[i]->merge(*table[j]))
+				count++;
 			continue;
 		}
 		i = j;
 	}
 
-	return true;
+	return count;
 }
 
 
@@ -318,7 +319,7 @@ void post_process_file_entries(Profile *prof)
 void post_process_rule_entries(Profile *prof)
 {
 	for (RuleList::iterator i = prof->rule_ents.begin(); i != prof->rule_ents.end(); i++) {
-	  if ((*i)->skip_processing())
+		if ((*i)->skip())
 			continue;
 		(*i)->post_parse_profile(*prof);
   }

parser/profile.h

@@ -249,7 +249,7 @@ public:
 	 * Requires the merged rules have customized methods
 	 * cmp(), is_mergeable() and merge()
 	 */
-	virtual bool merge_rules(void);
+	virtual int merge_rules(void);
 
 	void dump(void)
 	{

parser/rule.h

@@ -38,6 +38,10 @@ class Profile;
 // RULE_TYPE_CLASS needs to be last because various class follow it
 #define RULE_TYPE_CLASS		3
 
+// rule_cast should only be used after a comparison of rule_type to ensure
+// that it is valid. Change to dynamic_cast for debugging
+//#define rule_cast dynamic_cast
+#define rule_cast static_cast
 
 typedef enum { RULE_FLAG_NONE = 0,
 	       RULE_FLAG_DELETED = 1,	// rule deleted - skip
@@ -48,21 +52,38 @@ typedef enum { RULE_FLAG_NONE = 0,
 					// added because it is implied
 } rule_flags_t;
 
+inline rule_flags_t operator|(rule_flags_t a, rule_flags_t b)
+{
+    return static_cast<rule_flags_t>(static_cast<unsigned int>(a) | static_cast<unsigned int>(b));
+}
+
+inline rule_flags_t operator&(rule_flags_t a, rule_flags_t b)
+{
+    return static_cast<rule_flags_t>(static_cast<unsigned int>(a) & static_cast<unsigned int>(b));
+}
+
+inline rule_flags_t& operator|=(rule_flags_t &a, const rule_flags_t &b)
+{
+	a = a | b;
+	return a;
+}
+
 class rule_t {
 public:
 	int rule_type;
 	rule_flags_t flags;
 
-	rule_t(int t): rule_type(t), flags(RULE_FLAG_NONE) { }
+	rule_t *removed_by;
+
+	rule_t(int t): rule_type(t), flags(RULE_FLAG_NONE), removed_by(NULL) { }
 	virtual ~rule_t() { };
 
 	bool is_type(int type) { return rule_type == type; }
 
 	// rule has been marked as should be skipped by regular processing
-	bool skip_processing()
+	bool skip()
 	{
-		return (flags == RULE_FLAG_DELETED ||
-			flags == RULE_FLAG_MERGED);
+		return (flags & RULE_FLAG_DELETED);
 	}
 	//virtual bool operator<(rule_t const &rhs)const = 0;
 	virtual std::ostream &dump(std::ostream &os) = 0;
@@ -81,15 +102,37 @@ public:
 	// to support expansion in include names and profile names
 	virtual int expand_variables(void) = 0;
 
-	// called by duplicate rule merge/elimination after final expand_vars
-	virtual bool is_mergeable(void) { return false; }
 	virtual int cmp(rule_t const &rhs) const {
-		return rule_type < rhs.rule_type;
+		return rule_type - rhs.rule_type;
 	}
 	virtual bool operator<(rule_t const &rhs) const {
 		return cmp(rhs) < 0;
 	}
-	virtual bool merge(rule_t &rhs __attribute__ ((unused))) { return false; };
+	// called by duplicate rule merge/elimination after final expand_vars
+	// to get default rule dedup
+	// child object need to provide
+	// - cmp, operator<
+	// - is_mergeable() returning true
+	// if a child object wants to provide merging of permissions,
+	// it needs to provide a custom cmp fn that doesn't include
+	// permissions and a merge routine that does more than flagging
+	// as dup as below
+	virtual bool is_mergeable(void) { return false; }
+
+	// returns true if merged
+	virtual bool merge(rule_t &rhs)
+	{
+		if (rule_type != rhs.rule_type)
+			return false;
+		if (skip() || rhs.skip())
+			return false;
+		// default merge is just dedup
+		flags |= RULE_FLAG_MERGED;
+		rhs.flags |= (RULE_FLAG_MERGED | RULE_FLAG_DELETED);
+		rhs.removed_by = this;
+
+		return true;
+	};
 
 	// called late frontend to generate data for regex backend
 	virtual int gen_policy_re(Profile &prof) = 0;
@@ -162,6 +205,32 @@ public:
 
 		return os;
 	}
+
+	int cmp(prefixes const &rhs) const {
+		if ((uint) audit < (uint) rhs.audit)
+			return -1;
+		if ((uint) audit > (uint) rhs.audit)
+			return 1;
+		if ((uint) rule_mode < (uint) rhs.rule_mode)
+			return -1;
+		if ((uint) rule_mode > (uint) rhs.rule_mode)
+			return 1;
+		if (owner < rhs.owner)
+			return -1;
+		if (owner > rhs.owner)
+			return 1;
+		return 0;
+	}
+
+	bool operator<(prefixes const &rhs) const {
+		if ((uint) audit < (uint) rhs.audit)
+			return true;
+		if ((uint) rule_mode < (uint) rhs.rule_mode)
+			return true;
+		if (owner < rhs.owner)
+			return true;
+		return false;
+	}
 };
 
 class prefix_rule_t: public rule_t, public prefixes {
@@ -221,21 +290,32 @@ public:
 		return true;
 	}
 
-	virtual bool operator<(prefixes const &rhs) const {
-		if ((uint) audit < (uint) rhs.audit)
-			return true;
-		if ((uint) rule_mode < (uint) rhs.rule_mode)
-			return true;
-		if (owner < rhs.owner)
-			return true;
-		return false;
+	int cmp(prefixes const &rhs) const {
+		return prefixes::cmp(rhs);
 	}
-	virtual bool operator<(prefix_rule_t const &rhs) const {
+
+	virtual bool operator<(prefixes const &rhs) const {
+		const prefixes *ptr = this;
+		return *ptr < rhs;
+	}
+
+	virtual int cmp(rule_t const &rhs) const {
+		int res = rule_t::cmp(rhs);
+		if (res)
+			return res;
+		prefix_rule_t const &pr = rule_cast<prefix_rule_t const &>(rhs);
+		const prefixes *lhsptr = this, *rhsptr = &pr;
+		return lhsptr->cmp(*rhsptr);
+	}
+
+	virtual bool operator<(rule_t const &rhs) const {
 		if (rule_type < rhs.rule_type)
 			return true;
 		if (rhs.rule_type < rule_type)
 			return false;
-		return *this < (prefixes const &)rhs;
+		prefix_rule_t const &pr = rule_cast<prefix_rule_t const &>(rhs);
+		const prefixes *rhsptr = &pr;
+		return *this < *rhsptr;
 	}
 
 	virtual ostream &dump(ostream &os) {
@@ -253,6 +333,16 @@ public:
 
 	int aa_class(void) { return rule_type - RULE_TYPE_CLASS; }
 
+	/* inherit cmp */
+
+	/* we do not inherit operator< from so class_rules children
+	 * can in herit the generic one that redirects to cmp()
+	 * that does get overriden
+	 */
+	virtual bool operator<(rule_t const &rhs) const {
+		return cmp(rhs) < 0;
+	}
+
 	virtual ostream &dump(ostream &os) {
 		prefix_rule_t::dump(os);
 
@@ -267,6 +357,13 @@ class perms_rule_t: public class_rule_t {
 public:
 	perms_rule_t(int c): class_rule_t(c), perms(0) { };
 
+	virtual int cmp(rule_t const &rhs) const {
+		int res = class_rule_t::cmp(rhs);
+		if (res)
+			return res;
+		return perms - (rule_cast<perms_rule_t const &>(rhs)).perms;
+	}
+
 	/* defaut perms, override/mask off if none default used */
 	virtual ostream &dump(ostream &os) {