refactor(libsinsp): safer parameter handling part 2 #1502

LucaGuerra · 2023-11-22T17:26:56Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind cleanup

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area libsinsp

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

This PR continues an effort to clean up libsinsp from potential undefined behavior (see: #1470 ). It implements the following refactoring changes:

Added some const correctness around the code that handles sinsp_evt_params. When some raw parameter data is read from the driver it is never modified and so it should be const when processed.
Modified the sinsp_evt class and sinsp_evt_param. Now sinsp_evt_param has more reasons to exist because it is no longer simply a "buffer + length" wrapper but it is able to convert values to appropriate types (with the as() method) and also point back to the original event
Now we can use std::string_view in the template parameter to read string buffers
All uses of get_param(n)<T> are updated to get_param(n)->as<T>
The function get_param_as_str is updated to remove some UB according to the refactoring above

cc @FedeDP @gnosek

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

gnosek

Words can't express how beautiful that is (even if slightly more verbose). Just a few minor comments

gnosek · 2023-11-22T17:44:26Z

userspace/libsinsp/event.h

+	}
+
+	size_t string_len = strnlen(param.m_val, param.m_len);
+	if (param.m_len == string_len)


did you mean != here or is this e.g. a test for the NUL terminator? If so, we could use a comment why the difference compared to the generic version.

(but also, I don't think string_view requires NUL termination, it would defeat the purpose)

It is indeed a NUL terminator check. Sometimes the drivers will output a NUL terminated string and I feel we need a type for that, and that's what I'm using std::string_view for (i.e., this will interpret one parameter that contains a NUL-terminated string).

The resulting string view will not contain the null terminator within its bounds but if you call .data() it will return a NUL-terminated const char*. We wouldn't have this additional requirement if we were using std::string around libsinsp, but many functions right now do ask for a const char*.

This works, but does not feel ideal, I'm open to suggestions.

So we're technically doing an out of bounds read when treating the string_view as a NUL-terminated string (but it's fine because we know there's a NUL just outside), right?

I'm fine with this (this-is-fine.jpg) but:

please add a comment about it

I'd explicitly check param.m_val[param.m_len] == 0 instead (feels more direct) but that does change the behavior for NUL-separated string arrays (which we use in a few places) -- we'd start accepting them as string_views. I assume we don't want them as string views but I could also use a comment to that effect :)

doing an out of bounds read

Indeed we read within the event but outside the string_view which we guarantee to be NUL-terminated just outside. If we were using a NUL-terminated string_view we would have troubles converting to std::string because the terminator would be added to the string itself. Unless you can think of a way of having both right at the same time. The other alternative is to just copy the string.

Anyways I agree with you that the right way of fixing it is to avoid const char* as much as possible and use strings instead.

I have clarified the comment and changed the error condition, I think we simply expect the string to occupy the entire value except for the null terminator, all other cases should be handled differently.

no, no, I do not advocate for NUL-terminated string views, using the NUL just outside it is a great hack in the circumstances we're in ;)

👍

gnosek · 2023-11-22T17:48:05Z

userspace/libsinsp/parsers.cpp


 				/* If the pathname is `<NA>` here we shouldn't have problems during `parse_dirfd`.
 				* It doesn't start with "/" so it is not considered an absolute path.
 				*/
 				std::string sdir;
-				parse_dirfd(evt, pathname, dirfd, &sdir);
+				parse_dirfd(evt, pathname.data(), dirfd, &sdir);


...but for example this call might need a NUL (same for any other use of .data())

way out of scope but I'd love to see more string_views, fewer explicit const char*, size_t pairs (or even worse, raw char*s)

gnosek · 2023-11-22T17:52:03Z

userspace/libsinsp/event.cpp

@@ -2925,3 +2863,20 @@ std::optional<std::reference_wrapper<std::string>> sinsp_evt::get_enter_evt_para

 	return ret;
 }
+
+std::string sinsp_evt_param::invalid_len_error(size_t requested_length) const


for gcc we should be able to add an __attribute((cold)) to prevent inlining in hot paths

