From 553a77239aa13276439703e707c1e7bee0e7096c Mon Sep 17 00:00:00 2001 From: Luca Guerra Date: Wed, 7 Feb 2024 16:01:56 +0000 Subject: [PATCH] cleanup(libsinsp): cleanup unaligned access in plugin framework+tests Signed-off-by: Luca Guerra --- .../engine/source_plugin/source_plugin.c | 17 ++++--- userspace/libscap/scap_event.c | 5 +- userspace/libsinsp/sinsp.cpp | 3 +- .../libsinsp/test/plugins/plugin_source.cpp | 29 +++++------ .../libsinsp/test/plugins/syscall_async.cpp | 40 ++++----------- .../libsinsp/test/plugins/syscall_source.cpp | 49 ++++++------------- 6 files changed, 52 insertions(+), 91 deletions(-) diff --git a/userspace/libscap/engine/source_plugin/source_plugin.c b/userspace/libscap/engine/source_plugin/source_plugin.c index 4ac8398bd4..70116a5275 100644 --- a/userspace/libscap/engine/source_plugin/source_plugin.c +++ b/userspace/libscap/engine/source_plugin/source_plugin.c @@ -21,6 +21,7 @@ limitations under the License. #include #include #include +#include #include #include @@ -209,7 +210,10 @@ static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT // Sanity checks in case a plugin implements a non-syscall event source. // If a plugin has event sourcing capability and has a specific ID, then // it is allowed to produce only plugin events of its own event source. - uint32_t* plugin_id = (uint32_t*)((uint8_t*) evt + sizeof(scap_evt) + 4 + 4); + uint32_t* pplugin_id = (uint32_t*)((uint8_t*) evt + sizeof(scap_evt) + 4 + 4); + uint32_t plugin_id; + memcpy(&plugin_id, pplugin_id, sizeof(plugin_id)); + if (handle->m_input_plugin->id != 0) { /* @@ -224,13 +228,14 @@ static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT } // forcely setting plugin ID with the one of the open plugin - if (*plugin_id == 0) + if (plugin_id == 0) { - *plugin_id = handle->m_input_plugin->id; + plugin_id = handle->m_input_plugin->id; + memcpy(pplugin_id, &plugin_id, sizeof(plugin_id)); } - else if (*plugin_id != handle->m_input_plugin->id) + else if (plugin_id != handle->m_input_plugin->id) { - snprintf(lasterr, SCAP_LASTERR_SIZE, "unexpected plugin ID in plugin event: plugin='%s', expected_id=%d, actual_id=%d", handle->m_input_plugin->name, handle->m_input_plugin->id, *plugin_id); + snprintf(lasterr, SCAP_LASTERR_SIZE, "unexpected plugin ID in plugin event: plugin='%s', expected_id=%d, actual_id=%d", handle->m_input_plugin->name, handle->m_input_plugin->id, plugin_id); return SCAP_FAILURE; } } @@ -238,7 +243,7 @@ static int32_t next(struct scap_engine_handle engine, OUT scap_evt** pevent, OUT if (evt->type == PPME_PLUGINEVENT_E) { // a zero plugin ID is not allowed for PPME_PLUGINEVENT_E - if (*plugin_id == 0) + if (plugin_id == 0) { snprintf(lasterr, SCAP_LASTERR_SIZE, "malformed plugin event produced by plugin (no ID): '%s'", handle->m_input_plugin->name); return SCAP_FAILURE; diff --git a/userspace/libscap/scap_event.c b/userspace/libscap/scap_event.c index d906f4869b..09bf57d59e 100644 --- a/userspace/libscap/scap_event.c +++ b/userspace/libscap/scap_event.c @@ -331,7 +331,10 @@ int32_t scap_event_encode_params_v(const struct scap_sized_buffer event_buf, siz len = len + sizeof(uint32_t); #endif - *event_size = len; + if (event_size != NULL) + { + *event_size = len; + } // we were not able to write the event to the buffer if (!scap_buffer_can_fit(event_buf, len)) diff --git a/userspace/libsinsp/sinsp.cpp b/userspace/libsinsp/sinsp.cpp index 536e644314..b99c969619 100644 --- a/userspace/libsinsp/sinsp.cpp +++ b/userspace/libsinsp/sinsp.cpp @@ -18,6 +18,7 @@ limitations under the License. #include #include +#include #ifndef _WIN32 #include #include @@ -2222,7 +2223,7 @@ void sinsp::handle_plugin_async_event(const sinsp_plugin& p, std::unique_ptrget_scap_evt() + sizeof(scap_evt) + 4+4+4); - *plid = cur_plugin_id; + memcpy(plid, &cur_plugin_id, sizeof(cur_plugin_id)); handle_async_event(std::move(evt)); } } diff --git a/userspace/libsinsp/test/plugins/plugin_source.cpp b/userspace/libsinsp/test/plugins/plugin_source.cpp index 411377b30c..32b67a9750 100644 --- a/userspace/libsinsp/test/plugins/plugin_source.cpp +++ b/userspace/libsinsp/test/plugins/plugin_source.cpp @@ -21,6 +21,7 @@ limitations under the License. #include #include +#include #include "test_plugins.h" @@ -139,27 +140,21 @@ static ss_plugin_rc plugin_next_batch(ss_plugin_t* s, ss_instance_t* i, uint32_t *nevts = 1; *evts = &istate->evt; - istate->evt->type = PPME_PLUGINEVENT_E; - istate->evt->tid = -1; - istate->evt->ts = UINT64_MAX; - istate->evt->len = sizeof(ss_plugin_event); - istate->evt->nparams = 2; - uint8_t* parambuf = &istate->evt_buf[0] + sizeof(ss_plugin_event); + char error[SCAP_LASTERR_SIZE]; - // lenghts - *((uint32_t*) parambuf) = sizeof(uint32_t); - parambuf += sizeof(uint32_t); - *((uint32_t*) parambuf) = strlen(s_evt_data) + 1; - parambuf += sizeof(uint32_t); + int32_t encode_res = scap_event_encode_params(scap_sized_buffer{istate->evt, sizeof(istate->evt_buf)}, + nullptr, error, PPME_PLUGINEVENT_E, 2, + plugin_get_id(), s_evt_data); - // params - *((uint32_t*) parambuf) = plugin_get_id(); - parambuf += sizeof(uint32_t); - strcpy((char*) parambuf, s_evt_data); - parambuf += strlen(s_evt_data) + 1; + if (encode_res == SCAP_FAILURE) + { + return SS_PLUGIN_FAILURE; + } + + istate->evt->tid = -1; + istate->evt->ts = UINT64_MAX; - istate->evt->len += parambuf - (&istate->evt_buf[0] + sizeof(ss_plugin_event)); istate->count--; return SS_PLUGIN_SUCCESS; } diff --git a/userspace/libsinsp/test/plugins/syscall_async.cpp b/userspace/libsinsp/test/plugins/syscall_async.cpp index d56f83d3a2..df4b06f282 100644 --- a/userspace/libsinsp/test/plugins/syscall_async.cpp +++ b/userspace/libsinsp/test/plugins/syscall_async.cpp @@ -23,6 +23,7 @@ limitations under the License. #include #include +#include #include "test_plugins.h" @@ -124,35 +125,6 @@ static const char* plugin_get_last_error(ss_plugin_t* s) return ((plugin_state *) s)->lasterr.c_str(); } -static void encode_async_event(ss_plugin_event* evt, uint64_t tid, const char* name, const char* data) -{ - // set event info - evt->type = PPME_ASYNCEVENT_E; - evt->tid = tid; - evt->len = sizeof(ss_plugin_event); - evt->nparams = 3; - - // lenghts - uint8_t* parambuf = (uint8_t*) evt + sizeof(ss_plugin_event); - *((uint32_t*) parambuf) = sizeof(uint32_t); - parambuf += sizeof(uint32_t); - *((uint32_t*) parambuf) = strlen(name) + 1; - parambuf += sizeof(uint32_t); - *((uint32_t*) parambuf) = strlen(data) + 1; - parambuf += sizeof(uint32_t); - - // params - // skip plugin ID, it will be filled by the framework - parambuf += sizeof(uint32_t); - strcpy((char*) parambuf, name); - parambuf += strlen(name) + 1; - strcpy((char*) parambuf, data); - parambuf += strlen(data) + 1; - - // update event's len - evt->len += parambuf - ((uint8_t*) evt + sizeof(ss_plugin_event)); -} - static ss_plugin_rc plugin_set_async_event_handler(ss_plugin_t* s, ss_plugin_owner_t* owner, const ss_plugin_async_event_handler_t handler) { plugin_state *ps = (plugin_state *) s; @@ -179,7 +151,10 @@ static ss_plugin_rc plugin_set_async_event_handler(ss_plugin_t* s, ss_plugin_own for (uint64_t i = 0; i < ps->async_maxevts && ps->async_thread_run; i++) { // attempt sending an event that is not in the allowed name list - encode_async_event(ps->async_evt, 1, "unsupportedname", data); + scap_event_encode_params(scap_sized_buffer{ps->async_evt, sizeof(ps->async_evt_buf)}, nullptr, err, + PPME_ASYNCEVENT_E, 3, 0, "unsupportedname", scap_const_sized_buffer{data, strlen(data) + 1}); + ps->async_evt->tid = 1; + if (SS_PLUGIN_SUCCESS == handler(owner, ps->async_evt, err)) { printf("sample_syscall_async: unexpected success in sending unsupported asynchronous event from plugin\n"); @@ -189,7 +164,10 @@ static ss_plugin_rc plugin_set_async_event_handler(ss_plugin_t* s, ss_plugin_own // send an event in the allowed name list // note: we set a tid=1 to test that async events can have // either an empty (-1) or a non-empty tid value - encode_async_event(ps->async_evt, 1, name, data); + scap_event_encode_params(scap_sized_buffer{ps->async_evt, sizeof(ps->async_evt_buf)}, nullptr, err, + PPME_ASYNCEVENT_E, 3, 0, name, scap_const_sized_buffer{data, strlen(data) + 1}); + ps->async_evt->tid = 1; + if (SS_PLUGIN_SUCCESS != handler(owner, ps->async_evt, err)) { printf("sample_syscall_async: unexpected failure in sending asynchronous event from plugin: %s\n", err); diff --git a/userspace/libsinsp/test/plugins/syscall_source.cpp b/userspace/libsinsp/test/plugins/syscall_source.cpp index fcf67e19fa..9b80d67169 100644 --- a/userspace/libsinsp/test/plugins/syscall_source.cpp +++ b/userspace/libsinsp/test/plugins/syscall_source.cpp @@ -23,6 +23,7 @@ limitations under the License. #include #include "test_plugins.h" +#include /** * Example of plugin implementing only the event sourcing capability, which: @@ -127,43 +128,21 @@ static ss_plugin_rc plugin_next_batch(ss_plugin_t* s, ss_instance_t* i, uint32_t *nevts = 1; *evts = &istate->evt; - istate->evt->type = PPME_SYSCALL_OPEN_X; + + char error[SCAP_LASTERR_SIZE]; + + int32_t encode_res = scap_event_encode_params(scap_sized_buffer{istate->evt, sizeof(istate->evt_buf)}, + nullptr, error, PPME_SYSCALL_OPEN_X, 6, + (uint64_t) 3, "/tmp/the_file", ((1 << 0) | (1 << 1)), 0, 5, (uint64_t) 123); + + if (encode_res == SCAP_FAILURE) + { + return SS_PLUGIN_FAILURE; + } + istate->evt->tid = 1; istate->evt->ts = UINT64_MAX; - istate->evt->len = sizeof(ss_plugin_event); - istate->evt->nparams = 6; - - uint8_t* parambuf = &istate->evt_buf[0] + sizeof(ss_plugin_event); - - // lenghts - *((uint16_t*) parambuf) = sizeof(uint64_t); - parambuf += sizeof(uint16_t); - *((uint16_t*) parambuf) = strlen("/tmp/the_file") + 1; - parambuf += sizeof(uint16_t); - *((uint16_t*) parambuf) = sizeof(uint32_t); - parambuf += sizeof(uint16_t); - *((uint16_t*) parambuf) = sizeof(uint32_t); - parambuf += sizeof(uint16_t); - *((uint16_t*) parambuf) = sizeof(uint32_t); - parambuf += sizeof(uint16_t); - *((uint16_t*) parambuf) = sizeof(uint64_t); - parambuf += sizeof(uint16_t); - - // params - *((uint64_t*) parambuf) = 3; - parambuf += sizeof(uint64_t); - strcpy((char*) parambuf, "/tmp/the_file"); - parambuf += strlen("/tmp/the_file") + 1; - *((uint32_t*) parambuf) = ((1 << 0) | (1 << 1)); - parambuf += sizeof(uint32_t); - *((uint32_t*) parambuf) = 0; - parambuf += sizeof(uint32_t); - *((uint32_t*) parambuf) = 5; - parambuf += sizeof(uint32_t); - *((uint64_t*) parambuf) = 123; - parambuf += sizeof(uint64_t); - - istate->evt->len += parambuf - (&istate->evt_buf[0] + sizeof(ss_plugin_event)); + istate->count--; return SS_PLUGIN_SUCCESS; }