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

fix(userspace/libsinsp): multiple fixes related to rawargs. #2130

Merged
merged 1 commit into from
Oct 30, 2024
+80 −6
Merged

fix(userspace/libsinsp): multiple fixes related to rawargs. #2130

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
22 changes: 19 additions & 3 deletions userspace/libsinsp/sinsp_filtercheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@

case PT_L4PROTO: // This can be resolved in the future
case PT_UINT8:
case PT_FLAGS8:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We did not support FLAGS and ENUMFLAGS in rawval_to_{json,string}.

case PT_ENUMFLAGS8:

Check warning on line 143 in userspace/libsinsp/sinsp_filtercheck.cpp

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/sinsp_filtercheck.cpp#L142-L143 

Added lines #L142 - L143 were not covered by tests
if(print_format == PF_DEC || print_format == PF_ID) {
return *(uint8_t*)rawval;
} else if(print_format == PF_OCT || print_format == PF_HEX) {
Expand All @@ -150,6 +152,8 @@

case PT_PORT: // This can be resolved in the future
case PT_UINT16:
case PT_FLAGS16:
case PT_ENUMFLAGS16:

Check warning on line 156 in userspace/libsinsp/sinsp_filtercheck.cpp

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/sinsp_filtercheck.cpp#L155-L156 

Added lines #L155 - L156 were not covered by tests
if(print_format == PF_DEC || print_format == PF_ID) {
return *(uint16_t*)rawval;
} else if(print_format == PF_OCT || print_format == PF_HEX) {
Expand All @@ -160,6 +164,10 @@
}

case PT_UINT32:
case PT_FLAGS32:
case PT_ENUMFLAGS32:
case PT_UID:
case PT_GID:

Check warning on line 170 in userspace/libsinsp/sinsp_filtercheck.cpp

View check run for this annotation

Codecov / codecov/patch

userspace/libsinsp/sinsp_filtercheck.cpp#L167-L170 

Added lines #L167 - L170 were not covered by tests
if(print_format == PF_DEC || print_format == PF_ID) {
return *(uint32_t*)rawval;
} else if(print_format == PF_OCT || print_format == PF_HEX) {
Expand Down Expand Up @@ -292,12 +300,14 @@
return m_getpropertystr_storage.data();
case PT_L4PROTO: // This can be resolved in the future
case PT_UINT8:
case PT_FLAGS8:
case PT_ENUMFLAGS8:
if(print_format == PF_OCT) {
prfmt = (char*)"%" PRIo8;
} else if(print_format == PF_DEC || print_format == PF_ID) {
prfmt = (char*)"%" PRIu8;
} else if(print_format == PF_HEX) {
prfmt = (char*)"%" PRIu8;
prfmt = (char*)"%" PRIX8;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix print format for 8,16,32bits types for PF_HEX.

} else {
ASSERT(false);
return NULL;
Expand All @@ -311,12 +321,14 @@
return m_getpropertystr_storage.data();
case PT_PORT: // This can be resolved in the future
case PT_UINT16:
case PT_FLAGS16:
case PT_ENUMFLAGS16:
if(print_format == PF_OCT) {
prfmt = (char*)"%" PRIo16;
} else if(print_format == PF_DEC || print_format == PF_ID) {
prfmt = (char*)"%" PRIu16;
} else if(print_format == PF_HEX) {
prfmt = (char*)"%" PRIu16;
prfmt = (char*)"%" PRIX16;
} else {
ASSERT(false);
return NULL;
Expand All @@ -329,12 +341,16 @@
rawval_cast<uint16_t>(rawval));
return m_getpropertystr_storage.data();
case PT_UINT32:
case PT_FLAGS32:
case PT_ENUMFLAGS32:
case PT_UID:
case PT_GID:
if(print_format == PF_OCT) {
prfmt = (char*)"%" PRIo32;
} else if(print_format == PF_DEC || print_format == PF_ID) {
prfmt = (char*)"%" PRIu32;
} else if(print_format == PF_HEX) {
prfmt = (char*)"%" PRIu32;
prfmt = (char*)"%" PRIX32;
} else {
ASSERT(false);
return NULL;
Expand Down
18 changes: 15 additions & 3 deletions userspace/libsinsp/sinsp_filtercheck_event.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,13 @@ int32_t sinsp_filter_check_event::parse_field_name(std::string_view val,

res = extract_arg("evt.rawarg", val, &m_arginfo);

// These fields will be overridden with correct ones from the current event
// while extracting from event.
// But we also need to set them at init time to something different than `PT_DYN`
// to allow `eval_filter` expressions to be properly compiled.
// Note: even if they'll be overridden,
// it is safe to guess that eg: `size` param will always be a numeric type.
// Of course if we had a `size` param that is string, it would be troublesome.
Comment on lines +557 to +558
Copy link
Member

@Andreagit97 Andreagit97 Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i would just remove this since we have already found a corner case (addr). Maybe we could list here all the cases we are aware of, for example for now addr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep i am afraid that we forgot once again 😆

m_customfield.m_type = m_arginfo->type;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per-comment: store the first one found to allow proper compilation of filters on this field; this works fine unless we have an event param, eg: flags, that in an event is int type, and in another is string type.
That's not the case right now.

m_customfield.m_print_format = m_arginfo->fmt;
} else if(STR_MATCH("evt.around")) {
Expand Down Expand Up @@ -650,10 +657,15 @@ void sinsp_filter_check_event::validate_filter_value(const char* str, uint32_t l
}
}

uint8_t* extract_argraw(sinsp_evt* evt, uint32_t* len, const char* argname) {
const sinsp_evt_param* pi = evt->get_param_by_name(argname);
uint8_t* sinsp_filter_check_event::extract_argraw(sinsp_evt* evt,
uint32_t* len,
const char* argname) {
auto pi = evt->get_param_by_name(argname);

if(pi != NULL) {
m_arginfo = pi->get_info();
m_customfield.m_type = m_arginfo->type;
Copy link
Contributor Author

@FedeDP FedeDP Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Store correct field type and print format from the being-extracted event.

m_customfield.m_print_format = m_arginfo->fmt;
*len = pi->m_len;
return (uint8_t*)pi->m_val;
} else {
Expand Down Expand Up @@ -1192,7 +1204,7 @@ uint8_t* sinsp_filter_check_event::extract_single(sinsp_evt* evt,
m_val.u16 = evt->get_cpuid();
RETURN_EXTRACT_VAR(m_val.u16);
case TYPE_ARGRAW:
return extract_argraw(evt, len, m_arginfo->name);
return extract_argraw(evt, len, m_argname.c_str());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use m_argname here that is exactly the same as m_arginfo->name.

break;
case TYPE_ARGSTR: {
const char* resolved_argstr;
Expand Down
1 change: 1 addition & 0 deletions userspace/libsinsp/sinsp_filtercheck_event.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class sinsp_filter_check_event : public sinsp_filter_check {
uint8_t* extract_error_count(sinsp_evt* evt, uint32_t* len);
uint8_t* extract_abspath(sinsp_evt* evt, uint32_t* len);
inline uint8_t* extract_buflen(sinsp_evt* evt, uint32_t* len);
uint8_t* extract_argraw(sinsp_evt* evt, uint32_t* len, const char* argname);

union {
uint16_t u16;
Expand Down
45 changes: 45 additions & 0 deletions userspace/libsinsp/test/filterchecks/evt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,48 @@ TEST_F(sinsp_with_test_input, EVT_FILTER_check_evt_arg_uid) {
ASSERT_EQ(get_field_as_string(evt, "evt.arg[0]"), "<NA>");
ASSERT_EQ(get_field_as_string(evt, "evt.args"), "uid=5(<NA>)");
}

// Test that for rawarg.X we are correctly retrieving the correct field type/format.
TEST_F(sinsp_with_test_input, rawarg_madness) {
add_default_init_thread();
open_inspector();

// [PPME_SYSCALL_EPOLL_CREATE_E] = {"epoll_create", EC_WAIT | EC_SYSCALL, EF_CREATES_FD |
// EF_MODIFIES_STATE, 1, { {"size", PT_INT32, PF_DEC} } },
sinsp_evt* evt =
add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_EPOLL_CREATE_E, 1, (int32_t)-22);
ASSERT_EQ(get_field_as_string(evt, "evt.rawarg.size"), "-22");
ASSERT_TRUE(eval_filter(evt, "evt.rawarg.size < -20"));

// [PPME_SYSCALL_SIGNALFD4_X] = {"signalfd4", EC_SIGNAL | EC_SYSCALL, EF_CREATES_FD |
// EF_MODIFIES_STATE, 2, {{"res", PT_FD, PF_DEC}, {"flags", PT_FLAGS16, PF_HEX, file_flags}}},
evt = add_event_advance_ts(increasing_ts(),
1,
PPME_SYSCALL_SIGNALFD4_X,
2,
(int64_t)-1,
(uint16_t)512);
// 512 in hex is 200
ASSERT_EQ(get_field_as_string(evt, "evt.rawarg.flags"), "200");
ASSERT_TRUE(eval_filter(evt, "evt.rawarg.flags < 515"));

// [PPME_SYSCALL_TIMERFD_CREATE_E] = {"timerfd_create",EC_TIME | EC_SYSCALL,EF_CREATES_FD |
// EF_MODIFIES_STATE,2,{{"clockid", PT_UINT8, PF_DEC},{"flags", PT_UINT8, PF_HEX}}},
evt = add_event_advance_ts(increasing_ts(),
1,
PPME_SYSCALL_TIMERFD_CREATE_E,
2,
(uint8_t)-1,
(uint8_t)255);
// 255 in hex is FF
ASSERT_EQ(get_field_as_string(evt, "evt.rawarg.flags"), "FF");
ASSERT_TRUE(eval_filter(evt, "evt.rawarg.flags <= 255"));

// [PPME_SYSCALL_BRK_4_E] = {"brk", EC_MEMORY | EC_SYSCALL, EF_NONE, 1, {{"addr", PT_UINT64,
// PF_HEX}}}
uint64_t addr = UINT64_MAX;
evt = add_event_advance_ts(increasing_ts(), 1, PPME_SYSCALL_BRK_4_E, 1, addr);
// UINT64_MAX is FFFFFFFFFFFFFFFF
ASSERT_EQ(get_field_as_string(evt, "evt.rawarg.addr"), "FFFFFFFFFFFFFFFF");
ASSERT_ANY_THROW(eval_filter(evt, "evt.rawarg.addr > 0")); // PT_SOCKADDR is not comparable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue is still present: even f the addr arg for brk event is an uint64_t, the initial filter arginfo is set to the first addr found in the event_table, ie: PPME_SOCKET_BIND_X that has a PT_SOCKADDR addr argument.
PT_SOCKADDR is not a comparable type (see filter_compare.cpp flt_is_comparable function) therefore we are not able to compile any filter against it.
NOTE: this was an issue already present on master.

Copy link
Member

Choose a reason for hiding this comment

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

yep not sure what to do here, one thing could be to rename some parameters to avoid the issue. We should reach a point where params with the same name have the same type, but yes this would be a breaking change...

}
Loading