I have done some more experimentation. I know this is not pretty but by moving everything including the throw to a separate method we actually get the desired result (inlined _as() function with a call to the exception method, without that sometimes it would not be inlined). I also added the cold attribute although I'm not completely sure if it changes anything.

gnosek · 2023-11-22T17:54:42Z

userspace/libsinsp/event.cpp

 		while(true)
 		{
 			uint32_t blen = binary_buffer_to_string(&m_paramstr_storage[0],
-				payload,
+				param->m_val,


we could consider reintroducing the payload and payload_len variables to reduce the size of the diff, but I'm fine either way

I think I'm good with param->m_val because it feels more like an "immutable" variable coming from somewhere else rather than something you can do clever tricks with (like payload was before)... if this makes sense 😅

gnosek · 2023-11-22T17:56:47Z

userspace/libsinsp/event.h

-
-		return ret;
-	}
+	const sinsp_evt_param* get_param(uint32_t id);


in the vast majority of cases, id is a compile time constant. not sure whether making it a template parameter would improve code generation but maybe it would be worth a try if you can easily test this (run size on a binary before and after the change)

I'm also not sure, but I'm ok with keeping all accesses the same (without the compile time distinction, maybe if we want to experiment a constexpr could be enough for some cases?) for now so we can move on with continuing the saga

userspace/libsinsp/event.cpp

LucaGuerra · 2023-11-23T13:59:07Z

/milestone 0.14.0

userspace/libsinsp/event.cpp

FedeDP

I really love this! Thank you very much Luca!
I only left a question, but this LGTM!

FedeDP · 2023-11-24T16:06:36Z

Uh conflict :) then i'll approve!

Signed-off-by: Luca Guerra <[email protected]>

LucaGuerra · 2023-11-24T16:28:51Z

rebased!

FedeDP

/approve

poiana · 2023-11-24T16:35:36Z

LGTM label has been added.

Git tree hash: 1cae1a479602401684ef7ed34599c63d5745a833

LucaGuerra · 2023-11-24T17:02:28Z

Ah, need to fix a couple of const. I had rebased but not tried the build 😂

... will be back.

Signed-off-by: Luca Guerra <[email protected]>

… inlining Signed-off-by: Luca Guerra <[email protected]>

Signed-off-by: Luca Guerra <[email protected]>

LucaGuerra · 2023-11-24T17:29:43Z

ok, fixed!

FedeDP

/approve

poiana · 2023-11-24T18:05:27Z

LGTM label has been added.

Git tree hash: 93b5501f61951077d13dcd2065c8a34696d83ea6

gnosek

SHIP IT

poiana · 2023-11-27T09:33:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, gnosek, LucaGuerra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,LucaGuerra,gnosek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added release-note-none kind/cleanup dco-signoff: yes area/libsinsp approved labels Nov 22, 2023

poiana requested review from gnosek and hbrueckner November 22, 2023 17:27

poiana added the size/XXL label Nov 22, 2023

gnosek reviewed Nov 22, 2023

View reviewed changes

LucaGuerra force-pushed the refactor-param-get-part-2 branch from bd64fbd to c187c55 Compare November 23, 2023 09:30

poiana added this to the 0.14.0 milestone Nov 23, 2023

LucaGuerra mentioned this pull request Nov 23, 2023

fix(userspace/libsinsp): remove warning in scanf on uint64_t #1505

Merged

LucaGuerra force-pushed the refactor-param-get-part-2 branch 2 times, most recently from fe238ce to 29dbd45 Compare November 24, 2023 11:23

FedeDP reviewed Nov 24, 2023

View reviewed changes

userspace/libsinsp/event.cpp Outdated Show resolved Hide resolved

FedeDP reviewed Nov 24, 2023

View reviewed changes

LucaGuerra force-pushed the refactor-param-get-part-2 branch from 64a3075 to 166887f Compare November 24, 2023 15:42

LucaGuerra added 2 commits November 24, 2023 16:25

refactor(libsinsp): get_param_value_raw -> get_param_by_name

b6409c4

Signed-off-by: Luca Guerra <[email protected]>

