fix(plugin): fix and clarify extract offsets docs/tests

The docs (and a unit test) specified `value_offsets` to be an array
of `ss_plugin_extract_value_offsets` structs, while the code in
plugin_filtercheck.cpp expected it to be a struct of arrays.
Things worked out only because we never extract multiple fields
in one go (at least in libsinsp itself).

Keep the plugin_filtercheck.cpp behavior and adapt the documentation
and tests to match.

Additionally, clarify that the offsets are counted from the start
of the event buffer (including the header).

Signed-off-by: Grzegorz Nosek <[email protected]>

Author: gnosek

userspace/libsinsp/test/plugins.ut.cpp

@@ -376,8 +376,30 @@ TEST_F(sinsp_with_test_input, plugin_custom_source) {
 	ASSERT_FALSE(field_has_value(evt, "fd.name", filterlist));
 	ASSERT_EQ(get_field_as_string(evt, "evt.pluginname", filterlist), src_pl->name());
 	ASSERT_EQ(get_field_as_string(evt, "sample.hello", filterlist), "hello world");
-	ASSERT_EQ(get_value_offset_start(evt, "sample.hello", filterlist, 0), 0);
-	ASSERT_EQ(get_value_offset_length(evt, "sample.hello", filterlist, 0), 11);
+
+	auto offset = get_value_offset_start(evt, "sample.hello", filterlist, 0);
+	ASSERT_EQ(offset, PLUGIN_EVENT_PAYLOAD_OFFSET);
+	auto length = get_value_offset_length(evt, "sample.hello", filterlist, 0);
+	ASSERT_EQ(length, 11);
+
+	const auto raw_evt = reinterpret_cast<const char*>(evt->get_scap_evt());
+	for(int i = 0; i < evt->get_scap_evt()->len; i++) {
+		printf("%02x ", (unsigned char)raw_evt[i]);
+	}
+	printf("\n");
+	for(int i = 0; i < evt->get_scap_evt()->len; i++) {
+		char c = ' ';
+		if(i >= offset && i < offset + length) {
+			c = '^';
+		}
+		printf("%c%c%c", c, c, c);
+	}
+	printf("\n");
+
+	// Note: here we are asserting that the exact bytes are present in the event payload
+	// This does not need to hold for all plugins (e.g. due to serialization format)
+	// but is a useful check here to validate we got the offsets correct.
+	ASSERT_EQ(memcmp(raw_evt + offset, "hello world", 11), 0);
 	ASSERT_EQ(next_event(), nullptr);  // EOF is expected
 }
 

userspace/libsinsp/test/plugins/plugin_extract.cpp

@@ -35,6 +35,8 @@ namespace {
 struct plugin_state {
 	std::string lasterr;
 	std::string strstorage;
+	std::vector<uint32_t> offsets;
+	std::vector<uint32_t> lengths;
 	const char* strptr;
 	std::vector<uint16_t> event_types;
 	ss_plugin_owner_t* owner;
@@ -130,6 +132,12 @@ ss_plugin_rc plugin_extract_fields(ss_plugin_t* s,
                                    const ss_plugin_event_input* ev,
                                    const ss_plugin_field_extract_input* in) {
 	auto ps = reinterpret_cast<plugin_state*>(s);
+
+	if(in->value_offsets) {
+		ps->offsets.resize(in->num_fields);
+		ps->lengths.resize(in->num_fields);
+	}
+
 	for(uint32_t i = 0; i < in->num_fields; i++) {
 		switch(in->fields[i].field_id) {
 		case 0:  // test.hello
@@ -141,15 +149,19 @@ ss_plugin_rc plugin_extract_fields(ss_plugin_t* s,
 			in->fields[i].res.str = &ps->strptr;
 			in->fields[i].res_len = 1;
 			if(in->value_offsets) {
-				in->value_offsets[i].start = &res_start;
-				in->value_offsets[i].length = &res_length;
+				ps->offsets[i] = PLUGIN_EVENT_PAYLOAD_OFFSET + res_start;
+				ps->lengths[i] = res_length;
 			}
 		} break;
 		default:
 			in->fields[i].res_len = 0;
 			return SS_PLUGIN_FAILURE;
 		}
 	}
+	if(in->value_offsets) {
+		in->value_offsets->start = ps->offsets.data();
+		in->value_offsets->length = ps->lengths.data();
+	}
 	return SS_PLUGIN_SUCCESS;
 }
 

userspace/plugin/plugin_api.h

@@ -46,6 +46,15 @@ extern "C" {
 //
 #define PLUGIN_MAX_ERRLEN 1024
 
+// The offset in the event where a plugin event payload starts
+// Since the event payload is at a fixed offset, you can add this value
+// to the start of an extracted field within the payload to get the offset
+// from the start of the event.
+//
+// 26 bytes for the event header, plus 2*4 bytes for the parameter lengths,
+// plus 4 bytes for the plugin ID.
+#define PLUGIN_EVENT_PAYLOAD_OFFSET 38
+
 // Supported by the API but deprecated. Use the extended version ss_plugin_table_reader_vtable_ext
 // instead. todo(jasondellaluce): when/if major changes to v4, remove this and give this name to the
 // associated *_ext struct.
@@ -371,9 +380,13 @@ typedef struct ss_plugin_field_extract_input {
 	// Vtable for controlling a state table for read operations.
 	ss_plugin_table_reader_vtable_ext* table_reader_ext;
 
-	// An array of ss_plugin_extract_value_offsets structs. The start
-	// and length in each entry should be set to nullptr, and as with
-	// the "fields" member, memory pointers set as output must be
+	// An optional ss_plugin_extract_value_offsets struct. If set, the plugin
+	// is expected to allocate two arrays of size num_fields, one for
+	// the offsets and one for the lengths of the extracted values.
+	// The offsets are counted starting from the beginning of the event
+	// (including the header), and the lengths are the number of bytes
+	// that the extracted value occupies in the event data.
+	// As with the "fields" member, memory pointers set as output must be
 	// allocated by the plugin and must not be deallocated or modified
 	// until the next extract_fields() call.
 	// This member is optional, and might be ignored by extractors.

userspace/plugin/plugin_types.h

@@ -130,11 +130,11 @@ typedef struct ss_plugin_byte_buffer {
 
 // Used in extract_fields_and_offsets to receive field value offsets
 // along with field data.
-// Extraction functions that support offsets should be set these to an
+// Extraction functions that support offsets should set these to an
 // array of zero-indexed start offsets and lengths of each returned
-// value in the event or log data. {0, 0} can be used to indicate that
-// there are no valid offsets, e.g. if the value was generated or
-// computed from other data.
+// value in the event or log data, counting from the start of the event
+// header. {0, 0} can be used to indicate that there are no valid offsets,
+// e.g. if the value was generated or computed from other data.
 // Extraction functions might not support offsets. In order to detect
 // this, callers should initialize the start and length to nullptr.
 typedef struct ss_plugin_extract_value_offsets {