fix: send enter events also with scap files not only in live captures

test/libscap/test_suites/userspace/event_table.cpp

@@ -190,3 +190,27 @@ TEST(event_table, check_exit_param_names) {
 		}
 	}
 }
+
+// todo!: revisit this test after we remove the enter event support in sinsp
+TEST(event_table, check_EF_USED_FD) {
+	for(int evt = 0; evt < PPM_EVENT_MAX; evt++) {
+		auto evt_info = scap_get_event_info_table()[evt];
+		if((evt_info.flags & EF_USES_FD) == 0) {
+			continue;
+		}
+
+		if(PPME_IS_ENTER(evt)) {
+			int location = get_enter_event_fd_location((ppm_event_code)evt);
+			ASSERT_EQ(evt_info.params[location].type, PT_FD)
+			        << "event_type " << evt << " uses a wrong location " << location;
+		}
+
+		if(PPME_IS_EXIT(evt) && evt_info.flags & EF_TMP_CONVERTER_MANAGED) {
+			int location = get_exit_event_fd_location((ppm_event_code)evt);
+			ASSERT_NE(location, -1)
+			        << "event_type " << evt << " uses a wrong location " << location;
+			ASSERT_EQ(evt_info.params[location].type, PT_FD)
+			        << "event_type " << evt << " uses a wrong location " << location;
+		}
+	}
+}

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