refactor(libsinsp): const correctness for sinsp_evt_param

e52c8b4

Signed-off-by: Luca Guerra <[email protected]>

LucaGuerra force-pushed the refactor-param-get-part-2 branch from 166887f to 0f16eb2 Compare November 24, 2023 16:26

FedeDP previously approved these changes Nov 24, 2023

View reviewed changes

poiana assigned FedeDP Nov 24, 2023

poiana added the lgtm label Nov 24, 2023

LucaGuerra added 2 commits November 24, 2023 17:13

refactor(libsinsp): simplify evt param, enforce const correctness

f18923a

Signed-off-by: Luca Guerra <[email protected]>

refactor(libsinsp): use param->as<>() to interpret params

4367d31

Signed-off-by: Luca Guerra <[email protected]>

LucaGuerra added 3 commits November 24, 2023 17:13

refactor(libsinsp): update get_param_as_str to remove potential ub

ce8b8a1

Signed-off-by: Luca Guerra <[email protected]>

update(libsinsp): move exception to a separate function to facilitate…

5e7822d

… inlining Signed-off-by: Luca Guerra <[email protected]>

refactor(libsinsp): clarify comments and string conditions

5e155ed

Signed-off-by: Luca Guerra <[email protected]>

LucaGuerra dismissed FedeDP’s stale review via 5e155ed November 24, 2023 17:13

LucaGuerra force-pushed the refactor-param-get-part-2 branch from 0f16eb2 to 5e155ed Compare November 24, 2023 17:13

poiana removed the lgtm label Nov 24, 2023

poiana requested a review from FedeDP November 24, 2023 17:13

FedeDP approved these changes Nov 24, 2023

View reviewed changes

poiana added the lgtm label Nov 24, 2023

gnosek approved these changes Nov 27, 2023

View reviewed changes

poiana assigned gnosek Nov 27, 2023

poiana merged commit a894e67 into falcosecurity:master Nov 27, 2023

LucaGuerra deleted the refactor-param-get-part-2 branch November 27, 2023 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(libsinsp): safer parameter handling part 2 #1502

refactor(libsinsp): safer parameter handling part 2 #1502

LucaGuerra commented Nov 22, 2023 •

edited

Loading

gnosek left a comment

gnosek Nov 22, 2023

LucaGuerra Nov 23, 2023

gnosek Nov 24, 2023

LucaGuerra Nov 24, 2023 •

edited

Loading

gnosek Nov 24, 2023

gnosek Nov 22, 2023

gnosek Nov 22, 2023

LucaGuerra Nov 24, 2023

gnosek Nov 22, 2023

LucaGuerra Nov 24, 2023

gnosek Nov 22, 2023

LucaGuerra Nov 24, 2023

LucaGuerra commented Nov 23, 2023

FedeDP left a comment •

edited

Loading

FedeDP commented Nov 24, 2023

LucaGuerra commented Nov 24, 2023

FedeDP left a comment

poiana commented Nov 24, 2023

LucaGuerra commented Nov 24, 2023

LucaGuerra commented Nov 24, 2023

FedeDP left a comment

poiana commented Nov 24, 2023

gnosek left a comment

poiana commented Nov 27, 2023

refactor(libsinsp): safer parameter handling part 2 #1502

refactor(libsinsp): safer parameter handling part 2 #1502

Conversation

LucaGuerra commented Nov 22, 2023 • edited Loading

gnosek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaGuerra Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaGuerra commented Nov 23, 2023

FedeDP left a comment • edited Loading

Choose a reason for hiding this comment

FedeDP commented Nov 24, 2023

LucaGuerra commented Nov 24, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Nov 24, 2023

LucaGuerra commented Nov 24, 2023

LucaGuerra commented Nov 24, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Nov 24, 2023

gnosek left a comment

Choose a reason for hiding this comment

poiana commented Nov 27, 2023

LucaGuerra commented Nov 22, 2023 •

edited

Loading

LucaGuerra Nov 24, 2023 •

edited

Loading

FedeDP left a comment •

edited

Loading