mirrors/apparmor
1
0
Fork 0
mirror of https://gitlab.com/apparmor/apparmor.git synced 2025-03-04 08:24:42 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions

Merge Make libaalogparse fully reentrant by removing its globals

Browse source 
Tested by using Valgrind's Helgrind and DRD against the reentrancy test that I wrote: they both report no errors with the changes while reporting many errors with the old versions.

Commits "Inline _parse_yacc in libaalogparse" and "Make parse_record take a const char pointer since it never modified str anyways" have a tiny potential to be backwards-incompatible changes: I have justified why they shouldn't be in the commit messages, but it's worth looking over in case I was mistaken and we need to back those out.

Signed-off-by: Ryan Lee <ryan.lee@canonical.com>

MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1322
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
This commit is contained in:
John Johansen 2024-09-11 07:54:39 +00:00
commit 37cac653d1
8 changed files with 243 additions and 141 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
.gitignore vendored
View file

@ -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

71
libraries/libapparmor/include/aalogparse.h
View file

@ -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;
@ -177,7 +114,7 @@ typedef struct
* @return Parsed data.
*/
aa_log_record *
parse_record(char *str);
parse_record(const char *str);
/**
* Frees all struct data.

8
libraries/libapparmor/src/Makefile.am
View file

@ -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 $<
@ -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

54
libraries/libapparmor/src/grammar.y
View file

@ -15,17 +15,15 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/
/* 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 <aalogparse.h>
}
%{
/* 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 <string.h>
#include <aalogparse.h>
#include "parser.h"
@ -41,12 +39,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 +85,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 +281,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);
@ -477,31 +475,3 @@ protocol: TOK_QUOTED_STRING
}
;
%%
aa_log_record *
_parse_yacc(char *str)
{
/* yydebug = 1; */
YY_BUFFER_STATE lex_buf;
yyscan_t scanner;
ret_record = NULL;
ret_record = malloc(sizeof(aa_log_record));
_init_log_record(ret_record);
if (ret_record == NULL)
return NULL;
#if (YYDEBUG != 0)
yydebug = 1;
#endif
aalogparse_lex_init(&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);
return ret_record;
}

33
libraries/libapparmor/src/libaalogparse.c
View file

@ -34,13 +34,42 @@
#include <aalogparse.h>
#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)
aa_log_record *parse_record(const 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)

8
libraries/libapparmor/src/parser.h
View file

@ -19,8 +19,14 @@
#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);
extern char *ipproto_to_string(unsigned int proto);

52
libraries/libapparmor/src/scanner.l
View file

@ -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); }
<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);
}
<quoted_string>\\(.|\n) { string_buf_append(1, &yytext[1]); }
<quoted_string>\\(.|\n) { string_buf_append(yyextra, 1, &yytext[1]); }
<quoted_string>[^\\\n\"]+ { string_buf_append(yyleng, yytext); }
<quoted_string>[^\\\n\"]+ { string_buf_append(yyextra, yyleng, yytext); }
<safe_string>{
\" { 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); }

154
libraries/libapparmor/src/tst_aalogparse_reentrancy.c Normal file
View file

@ -0,0 +1,154 @@
#include <pthread.h>
#include <string.h>
#include <aalogparse.h>
#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 {
const 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; i<NUM_THREADS; i++) {
if (i%2 == 0) {
pthread_create(&thread_ids[i], NULL, pthread_parse_log, (void *) &args);
} else {
pthread_create(&thread_ids[i], NULL, pthread_parse_log, (void *) &args2);
}
}
/* Final barrier_wait to set off the thread race */
barrier_wait_result = pthread_barrier_wait(&barrier);
MY_TEST(pthread_barrier_ok(barrier_wait_result), "Could not wait on pthread barrier");
/* Wait for threads to finish parsing the logs */
for (int i=0; i<NUM_THREADS; i++) {
MY_TEST(pthread_join(thread_ids[i], (void*) &parsed_logs[i]) == 0, "Could not join thread");
}
/* Check that all logs parsed and are equal */
for (int i=0; i<NUM_THREADS; i++) {
MY_TEST(parsed_logs[i] != NULL, "Log failed to parse");
MY_TEST(parsed_logs[i]->version == 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<NUM_THREADS; i++) {
free_record(parsed_logs[i]);
}
return rc;
}