@@ -317,6 +317,10 @@ static conversion_result convert_event(scap_evt *new_evt,
 	uint16_t params_offset = 0;
 	int param_to_populate = 0;
 
+	// We copy the entire event in any case so that we are ready to handle `CONVERSION_SKIP` cases
+	// without further actions.
+	memcpy(new_evt, evt_to_convert, evt_to_convert->len);
+
 	switch(ci.m_action) {
 	case C_ACTION_SKIP:
 		return CONVERSION_SKIP;
@@ -326,15 +330,13 @@ static conversion_result convert_event(scap_evt *new_evt,
 		return CONVERSION_SKIP;
 
 	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.m_instrs.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.m_instrs.size();
 		new_evt->type = ci.m_desired_type;
@@ -355,8 +357,6 @@ static conversion_result convert_event(scap_evt *new_evt,
 	PRINT_EVENT(new_evt, PRINT_HEADER);
 
 	scap_evt *tmp_evt = NULL;
-	// If this is true at the end of the for loop we will free its memory.
-	bool used_enter_event = false;
 
 	// We iterate over the instructions
 	for(int i = 0; i < ci.m_instrs.size(); i++, param_to_populate++) {
@@ -390,7 +390,6 @@ static conversion_result convert_event(scap_evt *new_evt,
 				         get_direction_char((ppm_event_code)tmp_evt->type));
 				return CONVERSION_ERROR;
 			}
-			used_enter_event = true;
 			break;
 
 		case C_INSTR_FROM_OLD:
@@ -429,11 +428,10 @@ static conversion_result convert_event(scap_evt *new_evt,
 		}
 	}
 
-	if(used_enter_event) {
-		// We can free the enter event because we don't need it anymore.
+	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);
 	}
-
 	new_evt->len = params_offset;
 
 	PRINT_MESSAGE("Final event:\n");

userspace/libscap/engine/savefile/scap_savefile.c

@@ -2027,7 +2027,6 @@ static int32_t next(struct scap_engine_handle engine,
                     uint16_t *pdevid,
                     uint32_t *pflags) {
 	struct savefile_engine *handle = engine.m_handle;
-read_event:;
 	int32_t res = next_event_from_file(handle, pevent, pdevid, pflags);
 	// If we fail we don't convert the event.
 	if(res != SCAP_SUCCESS) {
@@ -2065,10 +2064,8 @@ read_event:;
 	case CONVERSION_ERROR:
 		return SCAP_FAILURE;
 
+	// today with CONVERSION_SKIP we send the event to userspace, tomorrow we could drop it.
 	case CONVERSION_SKIP:
-		// Probably an enter event that we don't want to consider. So we read another event.
-		goto read_event;
-
 	case CONVERSION_COMPLETED:
 	case CONVERSION_CONTINUE:
 	default:

userspace/libscap/scap.h

@@ -874,7 +874,8 @@ typedef enum scap_print_info {
 	PRINT_FULL,
 } scap_print_info;
 void scap_print_event(scap_evt* ev, scap_print_info i);
-
+int get_enter_event_fd_location(ppm_event_code etype);
+int get_exit_event_fd_location(ppm_event_code etype);
 /*@}*/
 
 ///////////////////////////////////////////////////////////////////////////////

userspace/libscap/scap_event.c

@@ -496,3 +496,45 @@ scap_evt *scap_create_event(char *error,
 	va_end(args);
 	return ret;
 }
+
+// Only enter events have a convention on the fd parameter position.
+// Should be always the first parameter apart from some exceptions.
+int get_enter_event_fd_location(ppm_event_code etype) {
+	ASSERT(etype < PPM_EVENT_MAX);
+	ASSERT(PPME_IS_ENTER(etype));
+	ASSERT(scap_get_event_info_table()[etype].flags & EF_USES_FD);
+
+	// For almost all parameters the default position is `0`
+	int location = 0;
+	switch(etype) {
+	case PPME_SYSCALL_MMAP_E:
+	case PPME_SYSCALL_MMAP2_E:
+		location = 4;
+		break;
+	case PPME_SYSCALL_SPLICE_E:
+		location = 1;
+		break;
+	default:
+		break;
+	}
+	return location;
+}
+
+// In the exit events we don't have a precise convension on the fd parameter position.
+int get_exit_event_fd_location(ppm_event_code etype) {
+	ASSERT(etype < PPM_EVENT_MAX);
+	ASSERT(PPME_IS_EXIT(etype));
+	ASSERT(scap_get_event_info_table()[etype].flags & EF_USES_FD);
+
+	// we want to return -1 as location if we forgot to handle something
+	int location = -1;
+	switch(etype) {
+	case PPME_SYSCALL_READ_X:
+	case PPME_SYSCALL_PREAD_X:
+		location = 2;
+		break;
+	default:
+		break;
+	}
+	return location;
+}

userspace/libsinsp/event.h

@@ -769,6 +769,10 @@ class SINSP_PUBLIC sinsp_evt {
 		}
 	}
 
+	inline bool uses_fd() const { return get_info_flags() & EF_USES_FD; }
+
+	inline bool creates_fd() const { return get_info_flags() & EF_CREATES_FD; }
+
 private:
 	sinsp* m_inspector;
 	scap_evt* m_pevt;

userspace/libsinsp/parsers.cpp

@@ -116,12 +116,6 @@ void sinsp_parser::process_event(sinsp_evt *evt) {
 	case PPME_SYSCALL_EXECVEAT_E:
 		store_event(evt);
 		break;
-	case PPME_SYSCALL_WRITE_E:
-		if(!m_inspector->is_dumping() && evt->get_tinfo() != nullptr) {
-			// note(jasondellaluce): this may be useless now that we removed tracers support
-			evt->set_fd_info(evt->get_tinfo()->get_fd(evt->get_tinfo()->m_lastevent_fd));
-		}
-		break;
 	case PPME_SYSCALL_MKDIR_X:
 	case PPME_SYSCALL_RMDIR_X:
 	case PPME_SYSCALL_LINK_X:
@@ -586,17 +580,19 @@ bool sinsp_parser::reset(sinsp_evt *evt) {
 		evt->get_tinfo()->m_flags |= PPM_CL_ACTIVE;
 	}
 
+	// todo!: at the end of we work we should remove this if/else and ideally we should set the
+	// fdinfos directly here and return if they are not present
 	if(PPME_IS_ENTER(etype)) {
 		evt->get_tinfo()->m_lastevent_fd = -1;
 		evt->get_tinfo()->set_lastevent_type(etype);
 
-		if(eflags & EF_USES_FD) {
+		if(evt->uses_fd()) {
 			//
 			// Get the fd.
 			// An fd will usually be the first parameter of the enter event,
 			// but there are exceptions, as is the case with mmap, mmap2
 			//
-			int fd_location = get_fd_location(etype);
+			int fd_location = get_enter_event_fd_location((ppm_event_code)etype);
 			ASSERT(evt->get_param_info(fd_location)->type == PT_FD);
 			evt->get_tinfo()->m_lastevent_fd = evt->get_param(fd_location)->as<int64_t>();
 			evt->set_fd_info(evt->get_tinfo()->get_fd(evt->get_tinfo()->m_lastevent_fd));
@@ -622,10 +618,10 @@ bool sinsp_parser::reset(sinsp_evt *evt) {
 			tinfo->set_lastevent_data_validity(true);
 		} else {
 			tinfo->set_lastevent_data_validity(false);
-
-			if(tinfo->get_lastevent_type() != PPME_TRACER_E) {
-				return false;
-			}
+			// We cannot be sure that the lastevent_fd is something valid, it could be the socket of
+			// the previous `socket` syscall or it could be something completely unrelated, for now
+			// we don't trust it in any case.
+			tinfo->m_lastevent_fd = -1;
 		}
 
 		//
@@ -642,7 +638,7 @@ bool sinsp_parser::reset(sinsp_evt *evt) {
 		//
 		// Retrieve the fd
 		//
-		if(eflags & EF_USES_FD) {
+		if(evt->uses_fd()) {
 			//
 			// The copy_file_range syscall has the peculiarity of using two fds
 			// Set as m_lastevent_fd the output fd
@@ -651,6 +647,13 @@ bool sinsp_parser::reset(sinsp_evt *evt) {
 				tinfo->m_lastevent_fd = evt->get_param(1)->as<int64_t>();
 			}
 
+			// todo!: this should become the unique logic when we'll disable the enter events.
+			if(tinfo->m_lastevent_fd == -1) {
+				int fd_location = get_exit_event_fd_location((ppm_event_code)etype);
+				if(fd_location != -1) {
+					tinfo->m_lastevent_fd = evt->get_param(fd_location)->as<int64_t>();
+				}
+			}
 			evt->set_fd_info(tinfo->get_fd(tinfo->m_lastevent_fd));
 
 			if(evt->get_fd_info() == NULL) {
@@ -5259,17 +5262,3 @@ void sinsp_parser::parse_pidfd_getfd_exit(sinsp_evt *evt) {
 	}
 	evt->get_tinfo()->add_fd(fd, targetfd_fdinfo->clone());
 }
-
-int sinsp_parser::get_fd_location(uint16_t etype) {
-	int location;
-	switch(etype) {
-	case PPME_SYSCALL_MMAP_E:
-	case PPME_SYSCALL_MMAP2_E:
-		location = 4;
-		break;
-	default:
-		location = 0;
-		break;
-	}
-	return location;
-}

userspace/libsinsp/parsers.h

@@ -151,7 +151,6 @@ class sinsp_parser {
 	void swap_addresses(sinsp_fdinfo* fdinfo);
 	uint8_t* reserve_event_buffer();
 	void free_event_buffer(uint8_t*);
-	inline int get_fd_location(uint16_t etype);
 
 	//
 	// Pointers to inspector context

userspace/libsinsp/test/events_net.ut.cpp

@@ -806,7 +806,6 @@ TEST_F(sinsp_with_test_input, net_connect_enter_event_is_missing_wo_fd_param_exi
 	add_default_init_thread();
 	open_inspector();
 	sinsp_evt* evt = NULL;
-	sinsp_fdinfo* fdinfo = NULL;
 	int64_t client_fd = 7;
 
 	add_event_advance_ts(increasing_ts(),
@@ -830,7 +829,9 @@ TEST_F(sinsp_with_test_input, net_connect_enter_event_is_missing_wo_fd_param_exi
 
 	/* We dropped connect enter! */
 
-	/* We read an old scap file with a connect exit event with just 2 params (no fd!) */
+	/* todo!: revisit this when we will manage CONNECT_X in scap files.
+	 * We simulate an event from an old scap file, a connect exit event with just 2 params (no fd!)
+	 */
 	std::vector<uint8_t> socktuple =
 	        test_utils::pack_socktuple(reinterpret_cast<sockaddr*>(&client),
 	                                   reinterpret_cast<sockaddr*>(&server));
@@ -841,8 +842,13 @@ TEST_F(sinsp_with_test_input, net_connect_enter_event_is_missing_wo_fd_param_exi
 	                           return_value,
 	                           scap_const_sized_buffer{socktuple.data(), socktuple.size()});
 
+	/* We cannot recover the file descriptor from the enter event neither from the exit event */
+	ASSERT_EQ(evt->get_fd_info(), nullptr);
+
+	/* We recover this from the tuple */
+	ASSERT_EQ(get_field_as_string(evt, "fd.name"), "80.9.11.45:12->152.40.111.222:25632");
+
 	/* Check that we are not able to load any info */
-	ASSERT_EQ(get_field_as_string(evt, "fd.name"), "");
 	ASSERT_FALSE(field_has_value(evt, "fd.sip"));
 	ASSERT_FALSE(field_has_value(evt, "fd.cip"));
 	ASSERT_FALSE(field_has_value(evt, "fd.rip"));
@@ -851,9 +857,4 @@ TEST_F(sinsp_with_test_input, net_connect_enter_event_is_missing_wo_fd_param_exi
 	ASSERT_FALSE(field_has_value(evt, "fd.sport"));
 	ASSERT_FALSE(field_has_value(evt, "fd.lport"));
 	ASSERT_FALSE(field_has_value(evt, "fd.rport"));
-
-	/* The parser is not able to obtain an updated fdname because the syscall fails and the parser
-	 * flow is truncated */
-	fdinfo = evt->get_fd_info();
-	ASSERT_EQ(fdinfo, nullptr);
 }