parser: add include dedup cache to handle include loops

Profile includes can be setup to loop and expand in a pathalogical
manner that causes build failures. Fix this by caching which includes
have already been seen in a given profile context.

In addition this can speed up some profile compiles, that end up
re-including common abstractions. By not only deduping the files
being included but skipping the need to reprocess and dedup the
rules within the include.

Fixes: https://bugzilla.suse.com/show_bug.cgi?id=1184779
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/743
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve.beattie@canonical.com>

parser/Makefile

@@ -102,7 +102,7 @@ SRCS = parser_common.c parser_include.c parser_interface.c parser_lex.c \
        af_rule.cc af_unix.cc policy_cache.c default_features.c
 HDRS = parser.h parser_include.h immunix.h mount.h dbus.h lib.h profile.h \
        rule.h common_optarg.h signal.h ptrace.h network.h af_rule.h af_unix.h \
-       policy_cache.h
+       policy_cache.h file_cache.h
 TOOLS = apparmor_parser
 
 OBJECTS = $(patsubst %.cc, %.o, $(SRCS:.c=.o))
@@ -215,10 +215,10 @@ apparmor_parser: $(OBJECTS) $(AAREOBJECTS) $(LIBAPPARMOR_A)
 	$(CXX) $(LDFLAGS) $(EXTRA_CFLAGS) -o $@ $(OBJECTS) $(LIBS) \
 	      ${LEXLIB}  $(AAREOBJECTS) $(AARE_LDFLAGS) $(AALIB)
 
-parser_yacc.c parser_yacc.h: parser_yacc.y parser.h profile.h
+parser_yacc.c parser_yacc.h: parser_yacc.y parser.h profile.h file_cache.h
 	$(YACC) $(YFLAGS) -o parser_yacc.c parser_yacc.y
 
-parser_lex.c: parser_lex.l parser_yacc.h parser.h profile.h mount.h dbus.h policy_cache.h
+parser_lex.c: parser_lex.l parser_yacc.h parser.h profile.h mount.h dbus.h policy_cache.h file_cache.h
 	$(LEX) ${LEXFLAGS} -o$@ $<
 
 parser_lex.o: parser_lex.c parser.h parser_yacc.h
@@ -230,13 +230,13 @@ parser_misc.o: parser_misc.c parser.h parser_yacc.h profile.h cap_names.h $(APPA
 parser_yacc.o: parser_yacc.c parser_yacc.h $(APPARMOR_H)
 	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
 
-parser_main.o: parser_main.c parser.h parser_version.h policy_cache.h libapparmor_re/apparmor_re.h $(APPARMOR_H)
+parser_main.o: parser_main.c parser.h parser_version.h policy_cache.h file_cache.h libapparmor_re/apparmor_re.h $(APPARMOR_H)
 	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
 
 parser_interface.o: parser_interface.c parser.h profile.h libapparmor_re/apparmor_re.h
 	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
 
-parser_include.o: parser_include.c parser.h parser_include.h
+parser_include.o: parser_include.c parser.h parser_include.h file_cache.h
 	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
 
 parser_merge.o: parser_merge.c parser.h profile.h
@@ -257,7 +257,7 @@ parser_policy.o: parser_policy.c parser.h parser_yacc.h profile.h
 parser_alias.o: parser_alias.c parser.h profile.h
 	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
 
-parser_common.o: parser_common.c parser.h
+parser_common.o: parser_common.c parser.h file_cache.h
 	$(CXX) $(EXTRA_CFLAGS) -c -o $@ $<
 
 mount.o: mount.cc mount.h parser.h immunix.h rule.h

parser/file_cache.h

@@ -0,0 +1,52 @@
+/*
+ *   Copyright (c) 2021
+ *   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, Canonical Ltd.
+ */
+
+#ifndef __AA_FILE_CACHE_H
+#define __AA_FILE_CACHE_H
+
+#include <set>
+#include <string>
+
+using namespace std;
+
+/* TODO: have includecache be a frontend for file cache, don't just
+ * store name.
+ */
+class IncludeCache_t {
+public:
+	set<string> cache;
+
+	IncludeCache_t() = default;
+	virtual ~IncludeCache_t() = default;
+
+	/* return true if in set */
+	bool find(const char *name) {
+		return cache.find(name) != cache.end();
+	}
+
+	bool insert(const char *name) {
+		pair<set<string>::iterator,bool> res = cache.insert(name);
+		if (res.second == false) {
+			return false;
+		}
+		/* inserted */
+
+		return true;
+	}
+};
+
+#endif /* __AA_FILE_CACHE_H */

parser/parser.h

@@ -32,6 +32,7 @@
 
 #include <sys/apparmor.h>
 
+#include "file_cache.h"
 #include "immunix.h"
 #include "libapparmor_re/apparmor_re.h"
 #include "libapparmor_re/aare_rules.h"
@@ -353,6 +354,8 @@ extern char *profile_ns;
 extern char *current_filename;
 extern FILE *ofile;
 extern int read_implies_exec;
+extern IncludeCache_t *g_includecache;
+
 extern void pwarnf(bool werr, const char *fmt, ...) __attribute__((__format__(__printf__, 2, 3)));
 extern void common_warn_once(const char *name, const char *msg, const char **warned_name);
 

parser/parser_common.c

@@ -20,6 +20,7 @@
 #include <stdarg.h>
 
 #include "parser.h"
+#include "file_cache.h"
 
 /* Policy versioning is determined by a combination of 3 values:
  * policy_version:     version of txt policy
@@ -95,6 +96,8 @@ char *current_filename = NULL;
 
 FILE *ofile = NULL;
 
+IncludeCache_t *g_includecache;
+
 #ifdef FORCE_READ_IMPLIES_EXEC
 int read_implies_exec = 1;
 #else

parser/parser_include.c

@@ -151,7 +151,7 @@ void parse_default_paths(void)
 	add_search_dir(basedir);
 }
 
-FILE *search_path(char *filename, char **fullpath)
+FILE *search_path(char *filename, char **fullpath, bool *skip)
 {
 	FILE *newf = NULL;
 	char *buf = NULL;
@@ -161,15 +161,27 @@ FILE *search_path(char *filename, char **fullpath)
 			perror("asprintf");
 			exit(1);
 		}
+
+		if (g_includecache->find(buf)) {
+			/* hit do not want to re-include */
+			*skip = true;
+			return NULL;
+		}
+
 		newf = fopen(buf, "r");
