From d9866f0a2492a60df7613fb545561ccf510e31ed Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Tue, 5 Aug 2025 15:48:28 -0300 Subject: [PATCH 1/3] parser: fix leaking disconnected paths when merging Valgrind showed that the disconnected paths variables were leaking during the merge. That happened because flagvals did not implement a destructor freeing the variables, so they leaked. flagvals cannot implement a destructor, because that would make it a non-trivial union member and parser_yacc.y would not compile. This patch implements a "clear" function that is supposed to act as the destructor. $ /usr/bin/valgrind --leak-check=full --error-exitcode=151 ../apparmor_parser -Q -I simple_tests/ -M ./features_files/features.all flags_ok_disconnected_ipc15.sd ... ==3708747== 5 bytes in 1 blocks are definitely lost in loss record 1 of 11 ==3708747== at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==3708747== by 0x492E35E: strdup (strdup.c:42) ==3708747== by 0x14C74E: set_disconnected_path (profile.h:188) ==3708747== by 0x14C74E: flagvals::init(char const*) (profile.h:223) ==3708747== by 0x14859B: yyparse() (parser_yacc.y:592) ==3708747== by 0x141A99: process_profile(int, aa_kernel_interface*, char const*, aa_policy_cache*) (parser_main.c:1187) ==3708747== by 0x135421: main (parser_main.c:1771) ... Signed-off-by: Georgia Garcia --- parser/parser_yacc.y | 1 + parser/profile.cc | 5 +---- parser/profile.h | 10 ++++++++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/parser/parser_yacc.y b/parser/parser_yacc.y index 0b5d4111f..0e4e4a643 100644 --- a/parser/parser_yacc.y +++ b/parser/parser_yacc.y @@ -577,6 +577,7 @@ flags: opt_flags TOK_OPENPAREN flagvals TOK_CLOSEPAREN flagvals: flagvals flagval { $1.merge($2); + $2.clear(); $$ = $1; }; diff --git a/parser/profile.cc b/parser/profile.cc index 52a195b9d..2e46ddf22 100644 --- a/parser/profile.cc +++ b/parser/profile.cc @@ -78,6 +78,7 @@ void ProfileList::dump_profile_names(bool children) Profile::~Profile() { hat_table.clear(); + flags.clear(); free_cod_entries(entries); free_cond_entry_list(xattrs); @@ -97,10 +98,6 @@ Profile::~Profile() free(name); if (attachment) free(attachment); - if (flags.disconnected_path) - free(flags.disconnected_path); - if (flags.disconnected_ipc) - free(flags.disconnected_ipc); if (ns) free(ns); for (int i = (AA_EXEC_LOCAL >> 10) + 1; i < AA_EXEC_COUNT; i++) diff --git a/parser/profile.h b/parser/profile.h index e7df0f90e..313940167 100644 --- a/parser/profile.h +++ b/parser/profile.h @@ -175,6 +175,12 @@ public: signal = 0; error = 0; } + + void clear(void) { + free(disconnected_path); + free(disconnected_ipc); + } + void init(const char *str) { init(); @@ -301,7 +307,7 @@ public: } // same ignore rhs.disconnect_path } else { - disconnected_path = rhs.disconnected_path; + disconnected_path = strdup(rhs.disconnected_path); } } if (rhs.disconnected_ipc) { @@ -311,7 +317,7 @@ public: } // same so do nothing } else { - disconnected_ipc = rhs.disconnected_ipc; + disconnected_ipc = strdup(rhs.disconnected_ipc); } } if (rhs.signal) { From bb03d9ee089d52d33ed200239a9d47ca827f0d32 Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Wed, 6 Aug 2025 18:28:35 -0300 Subject: [PATCH 2/3] parser: fix leak on conflicting x modifiers When the "conflicting x modifiers" exception was thrown, the DFA object creation would fail, therefore the destructor would not be called and the states previously allocated would leak. Unfortunately there's no way to call the destructor if the object was not created, so I moved the contents of the destructor into a cleanup helper function to be called in both instances. $ /usr/bin/valgrind --leak-check=full --error-exitcode=151 ../apparmor_parser -Q -I simple_tests/ -M ./features_files/features.all simple_tests/xtrans/x-conflict.sd ==564911== 592 (112 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 19 ==564911== at 0x4846FA3: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==564911== by 0x189C9A: DFA::add_new_state(optflags const&, std::set, std::allocator >*, std::set, std::allocator >*, State*) (hfa.cc:337) ==564911== by 0x18CB22: add_new_state (hfa.cc:357) ==564911== by 0x18CB22: DFA::DFA(Node*, optflags const&, bool) (hfa.cc:473) ==564911== by 0x178263: aare_rules::create_chfa(int*, std::vector >&, optflags const&, bool, bool) (aare_rules.cc:258) ==564911== by 0x178A4F: aare_rules::create_dfablob(unsigned long*, int*, std::vector >&, optflags const&, bool, bool) (aare_rules.cc:359) ==564911== by 0x14E4E1: process_profile_regex(Profile*) (parser_regex.c:791) ==564911== by 0x154CDF: process_profile_rules(Profile*) (parser_policy.c:194) ==564911== by 0x154E0F: post_process_profile(Profile*, int) (parser_policy.c:240) ==564911== by 0x154F7A: post_process_policy_list (parser_policy.c:257) ==564911== by 0x154F7A: post_process_policy(int) (parser_policy.c:267) ==564911== by 0x141B17: process_profile(int, aa_kernel_interface*, char const*, aa_policy_cache*) (parser_main.c:1227) ==564911== by 0x135421: main (parser_main.c:1771) Fixes: https://gitlab.com/apparmor/apparmor/-/issues/534 Signed-off-by: Georgia Garcia --- parser/libapparmor_re/hfa.cc | 17 +++++++++++------ parser/libapparmor_re/hfa.h | 9 +++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/parser/libapparmor_re/hfa.cc b/parser/libapparmor_re/hfa.cc index 75699ce8a..a1fea95b0 100644 --- a/parser/libapparmor_re/hfa.cc +++ b/parser/libapparmor_re/hfa.cc @@ -334,7 +334,16 @@ State *DFA::add_new_state(optflags const &opts, NodeSet *anodes, ProtoState proto; proto.init(nnodev, anodev); - State *state = new State(opts, node_map.size(), proto, other, filedfa); + State *state; + try { + state = new State(opts, node_map.size(), proto, other, filedfa); + } catch(int error) { + /* this function is called in the DFA object creation, + * and the exception prevents the destructor from + * being called, so call the helper here */ + cleanup(); + throw error; + } pair x = node_map.insert(proto, state); if (x.second == false) { delete state; @@ -522,11 +531,7 @@ DFA::DFA(Node *root, optflags const &opts, bool buildfiledfa): root(root), filed DFA::~DFA() { - anodes_cache.clear(); - nnodes_cache.clear(); - - for (Partition::iterator i = states.begin(); i != states.end(); i++) - delete *i; + cleanup(); } State *DFA::match_len(State *state, const char *str, size_t len) diff --git a/parser/libapparmor_re/hfa.h b/parser/libapparmor_re/hfa.h index 5bdd00bb5..8a9ea7e70 100644 --- a/parser/libapparmor_re/hfa.h +++ b/parser/libapparmor_re/hfa.h @@ -368,6 +368,15 @@ class DFA { NodeMap node_map; std::list work_queue; + void cleanup(void) { + anodes_cache.clear(); + nnodes_cache.clear(); + + for (Partition::iterator i = states.begin(); i != states.end(); i++) { + delete *i; + } + states.clear(); + } public: DFA(Node *root, optflags const &flags, bool filedfa); virtual ~DFA(); From 43fa5f88a774a967fbece1687ed32d28674648a4 Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Wed, 6 Aug 2025 18:34:46 -0300 Subject: [PATCH 3/3] parser: fix cases leaks when new state creation fails For every item in "cases", a new state is created, but if the creation of one of them fails, the rest of the items in that list would not be deleted and would leak. Fix it by continuing to iterate over the items in the list and delete them, and then re-throw the exception. $ /usr/bin/valgrind --leak-check=full --error-exitcode=151 ../apparmor_parser -Q -I simple_tests/ -M ./features_files/features.all simple_tests/xtrans/x-conflict.sd ==564911== 208 (48 direct, 160 indirect) bytes in 1 blocks are definitely lost in loss record 15 of 19 ==564911== at 0x4846FA3: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so) ==564911== by 0x179E74: CharNode::follow(Cases&) (expr-tree.h:447) ==564911== by 0x189F8B: DFA::update_state_transitions(optflags const&, State*) (hfa.cc:376) ==564911== by 0x18A25B: DFA::process_work_queue(char const*, optflags const&) (hfa.cc:442) ==564911== by 0x18CB65: DFA::DFA(Node*, optflags const&, bool) (hfa.cc:486) ==564911== by 0x178263: aare_rules::create_chfa(int*, std::vector >&, optflags const&, bool, bool) (aare_rules.cc:258) ==564911== by 0x178A4F: aare_rules::create_dfablob(unsigned long*, int*, std::vector >&, optflags const&, bool, bool) (aare_rules.cc:359) ==564911== by 0x14E4E1: process_profile_regex(Profile*) (parser_regex.c:791) ==564911== by 0x154CDF: process_profile_rules(Profile*) (parser_policy.c:194) ==564911== by 0x154E0F: post_process_profile(Profile*, int) (parser_policy.c:240) ==564911== by 0x154F7A: post_process_policy_list (parser_policy.c:257) ==564911== by 0x154F7A: post_process_policy(int) (parser_policy.c:267) ==564911== by 0x141B17: process_profile(int, aa_kernel_interface*, char const*, aa_policy_cache*) (parser_main.c:1227) Fixes: https://gitlab.com/apparmor/apparmor/-/issues/534 Signed-off-by: Georgia Garcia --- parser/libapparmor_re/hfa.cc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/parser/libapparmor_re/hfa.cc b/parser/libapparmor_re/hfa.cc index a1fea95b0..83aa19783 100644 --- a/parser/libapparmor_re/hfa.cc +++ b/parser/libapparmor_re/hfa.cc @@ -401,7 +401,17 @@ void DFA::update_state_transitions(optflags const &opts, State *state) */ for (Cases::iterator j = cases.begin(); j != cases.end(); j++) { State *target; - target = add_new_state(opts, j->second, nonmatching); + try { + target = add_new_state(opts, j->second, nonmatching); + } catch (int error) { + /* when add_new_state fails, there could still + * be NodeSets in the rest of cases, so clean + * them up before re-throwing the exception */ + for (Cases::iterator k = ++j; k != cases.end(); k++) { + delete k->second; + } + throw error; + } /* Don't insert transition that the otherwise transition * already covers