cleanup: improve EF_USES_FD event handling

Signed-off-by: Andrea Terzolo <[email protected]>

test/libscap/test_suites/userspace/event_table.cpp

@@ -167,3 +167,27 @@ TEST(event_table, check_usage_of_EC_UNKNOWN_flag) {
 		}
 	}
 }
+
+// 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_NE(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/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,48 @@ 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);
+	ASSERT(scap_get_event_info_table()[etype].flags & EF_TMP_CONVERTER_MANAGED);
+
+	// 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_WRITE_X:
+	case PPME_SYSCALL_PREAD_X:
+	case PPME_SYSCALL_PWRITE_X:
+		location = 2;
+		break;
+	default:
+		break;
+	}
+	return location;
+}

userspace/libsinsp/event.h

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

userspace/libsinsp/parsers.cpp

@@ -581,18 +581,17 @@ 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);
-			ASSERT(evt->get_param_info(fd_location)->type == PT_FD);
+			int fd_location = get_enter_event_fd_location((ppm_event_code)etype);
 			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));
 		}
@@ -645,7 +644,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
@@ -654,6 +653,8 @@ bool sinsp_parser::reset(sinsp_evt *evt) {
 				tinfo->m_lastevent_fd = evt->get_param(1)->as<int64_t>();
 			}
 
+			// todo!: This is ok until we have enter events because `m_lastevent_fd` should be
+			// populated by them. We must modify the logic when we will have only exit events.
 			evt->set_fd_info(tinfo->get_fd(tinfo->m_lastevent_fd));
 
 			if(evt->get_fd_info() == NULL) {
@@ -5257,17 +5258,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