-		if (newf && fullpath)
+		if (newf) {
-			*fullpath = buf;
+			/* ignore failing to insert into cache */
-		else
+			(void) g_includecache->insert(buf);
-			free(buf);
+			if (fullpath)
-		buf = NULL;
+				*fullpath = buf;
-		if (newf)
+			else
+				free(buf);
 			break;
+		}
+		free(buf);
+		buf = NULL;
 	}
+	*skip = false;
 	return newf;
 }
 

parser/parser_include.h

@@ -27,7 +27,7 @@ extern void init_base_dir(void);
 extern void set_base_dir(char *dir);
 extern void parse_default_paths(void);
 extern int do_include_preprocessing(char *profilename);
-FILE *search_path(char *filename, char **fullpath);
+FILE *search_path(char *filename, char **fullpath, bool *skip);
 
 extern void push_include_stack(char *filename);
 extern void pop_include_stack(void);

parser/parser_lex.l

@@ -44,6 +44,7 @@
 #include "parser_yacc.h"
 #include "lib.h"
 #include "policy_cache.h"
+#include "file_cache.h"
 
 #ifdef PDEBUG
 #undef PDEBUG
@@ -134,10 +135,17 @@ static int include_dir_cb(int dirfd unused, const char *name, struct stat *st,
 	if (is_blacklisted(name, path))
 		return 0;
 
+	if (g_includecache->find(path)) {
+		PDEBUG("skipping reinclude of \'%s\' in \'%s\'\n", path,
+			d->filename);
+		return 0;
+	}
+
 	if (S_ISREG(st->st_mode)) {
 		if (!(yyin = fopen(path,"r")))
 			yyerror(_("Could not open '%s' in '%s'"), path, d->filename);
 		PDEBUG("Opened include \"%s\" in \"%s\"\n", path, d->filename);
+		(void) g_includecache->insert(path);
 		update_mru_tstamp(yyin, path);
 		push_include_stack(path);
 		yypush_buffer_state(yy_create_buffer(yyin, YY_BUF_SIZE));
@@ -151,16 +159,29 @@ void include_filename(char *filename, int search, bool if_exists)
 	FILE *include_file = NULL;
 	struct stat my_stat;
 	autofree char *fullpath = NULL;
+	bool cached;
 
 	if (search) {
-		if (preprocess_only)
+		include_file = search_path(filename, &fullpath, &cached);
+		if (!include_file && cached) {
+			goto skip;
+		} else if (preprocess_only) {
 			fprintf(yyout, "\n\n##included <%s>\n", filename);
-		include_file = search_path(filename, &fullpath);
+		} else if (!include_file && preprocess_only) {
+			fprintf(yyout, "\n\n##failed include <%s>\n", filename);
+		}
+
+	} else if (g_includecache->find(filename)) {
+		/* duplicate entry skip */
+		goto skip;
 	} else {
 		if (preprocess_only)
 			fprintf(yyout, "\n\n##included \"%s\"\n", filename);
 		fullpath = strdup(filename);
 		include_file = fopen(fullpath, "r");
+		if (include_file)
+			/* ignore failure to insert into cache */
+			(void) g_includecache->insert(filename);
 	}
 
 	if (!include_file) {
@@ -188,6 +209,13 @@ void include_filename(char *filename, int search, bool if_exists)
 				  " '%s' in '%s'"), fullpath, filename);;
 		}
 	}
