From bb03d9ee089d52d33ed200239a9d47ca827f0d32 Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Wed, 6 Aug 2025 18:28:35 -0300 Subject: [PATCH] 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();