From 5094053c30d8d2e9165be608c4f438ad937b1e8e Mon Sep 17 00:00:00 2001 From: Andrea Terzolo Date: Wed, 27 Nov 2024 12:38:00 +0100 Subject: [PATCH] cleanup: address review comments Signed-off-by: Andrea Terzolo Co-authored-by: Federico Di Pierro --- .../engine/savefile/converter/CMakeLists.txt | 2 +- .../engine/savefile/converter/converter.cpp | 40 +++++++------- .../engine/savefile/converter/table.cpp | 23 ++++++++ .../libscap/engine/savefile/converter/table.h | 6 +-- userspace/libscap/scap.h | 1 - userspace/libscap/scap_event.c | 53 ------------------- 6 files changed, 46 insertions(+), 79 deletions(-) create mode 100644 userspace/libscap/engine/savefile/converter/table.cpp diff --git a/userspace/libscap/engine/savefile/converter/CMakeLists.txt b/userspace/libscap/engine/savefile/converter/CMakeLists.txt index 05e9f2ecb2..967798ec8b 100644 --- a/userspace/libscap/engine/savefile/converter/CMakeLists.txt +++ b/userspace/libscap/engine/savefile/converter/CMakeLists.txt @@ -15,7 +15,7 @@ # Since we have circular dependencies between libscap and the savefile engine, make this library # always static (directly linked into libscap) -add_library(scap_savefile_converter STATIC converter.cpp) +add_library(scap_savefile_converter STATIC converter.cpp table.cpp) add_dependencies(scap_savefile_converter uthash) target_include_directories( diff --git a/userspace/libscap/engine/savefile/converter/converter.cpp b/userspace/libscap/engine/savefile/converter/converter.cpp index 4a051b1eb0..ed91382d1e 100644 --- a/userspace/libscap/engine/savefile/converter/converter.cpp +++ b/userspace/libscap/engine/savefile/converter/converter.cpp @@ -17,14 +17,12 @@ limitations under the License. */ #include -#include -#include #include +#include #include #include #include #include -#include #include #include #include @@ -124,7 +122,6 @@ static void push_default_parameter(scap_evt *evt, uint16_t *params_offset, uint8 // Otherwise we will access the wrong entry in the event table. const struct ppm_event_info *event_info = &(g_event_info[evt->type]); uint16_t len = scap_get_size_bytes_from_type(event_info->params[param_num].type); - char *ptr = scap_get_default_value_from_type(event_info->params[param_num].type); uint16_t lens_offset = sizeof(scap_evt) + param_num * sizeof(uint16_t); PRINT_MESSAGE( @@ -136,8 +133,11 @@ static void push_default_parameter(scap_evt *evt, uint16_t *params_offset, uint8 *params_offset, lens_offset); - // If value is NULL, the len should be 0 - memcpy((char *)evt + *params_offset, ptr, len); + // The default param will be always 0 so we just need to copy the right number of 0 bytes. + // `uint64_t` should be enough for all the types considering that types like CHARBUF, BYTEBUF + // have `len==0` + uint64_t val = 0; + memcpy((char *)evt + *params_offset, (char *)&val, len); *params_offset += len; memcpy((char *)evt + lens_offset, &len, sizeof(uint16_t)); } @@ -242,7 +242,7 @@ extern "C" void scap_clear_converter_storage() { static conversion_result convert_event(scap_evt *new_evt, scap_evt *evt_to_convert, - const conversion_info *ci, + const conversion_info &ci, char *error) { ///////////////////////////// // Dispatch the action @@ -251,7 +251,7 @@ static conversion_result convert_event(scap_evt *new_evt, uint16_t params_offset = 0; int param_to_populate = 0; - switch(ci->action) { + switch(ci.action) { case C_ACTION_SKIP: return CONVERSION_SKIP; @@ -262,7 +262,7 @@ static conversion_result convert_event(scap_evt *new_evt, case C_ACTION_ADD_PARAMS: memcpy(new_evt, evt_to_convert, sizeof(scap_evt)); // The new number of params is the previous one plus the number of conversion instructions. - new_evt->nparams = evt_to_convert->nparams + ci->instr.size(); + new_evt->nparams = evt_to_convert->nparams + ci.instr.size(); params_offset = copy_old_params(new_evt, evt_to_convert); param_to_populate = evt_to_convert->nparams; break; @@ -270,14 +270,14 @@ static conversion_result convert_event(scap_evt *new_evt, case C_ACTION_CHANGE_TYPE: memcpy(new_evt, evt_to_convert, sizeof(scap_evt)); // The new number of params is the number of conversion instructions. - new_evt->nparams = ci->instr.size(); - new_evt->type = ci->desired_type; + new_evt->nparams = ci.instr.size(); + new_evt->type = ci.desired_type; params_offset = sizeof(scap_evt) + new_evt->nparams * sizeof(uint16_t); param_to_populate = 0; break; default: - snprintf(error, SCAP_LASTERR_SIZE, "Unhandled conversion action '%d'.", ci->action); + snprintf(error, SCAP_LASTERR_SIZE, "Unhandled conversion action '%d'.", ci.action); return CONVERSION_ERROR; } @@ -293,10 +293,10 @@ static conversion_result convert_event(scap_evt *new_evt, bool used_enter_event = false; // We iterate over the instructions - for(int i = 0; i < ci->instr.size(); i++, param_to_populate++) { + for(int i = 0; i < ci.instr.size(); i++, param_to_populate++) { PRINT_MESSAGE("Instruction n° %d. Param to populate: %d\n", i, param_to_populate); - switch(ci->instr[i].flags) { + switch(ci.instr[i].flags) { case C_INSTR_FROM_DEFAULT: tmp_evt = NULL; break; @@ -329,14 +329,14 @@ static conversion_result convert_event(scap_evt *new_evt, case C_INSTR_FROM_OLD: tmp_evt = evt_to_convert; - if(tmp_evt->nparams <= ci->instr[i].param_num) { + if(tmp_evt->nparams <= ci.instr[i].param_num) { // todo!: this sounds like an error but let's see in the future. At the moment we // fail snprintf(error, SCAP_LASTERR_SIZE, "We want to take parameter '%d' from event '%d' but this event has only " "'%d' parameters!", - ci->instr[i].param_num, + ci.instr[i].param_num, tmp_evt->type, tmp_evt->nparams); return CONVERSION_ERROR; @@ -347,8 +347,8 @@ static conversion_result convert_event(scap_evt *new_evt, snprintf(error, SCAP_LASTERR_SIZE, "Unknown instruction (flags: %d, param_num: %d).", - ci->instr[i].flags, - ci->instr[i].param_num); + ci.instr[i].flags, + ci.instr[i].param_num); return CONVERSION_ERROR; } @@ -359,7 +359,7 @@ static conversion_result convert_event(scap_evt *new_evt, tmp_evt, ¶ms_offset, param_to_populate, - ci->instr[i].param_num); + ci.instr[i].param_num); } } @@ -400,5 +400,5 @@ 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[conv_key], error); + return convert_event(new_evt, evt_to_convert, g_conversion_table.at(conv_key), error); } diff --git a/userspace/libscap/engine/savefile/converter/table.cpp b/userspace/libscap/engine/savefile/converter/table.cpp new file mode 100644 index 0000000000..cac323ffca --- /dev/null +++ b/userspace/libscap/engine/savefile/converter/table.cpp @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: Apache-2.0 +/* +Copyright (C) 2024 The Falco Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +*/ + +#include +#include +#include + +const std::unordered_map g_conversion_table = {}; diff --git a/userspace/libscap/engine/savefile/converter/table.h b/userspace/libscap/engine/savefile/converter/table.h index 15047d7b53..1bb66c6de4 100644 --- a/userspace/libscap/engine/savefile/converter/table.h +++ b/userspace/libscap/engine/savefile/converter/table.h @@ -15,12 +15,10 @@ See the License for the specific language governing permissions and limitations under the License. */ + #pragma once #include -#include -#include - #include -static std::unordered_map g_conversion_table = {}; +extern const std::unordered_map g_conversion_table; diff --git a/userspace/libscap/scap.h b/userspace/libscap/scap.h index 4927c5d123..7136090e51 100644 --- a/userspace/libscap/scap.h +++ b/userspace/libscap/scap.h @@ -856,7 +856,6 @@ int32_t scap_event_encode_params_v(struct scap_sized_buffer event_buf, uint32_t n, va_list args); uint8_t scap_get_size_bytes_from_type(enum ppm_param_type t); -char* scap_get_default_value_from_type(enum ppm_param_type t); bool scap_compare_events(scap_evt* curr, scap_evt* expected, char* error); scap_evt* scap_create_event_v(char* error, uint64_t ts, diff --git a/userspace/libscap/scap_event.c b/userspace/libscap/scap_event.c index b67f139ac4..86ab922f65 100644 --- a/userspace/libscap/scap_event.c +++ b/userspace/libscap/scap_event.c @@ -410,59 +410,6 @@ uint8_t scap_get_size_bytes_from_type(enum ppm_param_type t) { return 0; } -char *scap_get_default_value_from_type(enum ppm_param_type t) { - static uint8_t default_uint8 = 0; - static uint16_t default_uint16 = 0; - static uint32_t default_uint32 = 0; - static uint64_t default_uint64 = 0; - switch(t) { - case PT_INT8: - case PT_UINT8: - case PT_FLAGS8: - case PT_ENUMFLAGS8: - return (char *)&default_uint8; - case PT_INT16: - case PT_UINT16: - case PT_FLAGS16: - case PT_ENUMFLAGS16: - case PT_SYSCALLID: - return (char *)&default_uint16; - case PT_INT32: - case PT_UINT32: - case PT_FLAGS32: - case PT_ENUMFLAGS32: - case PT_UID: - case PT_GID: - case PT_MODE: - return (char *)&default_uint32; - case PT_INT64: - case PT_UINT64: - case PT_RELTIME: - case PT_ABSTIME: - case PT_ERRNO: - case PT_FD: - case PT_PID: - return (char *)&default_uint64; - // Should be ok to return NULL since the len will be 0 - case PT_BYTEBUF: - case PT_CHARBUF: - case PT_SOCKADDR: - case PT_SOCKTUPLE: - case PT_FDLIST: - case PT_FSPATH: - case PT_CHARBUFARRAY: - case PT_CHARBUF_PAIR_ARRAY: - case PT_FSRELPATH: - case PT_DYN: - return NULL; - default: - // We forgot to handle something - ASSERT(false); - break; - } - return 0; -} - // This should be only used for testing purposes in production we should use directly `memcmp` for // the whole event bool scap_compare_events(scap_evt *curr, scap_evt *expected, char *error) {