+
+	return;
+
+skip:
+	if (preprocess_only)
+		fprintf(yyout, "\n\n##skipped duplicate include <%s>\n", filename);
+	return;
 }
 
 static char *lsntrim(char *s, int l)

parser/parser_main.c

@@ -50,6 +50,7 @@
 #include "common_optarg.h"
 #include "policy_cache.h"
 #include "libapparmor_re/apparmor_re.h"
+#include "file_cache.h"
 
 #define OLD_MODULE_NAME "subdomain"
 #define PROC_MODULES "/proc/modules"
@@ -1035,6 +1036,8 @@ void reset_parser(const char *filename)
 	aa_features_unref(policy_features);
 	policy_features = NULL;
 	clear_cap_flag(CAPFLAG_POLICY_FEATURE);
+	delete g_includecache;
+	g_includecache = new IncludeCache_t();
 }
 
 int test_for_dir_mode(const char *basename, const char *linkdir)

parser/parser_yacc.y

@@ -220,6 +220,7 @@ void add_local_entry(Profile *prof);
 	struct cond_entry_list cond_entry_list;
 	int boolean;
 	struct prefixes prefix;
+	IncludeCache_t *includecache;
 }
 
 %type <id> 	TOK_ID
@@ -334,9 +335,17 @@ opt_id: { /* nothing */ $$ = NULL; }
 opt_id_or_var: { /* nothing */ $$ = NULL; }
 	| id_or_var { $$ = $1; }
 
-profile_base: TOK_ID opt_id_or_var opt_cond_list flags TOK_OPEN rules TOK_CLOSE
+profile_base: TOK_ID opt_id_or_var opt_cond_list flags TOK_OPEN
 	{
-		Profile *prof = $6;
+		/* mid rule action
+		 * save current cache, restore at end of block
+		 */
+		$<includecache>$ = g_includecache;
+		g_includecache = new IncludeCache_t();
+	}
+    rules TOK_CLOSE
+	{
+		Profile *prof = $7;
 		bool self_stack = false;
 
 		if (!prof) {
@@ -387,6 +396,10 @@ profile_base: TOK_ID opt_id_or_var opt_cond_list flags TOK_OPEN rules TOK_CLOSE
 		post_process_file_entries(prof);
 		post_process_rule_entries(prof);
 		prof->flags.debug(cerr);
+
+		/* restore previous blocks include cache */
+		delete g_includecache;
+		g_includecache = $<includecache>6;
 		$$ = prof;
 
 	};
@@ -1775,12 +1788,17 @@ static int abi_features_base(struct aa_features **features, char *filename, bool
 	autofclose FILE *f = NULL;
 	struct stat my_stat;
 	char *fullpath = NULL;
+	bool cached;
 
 	if (search) {
 		if (strcmp(filename, "kernel") == 0)
 			return aa_features_new_from_kernel(features);
-		f = search_path(filename, &fullpath);
+		f = search_path(filename, &fullpath, &cached);
-		PDEBUG("abi lookup '%s' -> '%s' f %p\n", filename, fullpath, f);
+		PDEBUG("abi lookup '%s' -> '%s' f %p cached %d\n", filename, fullpath, f, cached);
+		if (!f && cached) {
+			*features = NULL;
+			return 0;
+		}
 	} else {
 		f = fopen(filename, "r");
 		PDEBUG("abi relpath '%s' f %p\n", filename, f);
@@ -1809,10 +1827,15 @@ static void abi_features(char *filename, bool search)
 			yyerror(_("failed to find features abi '%s': %m"), filename);
 	}
 	if (policy_features) {
-		if (!aa_features_is_equal(tmp_features, policy_features)) {
+		if (tmp_features) {
-			pwarn(WARN_ABI, _("%s: %s features abi '%s' differs from policy declared feature abi, using the features abi declared in policy\n"), progname, current_filename, filename);
+			if (!aa_features_is_equal(tmp_features, policy_features)) {
+				pwarn(WARN_ABI, _("%s: %s features abi '%s' differs from policy declared feature abi, using the features abi declared in policy\n"), progname, current_filename, filename);
+			}
+			aa_features_unref(tmp_features);
 		}
-		aa_features_unref(tmp_features);
+	} else if (!tmp_features) {
+		/* skipped reinclude, but features not set */
+		yyerror(_("failed features abi not set but include cache skipped\n"));
 	} else {
 		/* first features abi declaration */
 		policy_features = tmp_features;