Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleanup: remove some extra code #2186

Merged
merged 1 commit into from
Dec 6, 2024
Merged

cleanup: remove some extra code #2186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions test/libscap/test_suites/userspace/event_table.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,3 +167,26 @@ TEST(event_table, check_usage_of_EC_UNKNOWN_flag) {
}
}
}

TEST(event_table, check_exit_param_names) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this test; do we really need to enforce the param name from now on, given your changes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should enforce the rule that the first parameter in the exit event is the return value of the syscall. Unfortunately, I've not found a great way to do that so for now we just monitor the parameters' names.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a best effort check, if the name matches we are quite confident that the first parameter is a return value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this already cached the weird case of `PPME_GENERIC_X'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah i see, makes sense.
But then we are not using the check on param name at runtime anymore, so this is just to avoid that in the future a new syscall exit event is added whose first param is not a return code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly, today we are quite confident the rule is valid, this test tries to prevent future issues

// We should use only res/fd but we have all these other variants
std::set<std::string> valid_names = {"res", "fd", "uid", "gid", "res_or_fd", "euid", "egid"};
for(int evt = 0; evt < PPM_EVENT_MAX; evt++) {
auto evt_info = scap_get_event_info_table()[evt];

// Generic is an exit event but it does not have the return code.
if(evt == PPME_GENERIC_X) {
continue;
}

if((evt_info.category & EC_SYSCALL) && PPME_IS_EXIT(evt) && evt_info.nparams > 0) {
const char* name = evt_info.params[0].name;
if(valid_names.find(name) != valid_names.end()) {
continue;
} else {
FAIL() << "Evt: '" << evt_info.name
<< "' has a first param name not allowed: " << name;
}
}
}
}
14 changes: 11 additions & 3 deletions userspace/libsinsp/event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,9 +722,17 @@ class SINSP_PUBLIC sinsp_evt {
inline bool is_syscall_event() const { return get_info()->category & EC_SYSCALL; }

inline bool has_return_value() {
// The event has a return value:
// * if it is a syscall event and it is an exit event.
if(is_syscall_event() && PPME_IS_EXIT(get_type())) {
// This event does not have a return value
if(get_type() == PPME_GENERIC_X) {
return false;
}

// The event has a return value if:
// * it is a syscall event.
// * it is an exit event.
// * it has at least one parameter. Some exit events are not instrumented, see
// `PPME_SOCKET_GETSOCKNAME_X`
if(is_syscall_event() && PPME_IS_EXIT(get_type()) && get_num_params() > 0) {
return true;
}

Expand Down
18 changes: 5 additions & 13 deletions userspace/libsinsp/parsers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,19 +632,11 @@ bool sinsp_parser::reset(sinsp_evt *evt) {
//
// Error detection logic
//
if(evt->get_num_params() != 0 && ((evt->get_info()->params[0].name[0] == 'r' &&
evt->get_info()->params[0].name[1] == 'e' &&
evt->get_info()->params[0].name[2] == 's' &&
evt->get_info()->params[0].name[3] == '\0') ||
(evt->get_info()->params[0].name[0] == 'f' &&
evt->get_info()->params[0].name[1] == 'd' &&
evt->get_info()->params[0].name[2] == '\0'))) {
if(evt->has_return_value()) {
int64_t res = evt->get_syscall_return_value();

if(res < 0) {
evt->set_errorcode(-(int32_t)res);
}
if(evt->has_return_value()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i don't miss something all the syscall exit event should have at least the return value so i don't see why we need the additional manual check on the parameter names

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I encountered this code right yesterday and was wondering exactly the same 🤣

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this logic was broken since there are all these possible values "uid", "gid", "res_or_fd", "euid", "egid"

int64_t res = evt->get_syscall_return_value();

if(res < 0) {
evt->set_errorcode(-(int32_t)res);
}
}

Expand Down
Loading