Skip to content

Commit

Permalink
fix(scap): remove a static global from event converter
Browse files Browse the repository at this point in the history 
Signed-off-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>
  • Loading branch information
@gnosek @poiana
gnosek authored and poiana committed Feb 10, 2025
1 parent a879a77 commit a2a32d7
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 27 deletions.
25 changes: 17 additions & 8 deletions test/libscap/test_suites/engines/savefile/convert_event_test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,13 @@ class convert_event_test : public testing::Test {
static constexpr uint16_t safe_margin = 100;

protected:
virtual void TearDown() {
// At every iteration we want to clear the storage in the converter
scap_clear_converter_storage();
virtual void SetUp() {
m_converter_buf = scap_convert_alloc_buffer();
ASSERT_NE(m_converter_buf, nullptr);
}

virtual void TearDown() { scap_convert_free_buffer(m_converter_buf); }

safe_scap_evt_t create_safe_scap_event(uint64_t ts,
uint64_t tid,
ppm_event_code event_type,
Expand All @@ -54,7 +57,8 @@ class convert_event_test : public testing::Test {
// We assume it's okay to create a new event with the same size as the expected event
auto storage = new_safe_scap_evt((scap_evt *)calloc(1, expected_evt->len));
// First we check the conversion result matches the expected result
ASSERT_EQ(scap_convert_event(storage.get(), evt_to_convert.get(), error), expected_res)
ASSERT_EQ(scap_convert_event(m_converter_buf, storage.get(), evt_to_convert.get(), error),
expected_res)
<< "Different conversion results: " << error;
if(!scap_compare_events(storage.get(), expected_evt.get(), error)) {
printf("\nExpected event:\n");
Expand All @@ -69,15 +73,17 @@ class convert_event_test : public testing::Test {
// We assume it's okay to create a new event with the same size as the expected event
auto storage = new_safe_scap_evt((scap_evt *)calloc(1, evt_to_convert->len));
// First we check the conversion result matches the expected result
ASSERT_EQ(scap_convert_event(storage.get(), evt_to_convert.get(), error), CONVERSION_ERROR)
ASSERT_EQ(scap_convert_event(m_converter_buf, storage.get(), evt_to_convert.get(), error),
CONVERSION_ERROR)
<< "The conversion is not failed: " << error;
}
void assert_single_conversion_skip(safe_scap_evt_t evt_to_convert) {
char error[SCAP_LASTERR_SIZE] = {'\0'};
// We assume it's okay to create a new event with the same size as the expected event
auto storage = new_safe_scap_evt((scap_evt *)calloc(1, evt_to_convert->len));
// First we check the conversion result matches the expected result
ASSERT_EQ(scap_convert_event(storage.get(), evt_to_convert.get(), error), CONVERSION_SKIP)
ASSERT_EQ(scap_convert_event(m_converter_buf, storage.get(), evt_to_convert.get(), error),
CONVERSION_SKIP)
<< "The conversion is not skipped: " << error;
}
void assert_full_conversion(safe_scap_evt_t evt_to_convert, safe_scap_evt_t expected_evt) {
Expand All @@ -99,7 +105,8 @@ class convert_event_test : public testing::Test {
conv_num++) {
// Copy the new event into the one to convert for the next conversion.
memcpy(to_convert_evt.get(), new_evt.get(), new_evt->len);
conv_res = scap_convert_event((scap_evt *)new_evt.get(),
conv_res = scap_convert_event(m_converter_buf,
(scap_evt *)new_evt.get(),
(scap_evt *)to_convert_evt.get(),
error);
}
Expand Down Expand Up @@ -128,12 +135,14 @@ class convert_event_test : public testing::Test {
void assert_event_storage_presence(safe_scap_evt_t expected_evt) {
char error[SCAP_LASTERR_SIZE] = {'\0'};
int64_t tid = expected_evt.get()->tid;
auto event = scap_retrieve_evt_from_converter_storage(tid);
auto event = scap_retrieve_evt_from_converter_storage(m_converter_buf, tid);
if(!event) {
FAIL() << "Event with tid " << tid << " not found in the storage";
}
if(!scap_compare_events(event, expected_evt.get(), error)) {
FAIL() << "Different events: " << error;
}
}

struct scap_convert_buffer *m_converter_buf = nullptr;
};
51 changes: 36 additions & 15 deletions userspace/libscap/engine/savefile/converter/converter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,9 @@ static inline safe_scap_evt_t safe_scap_evt(scap_evt *evt) {
return safe_scap_evt_t{evt, free};
}

// use a shared pointer to store the events
static std::unordered_map<uint64_t, safe_scap_evt_t> evt_storage = {};
struct scap_convert_buffer {
std::unordered_map<uint64_t, safe_scap_evt_t> evt_storage = {};
};

static const char *get_event_name(ppm_event_code event_type) {
const struct ppm_event_info *event_info = &g_event_info[event_type];
Expand All @@ -50,19 +51,21 @@ static char get_direction_char(ppm_event_code event_type) {
}
}

static void clear_evt(uint64_t tid) {
static void clear_evt(std::unordered_map<uint64_t, safe_scap_evt_t> &evt_storage, uint64_t tid) {
if(evt_storage.find(tid) != evt_storage.end()) {
evt_storage[tid].reset();
}
}

static void store_evt(uint64_t tid, scap_evt *evt) {
static void store_evt(std::unordered_map<uint64_t, safe_scap_evt_t> &evt_storage,
uint64_t tid,
scap_evt *evt) {
// if there was a previous event for this tid, we can overwrite the pointer because it means we
// don't need it anymore. We need to keep the enter event until we retrieve it in the
// corresponding exit event, but if the same thread is doing another enter event it means the
// previous syscall is already completed.

clear_evt(tid);
clear_evt(evt_storage, tid);

scap_evt *tmp_evt = (scap_evt *)malloc(evt->len);
if(!tmp_evt) {
Expand All @@ -72,7 +75,8 @@ static void store_evt(uint64_t tid, scap_evt *evt) {
evt_storage[tid] = safe_scap_evt(tmp_evt);
}

static scap_evt *retrieve_evt(uint64_t tid) {
static scap_evt *retrieve_evt(std::unordered_map<uint64_t, safe_scap_evt_t> &evt_storage,
uint64_t tid) {
if(evt_storage.find(tid) != evt_storage.end()) {
return evt_storage[tid].get();
}
Expand Down Expand Up @@ -289,15 +293,19 @@ extern "C" bool is_conversion_needed(scap_evt *evt_to_convert) {
return false;
}

extern "C" scap_evt *scap_retrieve_evt_from_converter_storage(uint64_t tid) {
return retrieve_evt(tid);
extern "C" scap_evt *scap_retrieve_evt_from_converter_storage(
std::unordered_map<uint64_t, safe_scap_evt_t> &evt_storage,
uint64_t tid) {
return retrieve_evt(evt_storage, tid);
}

extern "C" void scap_clear_converter_storage() {
extern "C" void scap_clear_converter_storage(
std::unordered_map<uint64_t, safe_scap_evt_t> &evt_storage) {
evt_storage.clear();
}

static conversion_result convert_event(scap_evt *new_evt,
static conversion_result convert_event(std::unordered_map<uint64_t, safe_scap_evt_t> &evt_storage,
scap_evt *new_evt,
scap_evt *evt_to_convert,
const conversion_info &ci,
char *error) {
Expand Down Expand Up @@ -326,7 +334,7 @@ static conversion_result convert_event(scap_evt *new_evt,
return CONVERSION_SKIP;

case C_ACTION_STORE:
store_evt(evt_to_convert->tid, evt_to_convert);
store_evt(evt_storage, evt_to_convert->tid, evt_to_convert);
return CONVERSION_SKIP;

case C_ACTION_ADD_PARAMS:
Expand Down Expand Up @@ -368,7 +376,7 @@ static conversion_result convert_event(scap_evt *new_evt,
break;

case C_INSTR_FROM_ENTER:
tmp_evt = retrieve_evt(evt_to_convert->tid);
tmp_evt = retrieve_evt(evt_storage, evt_to_convert->tid);
if(!tmp_evt) {
// It could be due to different reasons:
// - we dropped the enter event in the capture
Expand Down Expand Up @@ -430,7 +438,7 @@ static conversion_result convert_event(scap_evt *new_evt,

if(PPME_IS_EXIT(evt_to_convert->type)) {
// We can free the enter event for this thread because we don't need it anymore.
clear_evt(evt_to_convert->tid);
clear_evt(evt_storage, evt_to_convert->tid);
}
new_evt->len = params_offset;

Expand All @@ -439,7 +447,12 @@ static conversion_result convert_event(scap_evt *new_evt,
return is_conversion_needed(new_evt) ? CONVERSION_CONTINUE : CONVERSION_COMPLETED;
}

extern "C" conversion_result scap_convert_event(scap_evt *new_evt,
extern "C" struct scap_convert_buffer *scap_convert_alloc_buffer() {
return new scap_convert_buffer();
}

extern "C" conversion_result scap_convert_event(struct scap_convert_buffer *buf,
scap_evt *new_evt,
scap_evt *evt_to_convert,
char *error) {
// This should be checked by the caller but just double check here
Expand All @@ -464,5 +477,13 @@ extern "C" conversion_result scap_convert_event(scap_evt *new_evt,
}

// If we reached this point we have for sure an entry in the conversion table.
return convert_event(new_evt, evt_to_convert, g_conversion_table.at(conv_key), error);
return convert_event(buf->evt_storage,
new_evt,
evt_to_convert,
g_conversion_table.at(conv_key),
error);
}

extern "C" void scap_convert_free_buffer(struct scap_convert_buffer *buf) {
delete buf;
}
12 changes: 9 additions & 3 deletions userspace/libscap/engine/savefile/converter/converter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,19 @@ typedef struct ppm_evt_hdr scap_evt;
// 50 consecutive conversions on the same event should be more than enough
#define MAX_CONVERSION_BOUNDARY 50

conversion_result scap_convert_event(scap_evt* new_evt, scap_evt* evt_to_convert, char* error);
struct scap_convert_buffer;

struct scap_convert_buffer* scap_convert_alloc_buffer();
conversion_result scap_convert_event(struct scap_convert_buffer* buf,
scap_evt* new_evt,
scap_evt* evt_to_convert,
char* error);
void scap_convert_free_buffer(struct scap_convert_buffer* buf);

bool is_conversion_needed(scap_evt* evt_to_convert);

// Only for testing purposes
scap_evt* scap_retrieve_evt_from_converter_storage(uint64_t tid);
void scap_clear_converter_storage();
scap_evt* scap_retrieve_evt_from_converter_storage(struct scap_convert_buffer* buf, uint64_t tid);

#ifdef __cplusplus
};
Expand Down
1 change: 1 addition & 0 deletions userspace/libscap/engine/savefile/savefile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,5 @@ struct savefile_engine {
// Used by the scap-file converter
char* m_new_evt;
char* m_to_convert_evt;
struct scap_convert_buffer* m_converter_buf;
};
16 changes: 15 additions & 1 deletion userspace/libscap/engine/savefile/scap_savefile.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2053,7 +2053,8 @@ static int32_t next(struct scap_engine_handle engine,
conv_num++) {
// Before each conversion we move the current event into the storage
memcpy(handle->m_to_convert_evt, *pevent, (*pevent)->len);
conv_res = scap_convert_event((scap_evt *)handle->m_new_evt,
conv_res = scap_convert_event(handle->m_converter_buf,
(scap_evt *)handle->m_new_evt,
(scap_evt *)handle->m_to_convert_evt,
handle->m_lasterr);
// At the end of the conversion in any case we swith to the new event pointer
Expand Down Expand Up @@ -2239,6 +2240,14 @@ static int32_t init(struct scap *main_handle, struct scap_open_args *oargs) {
}
}

handle->m_converter_buf = scap_convert_alloc_buffer();
if(!handle->m_converter_buf) {
snprintf(main_handle->m_lasterr,
SCAP_LASTERR_SIZE,
"error allocating the conversion buffer");
return SCAP_FAILURE;
}

return SCAP_SUCCESS;
}

Expand Down Expand Up @@ -2268,6 +2277,11 @@ static int32_t scap_savefile_close(struct scap_engine_handle engine) {
handle->m_to_convert_evt = NULL;
}

if(handle->m_converter_buf) {
scap_convert_free_buffer(handle->m_converter_buf);
handle->m_converter_buf = NULL;
}

return SCAP_SUCCESS;
}

Expand Down

0 comments on commit a2a32d7

Please # to comment.