From e0504e697af49e721a7ce79b3389e1d909cd1e49 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Wed, 4 Sep 2024 12:00:13 -0700 Subject: [PATCH 1/8] Make libaalogparse lexer fully reentrant by removing its globals Signed-off-by: Ryan Lee --- libraries/libapparmor/src/grammar.y | 6 +++- libraries/libapparmor/src/parser.h | 7 ++++ libraries/libapparmor/src/scanner.l | 52 ++++++++++++++--------------- 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/libraries/libapparmor/src/grammar.y b/libraries/libapparmor/src/grammar.y index 6797a03b9..40bac9193 100644 --- a/libraries/libapparmor/src/grammar.y +++ b/libraries/libapparmor/src/grammar.y @@ -497,11 +497,15 @@ _parse_yacc(char *str) yydebug = 1; #endif - aalogparse_lex_init(&scanner); + struct string_buf string_buf = {.buf = NULL, .buf_len = 0, .buf_alloc = 0}; + + aalogparse_lex_init_extra(&string_buf, &scanner); lex_buf = aalogparse__scan_string(str, scanner); /* Ignore return value to return an AA_RECORD_INVALID event */ (void)aalogparse_parse(scanner); aalogparse__delete_buffer(lex_buf, scanner); aalogparse_lex_destroy(scanner); + // free(NULL) is a no-op + free(string_buf.buf); return ret_record; } diff --git a/libraries/libapparmor/src/parser.h b/libraries/libapparmor/src/parser.h index dcb6e5c91..521debb4b 100644 --- a/libraries/libapparmor/src/parser.h +++ b/libraries/libapparmor/src/parser.h @@ -19,6 +19,13 @@ #ifndef __AA_LOG_PARSER_H__ #define __AA_LOG_PARSER_H__ +// Internal-only type +struct string_buf { + char *buf; + unsigned int buf_len; + unsigned int buf_alloc; +}; + extern void _init_log_record(aa_log_record *record); extern aa_log_record *_parse_yacc(char *str); extern char *hex_to_string(char *str); diff --git a/libraries/libapparmor/src/scanner.l b/libraries/libapparmor/src/scanner.l index 6a3ce9be7..72e8825e5 100644 --- a/libraries/libapparmor/src/scanner.l +++ b/libraries/libapparmor/src/scanner.l @@ -19,6 +19,7 @@ %option nounput %option noyy_top_state %option reentrant +%option extra-type="struct string_buf*" %option prefix="aalogparse_" %option bison-bridge %option header-file="scanner.h" @@ -34,40 +35,37 @@ #define YY_NO_INPUT -unsigned int string_buf_alloc = 0; -unsigned int string_buf_len = 0; -char *string_buf = NULL; - -void string_buf_reset() +void string_buf_reset(struct string_buf* char_buf) { /* rewind buffer to zero, possibly doing initial allocation too */ - string_buf_len = 0; - if (string_buf == NULL) { - string_buf_alloc = 128; - string_buf = malloc(string_buf_alloc); - assert(string_buf != NULL); + char_buf->buf_len = 0; + if (char_buf->buf == NULL) { + char_buf->buf_alloc = 128; + char_buf->buf = malloc(char_buf->buf_alloc); + assert(char_buf->buf != NULL); } /* always start with a valid but empty string */ - string_buf[0] = '\0'; + char_buf->buf[0] = '\0'; } -void string_buf_append(unsigned int length, char *text) +void string_buf_append(struct string_buf* char_buf, unsigned int length, char *text) { - unsigned int current_length = string_buf_len; + unsigned int current_length = char_buf->buf_len; /* handle calling ..._append before ..._reset */ - if (string_buf == NULL) string_buf_reset(); + if (char_buf->buf == NULL) string_buf_reset(char_buf); - string_buf_len += length; + char_buf->buf_len += length; /* expand allocation if this append would exceed the allocation */ - while (string_buf_len >= string_buf_alloc) { - string_buf_alloc *= 2; - string_buf = realloc(string_buf, string_buf_alloc); - assert(string_buf != NULL); + while (char_buf->buf_len >= char_buf->buf_alloc) { + // TODO: overflow? + char_buf->buf_alloc *= 2; + char_buf->buf = realloc(char_buf->buf, char_buf->buf_alloc); + assert(char_buf->buf != NULL); } /* copy and unconditionally terminate */ - memcpy(string_buf+current_length, text, length); - string_buf[string_buf_len] = '\0'; + memcpy(char_buf->buf+current_length, text, length); + char_buf->buf[char_buf->buf_len] = '\0'; } %} @@ -232,7 +230,7 @@ yy_flex_debug = 0; {open_paren} { return(TOK_OPEN_PAREN); } {close_paren} { BEGIN(INITIAL); return(TOK_CLOSE_PAREN); } {ws} { } - \" { string_buf_reset(); BEGIN(quoted_string); } + \" { string_buf_reset(yyextra); BEGIN(quoted_string); } {ID}+ { yylval->t_str = strdup(yytext); BEGIN(INITIAL); @@ -241,20 +239,20 @@ yy_flex_debug = 0; {equals} { return(TOK_EQUALS); } } -\" { string_buf_reset(); BEGIN(quoted_string); } +\" { string_buf_reset(yyextra); BEGIN(quoted_string); } \" { /* End of the quoted string */ BEGIN(INITIAL); - yylval->t_str = strdup(string_buf); + yylval->t_str = strdup(yyextra->buf); return(TOK_QUOTED_STRING); } -\\(.|\n) { string_buf_append(1, &yytext[1]); } +\\(.|\n) { string_buf_append(yyextra, 1, &yytext[1]); } -[^\\\n\"]+ { string_buf_append(yyleng, yytext); } +[^\\\n\"]+ { string_buf_append(yyextra, yyleng, yytext); } { - \" { string_buf_reset(); BEGIN(quoted_string); } + \" { string_buf_reset(yyextra); BEGIN(quoted_string); } {hexstring} { yylval->t_str = hex_to_string(yytext); BEGIN(INITIAL); return(TOK_HEXSTRING);} {equals} { return(TOK_EQUALS); } . { /* eek, error! try another state */ BEGIN(INITIAL); yyless(0); } From c5c7565357259f7f5c24e920842551cca43da542 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Wed, 4 Sep 2024 14:54:02 -0700 Subject: [PATCH 2/8] Silence -Wyacc because we rely on GNU bison extensions to yacc Signed-off-by: Ryan Lee --- libraries/libapparmor/src/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/libapparmor/src/Makefile.am b/libraries/libapparmor/src/Makefile.am index 239fc7506..0b89eccfb 100644 --- a/libraries/libapparmor/src/Makefile.am +++ b/libraries/libapparmor/src/Makefile.am @@ -44,7 +44,7 @@ include $(COMMONDIR)/Make.rules BUILT_SOURCES = grammar.h scanner.h af_protos.h AM_LFLAGS = -v -AM_YFLAGS = -d -p aalogparse_ +AM_YFLAGS = -Wno-yacc -d -p aalogparse_ AM_CPPFLAGS = -D_GNU_SOURCE -I$(top_srcdir)/include/ scanner.h: scanner.l $(LEX) -v $< From dba76694436cff1c6fdf412b9d113179efaf95bf Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Wed, 4 Sep 2024 14:54:50 -0700 Subject: [PATCH 3/8] Also make the bison parser of libaalogparse fully reentrant Signed-off-by: Ryan Lee --- libraries/libapparmor/src/grammar.y | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/libraries/libapparmor/src/grammar.y b/libraries/libapparmor/src/grammar.y index 40bac9193..23f746169 100644 --- a/libraries/libapparmor/src/grammar.y +++ b/libraries/libapparmor/src/grammar.y @@ -15,6 +15,12 @@ * along with this program. If not, see . */ +/* aalogparse_error now requires visibility of the aa_log_record type + * Also include in a %code requires block to add it to the header + */ +%code requires{ + #include +} %{ @@ -41,12 +47,10 @@ #define debug_unused_ unused_ #endif -aa_log_record *ret_record; - /* Since we're a library, on any errors we don't want to print out any * error messages. We should probably add a debug interface that does * emit messages when asked for. */ -void aalogparse_error(unused_ void *scanner, debug_unused_ char const *s) +void aalogparse_error(unused_ void *scanner, aa_log_record *ret_record, debug_unused_ char const *s) { #if (YYDEBUG != 0) printf("ERROR: %s\n", s); @@ -89,9 +93,10 @@ aa_record_event_type lookup_aa_event(unsigned int type) %define parse.trace */ -%define api.pure +%define api.pure full %lex-param{void *scanner} %parse-param{void *scanner} +%parse-param{aa_log_record *ret_record} %union { @@ -284,8 +289,9 @@ audit_user_msg: TOK_KEY_MSG TOK_EQUALS audit_id audit_user_msg_tail audit_id: TOK_AUDIT TOK_OPEN_PAREN TOK_AUDIT_DIGITS TOK_PERIOD TOK_AUDIT_DIGITS TOK_COLON TOK_AUDIT_DIGITS TOK_CLOSE_PAREN TOK_COLON { - if (!asprintf(&ret_record->audit_id, "%s.%s:%s", $3, $5, $7)) - yyerror(scanner, YY_("Out of memory")); + if (!asprintf(&ret_record->audit_id, "%s.%s:%s", $3, $5, $7)) { + yyerror(scanner, ret_record, YY_("Out of memory")); + } ret_record->epoch = atol($3); ret_record->audit_sub_id = atoi($7); free($3); @@ -485,8 +491,7 @@ _parse_yacc(char *str) YY_BUFFER_STATE lex_buf; yyscan_t scanner; - ret_record = NULL; - ret_record = malloc(sizeof(aa_log_record)); + aa_log_record *ret_record = malloc(sizeof(aa_log_record)); _init_log_record(ret_record); @@ -502,7 +507,7 @@ _parse_yacc(char *str) aalogparse_lex_init_extra(&string_buf, &scanner); lex_buf = aalogparse__scan_string(str, scanner); /* Ignore return value to return an AA_RECORD_INVALID event */ - (void)aalogparse_parse(scanner); + (void)aalogparse_parse(scanner, ret_record); aalogparse__delete_buffer(lex_buf, scanner); aalogparse_lex_destroy(scanner); // free(NULL) is a no-op From 7ff045583db7220c6bc3adf729192c8f671ff453 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Tue, 10 Sep 2024 10:28:26 -0700 Subject: [PATCH 4/8] Remove manual YYDEBUG define in grammar.y The generated grammar.h already sets the correct YYDEBUG value regardless of whether parse.trace is defined Signed-off-by: Ryan Lee --- libraries/libapparmor/src/grammar.y | 8 -------- 1 file changed, 8 deletions(-) diff --git a/libraries/libapparmor/src/grammar.y b/libraries/libapparmor/src/grammar.y index 23f746169..0adde6fc0 100644 --- a/libraries/libapparmor/src/grammar.y +++ b/libraries/libapparmor/src/grammar.y @@ -24,14 +24,6 @@ %{ -/* set the following to non-zero to get bison to emit debugging - * information about tokens given and rules matched. - * Also: - * Uncomment the %defines - * parse.error - * parse.trace - */ -#define YYDEBUG 0 #include #include #include "parser.h" From 6a55fb5613d17b1f8f177eef6cd3e8bec5b266fd Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Wed, 4 Sep 2024 17:07:41 -0700 Subject: [PATCH 5/8] Inline _parse_yacc in libaalogparse This function was only ever called once inside libaalogparse.c, and it looks simple enough to not need to be split out into its own helper function. As this function was never exposed publicly in installed header files, removing it is not a breaking API change. Signed-off-by: Ryan Lee --- libraries/libapparmor/src/grammar.y | 31 ----------------------- libraries/libapparmor/src/libaalogparse.c | 31 ++++++++++++++++++++++- libraries/libapparmor/src/parser.h | 1 - 3 files changed, 30 insertions(+), 33 deletions(-) diff --git a/libraries/libapparmor/src/grammar.y b/libraries/libapparmor/src/grammar.y index 0adde6fc0..7eb47cb9e 100644 --- a/libraries/libapparmor/src/grammar.y +++ b/libraries/libapparmor/src/grammar.y @@ -475,34 +475,3 @@ protocol: TOK_QUOTED_STRING } ; %% - -aa_log_record * -_parse_yacc(char *str) -{ - /* yydebug = 1; */ - YY_BUFFER_STATE lex_buf; - yyscan_t scanner; - - aa_log_record *ret_record = malloc(sizeof(aa_log_record)); - - _init_log_record(ret_record); - - if (ret_record == NULL) - return NULL; - -#if (YYDEBUG != 0) - yydebug = 1; -#endif - - struct string_buf string_buf = {.buf = NULL, .buf_len = 0, .buf_alloc = 0}; - - aalogparse_lex_init_extra(&string_buf, &scanner); - lex_buf = aalogparse__scan_string(str, scanner); - /* Ignore return value to return an AA_RECORD_INVALID event */ - (void)aalogparse_parse(scanner, ret_record); - aalogparse__delete_buffer(lex_buf, scanner); - aalogparse_lex_destroy(scanner); - // free(NULL) is a no-op - free(string_buf.buf); - return ret_record; -} diff --git a/libraries/libapparmor/src/libaalogparse.c b/libraries/libapparmor/src/libaalogparse.c index 934eeb258..23b2c9cf7 100644 --- a/libraries/libapparmor/src/libaalogparse.c +++ b/libraries/libapparmor/src/libaalogparse.c @@ -34,13 +34,42 @@ #include #include "parser.h" +#include "grammar.h" +#include "scanner.h" + /* This is mostly just a wrapper around the code in grammar.y */ aa_log_record *parse_record(char *str) { + YY_BUFFER_STATE lex_buf; + yyscan_t scanner; + aa_log_record *ret_record; + if (str == NULL) return NULL; - return _parse_yacc(str); + ret_record = malloc(sizeof(aa_log_record)); + + _init_log_record(ret_record); + + if (ret_record == NULL) + return NULL; + + struct string_buf string_buf = {.buf = NULL, .buf_len = 0, .buf_alloc = 0}; + +#if (YYDEBUG != 0) + /* Warning: this is still a global even in reentrant parsers */ + aalogparse_debug = 1; +#endif + + aalogparse_lex_init_extra(&string_buf, &scanner); + lex_buf = aalogparse__scan_string(str, scanner); + /* Ignore return value to return an AA_RECORD_INVALID event */ + (void)aalogparse_parse(scanner, ret_record); + aalogparse__delete_buffer(lex_buf, scanner); + aalogparse_lex_destroy(scanner); + /* free(NULL) is a no-op */ + free(string_buf.buf); + return ret_record; } void free_record(aa_log_record *record) diff --git a/libraries/libapparmor/src/parser.h b/libraries/libapparmor/src/parser.h index 521debb4b..d81e4fef8 100644 --- a/libraries/libapparmor/src/parser.h +++ b/libraries/libapparmor/src/parser.h @@ -27,7 +27,6 @@ struct string_buf { }; extern void _init_log_record(aa_log_record *record); -extern aa_log_record *_parse_yacc(char *str); extern char *hex_to_string(char *str); extern char *ipproto_to_string(unsigned int proto); From 66e14392937bdbe2d8df7d30cfa00592f3f435e8 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Thu, 5 Sep 2024 10:35:12 -0700 Subject: [PATCH 6/8] Add an aalogparse reentrancy test for simultaneous log parsing from different threads Signed-off-by: Ryan Lee --- .gitignore | 4 + libraries/libapparmor/src/Makefile.am | 6 +- .../src/tst_aalogparse_reentrancy.c | 154 ++++++++++++++++++ 3 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 libraries/libapparmor/src/tst_aalogparse_reentrancy.c diff --git a/.gitignore b/.gitignore index 059b9448f..c32a35423 100644 --- a/.gitignore +++ b/.gitignore @@ -109,6 +109,10 @@ libraries/libapparmor/src/tst_aalogmisc libraries/libapparmor/src/tst_aalogmisc.log libraries/libapparmor/src/tst_aalogmisc.o libraries/libapparmor/src/tst_aalogmisc.trs +libraries/libapparmor/src/tst_aalogparse_reentrancy +libraries/libapparmor/src/tst_aalogparse_reentrancy.log +libraries/libapparmor/src/tst_aalogparse_reentrancy.o +libraries/libapparmor/src/tst_aalogparse_reentrancy.trs libraries/libapparmor/src/tst_features libraries/libapparmor/src/tst_features.log libraries/libapparmor/src/tst_features.o diff --git a/libraries/libapparmor/src/Makefile.am b/libraries/libapparmor/src/Makefile.am index 0b89eccfb..986b85f9c 100644 --- a/libraries/libapparmor/src/Makefile.am +++ b/libraries/libapparmor/src/Makefile.am @@ -73,6 +73,10 @@ CLEANFILES = libapparmor.pc tst_aalogmisc_SOURCES = tst_aalogmisc.c tst_aalogmisc_LDADD = .libs/libapparmor.a +tst_aalogparse_reentrancy_SOURCES = tst_aalogparse_reentrancy.c +tst_aalogparse_reentrancy_LDADD = .libs/libapparmor.a +tst_aalogparse_reentrancy_LDFLAGS = -pthread + tst_features_SOURCES = tst_features.c tst_features_LDADD = .libs/libapparmor.a @@ -80,7 +84,7 @@ tst_kernel_SOURCES = tst_kernel.c tst_kernel_LDADD = .libs/libapparmor.a tst_kernel_LDFLAGS = -pthread -check_PROGRAMS = tst_aalogmisc tst_features tst_kernel +check_PROGRAMS = tst_aalogmisc tst_aalogparse_reentrancy tst_features tst_kernel TESTS = $(check_PROGRAMS) .PHONY: check-local diff --git a/libraries/libapparmor/src/tst_aalogparse_reentrancy.c b/libraries/libapparmor/src/tst_aalogparse_reentrancy.c new file mode 100644 index 000000000..d11947b6a --- /dev/null +++ b/libraries/libapparmor/src/tst_aalogparse_reentrancy.c @@ -0,0 +1,154 @@ +#include +#include + +#include + +#include "private.h" + +const char* log_line = "[23342.075380] audit: type=1400 audit(1725487203.971:1831): apparmor=\"DENIED\" operation=\"open\" class=\"file\" profile=\"snap-update-ns.firmware-updater\" name=\"/proc/202964/maps\" pid=202964 comm=\"5\" requested_mask=\"r\" denied_mask=\"r\" fsuid=1000 ouid=0"; +const char* log_line_2 = "[ 4074.372559] audit: type=1400 audit(1725553393.143:793): apparmor=\"DENIED\" operation=\"capable\" class=\"cap\" profile=\"/usr/lib/snapd/snap-confine\" pid=19034 comm=\"snap-confine\" capability=12 capname=\"net_admin\""; + +static int pthread_barrier_ok(int barrier_result) { + return barrier_result == 0 || barrier_result == PTHREAD_BARRIER_SERIAL_THREAD; +} + +static int nullcmp_and_strcmp(const void *s1, const void *s2) +{ + /* Return 0 if both pointers are NULL & non-zero if only one is NULL */ + if (!s1 || !s2) + return s1 != s2; + + return strcmp(s1, s2); +} + +int aa_log_record_eq(aa_log_record *record1, aa_log_record *record2) { + int are_eq = 1; + + are_eq &= (record1->version == record2->version); + are_eq &= (record1->event == record2->event); + are_eq &= (record1->pid == record2->pid); + are_eq &= (record1->peer_pid == record2->peer_pid); + are_eq &= (record1->task == record2->task); + are_eq &= (record1->magic_token == record2->magic_token); + are_eq &= (record1->epoch == record2->epoch); + are_eq &= (record1->audit_sub_id == record2->audit_sub_id); + + are_eq &= (record1->bitmask == record2->bitmask); + are_eq &= (nullcmp_and_strcmp(record1->audit_id, record2->audit_id) == 0); + are_eq &= (nullcmp_and_strcmp(record1->operation, record2->operation) == 0); + are_eq &= (nullcmp_and_strcmp(record1->denied_mask, record2->denied_mask) == 0); + are_eq &= (nullcmp_and_strcmp(record1->requested_mask, record2->requested_mask) == 0); + are_eq &= (record1->fsuid == record2->fsuid); + are_eq &= (record1->ouid == record2->ouid); + are_eq &= (nullcmp_and_strcmp(record1->profile, record2->profile) == 0); + are_eq &= (nullcmp_and_strcmp(record1->peer_profile, record2->peer_profile) == 0); + are_eq &= (nullcmp_and_strcmp(record1->comm, record2->comm) == 0); + are_eq &= (nullcmp_and_strcmp(record1->name, record2->name) == 0); + are_eq &= (nullcmp_and_strcmp(record1->name2, record2->name2) == 0); + are_eq &= (nullcmp_and_strcmp(record1->namespace, record2->namespace) == 0); + are_eq &= (nullcmp_and_strcmp(record1->attribute, record2->attribute) == 0); + are_eq &= (record1->parent == record2->parent); + are_eq &= (nullcmp_and_strcmp(record1->info, record2->info) == 0); + are_eq &= (nullcmp_and_strcmp(record1->peer_info, record2->peer_info) == 0); + are_eq &= (record1->error_code == record2->error_code); + are_eq &= (nullcmp_and_strcmp(record1->active_hat, record2->active_hat) == 0); + are_eq &= (nullcmp_and_strcmp(record1->net_family, record2->net_family) == 0); + are_eq &= (nullcmp_and_strcmp(record1->net_protocol, record2->net_protocol) == 0); + are_eq &= (nullcmp_and_strcmp(record1->net_sock_type, record2->net_sock_type) == 0); + are_eq &= (nullcmp_and_strcmp(record1->net_local_addr, record2->net_local_addr) == 0); + are_eq &= (record1->net_local_port == record2->net_local_port); + are_eq &= (nullcmp_and_strcmp(record1->net_foreign_addr, record2->net_foreign_addr) == 0); + are_eq &= (record1->net_foreign_port == record2->net_foreign_port); + + are_eq &= (nullcmp_and_strcmp(record1->execpath, record2->execpath) == 0); + + are_eq &= (nullcmp_and_strcmp(record1->dbus_bus, record2->dbus_bus) == 0); + are_eq &= (nullcmp_and_strcmp(record1->dbus_path, record2->dbus_path) == 0); + are_eq &= (nullcmp_and_strcmp(record1->dbus_interface, record2->dbus_interface) == 0); + are_eq &= (nullcmp_and_strcmp(record1->dbus_member, record2->dbus_member) == 0); + are_eq &= (nullcmp_and_strcmp(record1->signal, record2->signal) == 0); + are_eq &= (nullcmp_and_strcmp(record1->peer, record2->peer) == 0); + + are_eq &= (nullcmp_and_strcmp(record1->fs_type, record2->fs_type) == 0); + are_eq &= (nullcmp_and_strcmp(record1->flags, record2->flags) == 0); + are_eq &= (nullcmp_and_strcmp(record1->src_name, record2->src_name) == 0); + + are_eq &= (nullcmp_and_strcmp(record1->class, record2->class) == 0); + + are_eq &= (nullcmp_and_strcmp(record1->net_addr, record2->net_addr) == 0); + are_eq &= (nullcmp_and_strcmp(record1->peer_addr, record2->peer_addr) == 0); + return are_eq; +} + +typedef struct { + char* log; + pthread_barrier_t *barrier; +} pthread_parse_args; + +void* pthread_parse_log(void* args) { + pthread_parse_args *args_real = (pthread_parse_args *) args; + int barrier_wait_result = pthread_barrier_wait(args_real->barrier); + /* Return NULL and fail test if barrier wait fails */ + if (!pthread_barrier_ok(barrier_wait_result)) { + return NULL; + } + aa_log_record *record = parse_record(args_real->log); + return (void*) record; +} + +#define NUM_THREADS 16 + +int main(void) { + pthread_t thread_ids[NUM_THREADS]; + pthread_barrier_t barrier; + int barrier_wait_result; + aa_log_record* parsed_logs[NUM_THREADS]; + int rc = 0; + /* Set up arguments to be passed to threads */ + pthread_parse_args args = {.log=log_line, .barrier=&barrier}; + pthread_parse_args args2 = {.log=log_line_2, .barrier=&barrier}; + + MY_TEST(NUM_THREADS > 2, "Test requires more than 2 threads"); + + /* Use barrier to synchronize the start of log parsing among all the threads + * This increases the likelihood of tickling race conditions, if there are any + */ + MY_TEST(pthread_barrier_init(&barrier, NULL, NUM_THREADS+1) == 0, + "Could not init pthread barrier"); + for (int i=0; iversion == AA_RECORD_SYNTAX_V2, "Log should have parsed as v2 form"); + MY_TEST(parsed_logs[i]->event == AA_RECORD_DENIED, "Log should have parsed as denied"); + + /* Also check i==0 and i==1 as a sanity check for aa_log_record_eq */ + if (i%2 == 0) { + MY_TEST(aa_log_record_eq(parsed_logs[0], parsed_logs[i]), "Log 0 != Log even"); + } else { + MY_TEST(aa_log_record_eq(parsed_logs[1], parsed_logs[i]), "Log 1 != Log odd"); + } + } + MY_TEST(!aa_log_record_eq(parsed_logs[0], parsed_logs[1]), "Log 0 and log 1 shouldn't be equal"); + /* Clean up */ + MY_TEST(pthread_barrier_destroy(&barrier) == 0, "Could not destroy pthread barrier"); + for (int i=0; i Date: Thu, 5 Sep 2024 10:40:53 -0700 Subject: [PATCH 7/8] Make parse_record take a const char pointer since it never modified str anyways This shouldn't be a breaking change because it's fine to pass a non-const pointer to a function taking a const pointer, but not the other way round Signed-off-by: Ryan Lee --- libraries/libapparmor/include/aalogparse.h | 2 +- libraries/libapparmor/src/libaalogparse.c | 2 +- libraries/libapparmor/src/tst_aalogparse_reentrancy.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/libraries/libapparmor/include/aalogparse.h b/libraries/libapparmor/include/aalogparse.h index 6e6294262..105113de0 100644 --- a/libraries/libapparmor/include/aalogparse.h +++ b/libraries/libapparmor/include/aalogparse.h @@ -177,7 +177,7 @@ typedef struct * @return Parsed data. */ aa_log_record * -parse_record(char *str); +parse_record(const char *str); /** * Frees all struct data. diff --git a/libraries/libapparmor/src/libaalogparse.c b/libraries/libapparmor/src/libaalogparse.c index 23b2c9cf7..bdacc6a59 100644 --- a/libraries/libapparmor/src/libaalogparse.c +++ b/libraries/libapparmor/src/libaalogparse.c @@ -38,7 +38,7 @@ #include "scanner.h" /* This is mostly just a wrapper around the code in grammar.y */ -aa_log_record *parse_record(char *str) +aa_log_record *parse_record(const char *str) { YY_BUFFER_STATE lex_buf; yyscan_t scanner; diff --git a/libraries/libapparmor/src/tst_aalogparse_reentrancy.c b/libraries/libapparmor/src/tst_aalogparse_reentrancy.c index d11947b6a..43b932adb 100644 --- a/libraries/libapparmor/src/tst_aalogparse_reentrancy.c +++ b/libraries/libapparmor/src/tst_aalogparse_reentrancy.c @@ -81,7 +81,7 @@ int aa_log_record_eq(aa_log_record *record1, aa_log_record *record2) { } typedef struct { - char* log; + const char* log; pthread_barrier_t *barrier; } pthread_parse_args; From 79670745d61b633dbff0428ed961e505cf46f820 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Thu, 5 Sep 2024 11:11:49 -0700 Subject: [PATCH 8/8] Remove remnants of comments regarding old apparmor log format The entry AA_RECORD_SYNTAX_V1 is only there for API compatibility reasons. If we wanted to remove it, we could just renumber the other two entries to preserve ABI compatibility. However, it seems easier to just delete the entry if we ever break backcompat with a libapparmor2. Signed-off-by: Ryan Lee --- libraries/libapparmor/include/aalogparse.h | 69 +--------------------- 1 file changed, 3 insertions(+), 66 deletions(-) diff --git a/libraries/libapparmor/include/aalogparse.h b/libraries/libapparmor/include/aalogparse.h index 105113de0..ced77ab47 100644 --- a/libraries/libapparmor/include/aalogparse.h +++ b/libraries/libapparmor/include/aalogparse.h @@ -26,10 +26,10 @@ #define AA_RECORD_LINK 16 /** - * This is just for convenience now that we have two - * wildly different grammars. + * Enum representing which syntax version the log entry used. + * Support for V1 parsing was completely removed in 2011 and that enum entry + * is only still there for API compatibility reasons. */ - typedef enum { AA_RECORD_SYNTAX_V1, @@ -48,69 +48,6 @@ typedef enum AA_RECORD_STATUS /* Configuration change */ } aa_record_event_type; -/** - * With the sole exception of active_hat, this is a 1:1 - * mapping from the keys that the new syntax uses. - * - * Some examples of the old syntax and how they're mapped with the aa_log_record struct: - * - * "PERMITTING r access to /path (program_name(12345) profile /profile active hat)" - * - operation: access - * - requested_mask: r - * - pid: 12345 - * - profile: /profile - * - name: /path - * - info: program_name - * - active_hat: hat - * - * "REJECTING mkdir on /path/to/something (bash(23415) profile /bin/freak-aa-out active /bin/freak-aa-out" - * - operation: mkdir - * - name: /path/to/something - * - info: bash - * - pid: 23415 - * - profile: /bin/freak-aa-out - * - active_hat: /bin/freak-aa-out - * - * "REJECTING xattr set on /path/to/something (bash(23415) profile /bin/freak-aa-out active /bin/freak-aa-out)" - * - operation: xattr - * - attribute: set - * - name: /path/to/something - * - info: bash - * - pid: 23415 - * - profile: /bin/freak-aa-out - * - active_hat: /bin/freak-aa-out - * - * "PERMITTING attribute (something) change to /else (bash(23415) profile /bin/freak-aa-out active /bin/freak-aa-out)" - * - operation: setattr - * - attribute: something - * - name: /else - * - info: bash - * - pid: 23415 - * - profile: /bin/freak-aa-out - * - active_hat: /bin/freak-aa-out - * - * "PERMITTING access to capability 'cap' (bash(23415) profile /bin/freak-aa-out active /bin/freak-aa-out)" - * - operation: capability - * - name: cap - * - info: bash - * - pid: 23415 - * - profile: /bin/freak-aa-out - * - active_hat: /bin/freak-aa-out - * - * "LOGPROF-HINT unknown_hat TESTHAT pid=27764 profile=/change_hat_test/test_hat active=/change_hat_test/test_hat" - * - operation: change_hat - * - name: TESTHAT - * - info: unknown_hat - * - pid: 27764 - * - profile: /change_hat_test/test_hat - * - active_hat: /change_hat_test/test_hat - * - * "LOGPROF-HINT fork pid=27764 child=38229" - * - operation: clone - * - task: 38229 - * - pid: 27764 - **/ - typedef struct { aa_record_syntax_version version;