cleanup: address review comments

Signed-off-by: Andrea Terzolo <[email protected]>
Co-authored-by: Federico Di Pierro <[email protected]>

Author: 2 people

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(

userspace/libscap/engine/savefile/converter/converter.cpp

@@ -17,14 +17,12 @@ limitations under the License.
 */
 
 #include <driver/ppm_events_public.h>
-#include <converter/types.h>
-#include <converter/results.h>
 #include <converter/table.h>
+#include <converter/results.h>
 #include <converter/debug_macro.h>
 #include <stdarg.h>
 #include <cstdio>
 #include <cassert>
-#include <unordered_map>
 #include <string>
 #include <stdexcept>
 #include <memory>
@@ -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,22 +262,22 @@ 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;
 
 	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,
 			               &params_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);
 }

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 <libscap/scap_const.h>
+#include <driver/ppm_events_public.h>
+#include <converter/table.h>
+
+const std::unordered_map<conversion_key, conversion_info> g_conversion_table = {};

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 <converter/types.h>
-#include <libscap/scap_const.h>
-#include <driver/ppm_events_public.h>
-
 #include <unordered_map>
 
-static std::unordered_map<conversion_key, conversion_info> g_conversion_table = {};
+extern const std::unordered_map<conversion_key, conversion_info> g_conversion_table;

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,

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) {