fix(userspace/libsinsp): revert to old `concatenate_paths` helper funtion for perf reasons #1645

FedeDP · 2024-01-25T16:02:11Z

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area libsinsp

Does this PR require a change in the driver versions?

What this PR does / why we need it:

This PR embeds a better fix for the issue found in #1530, reverting the new std::filesystem implementation (that was great btw, and much better edge cases coverage; see #1533 ) to the old one with some small fixes on top, for perf reasons. We found that the new impl had like a 7.5x slowdown; since it is used in hot paths, we had to revert.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

FedeDP · 2024-01-25T16:02:58Z

/cc @LucaGuerra @incertum @leogr

FedeDP · 2024-01-25T16:03:16Z

userspace/libsinsp/test/sinsp_utils.ut.cpp

Added and adapted tests.

FedeDP · 2024-01-25T16:03:34Z

userspace/libsinsp/utils.cpp

-#endif // _WIN32
+	(*tc)--;
+
+	while((*tc) >= targetbase + 1 && *((*tc) - 1) != '/')


Inverted check to avoid possible underflow.

We discussed that this is a better inverted check than https://github.com/falcosecurity/libs/pull/1530/files#diff-3a461a173cc85eaeb297c9d4c40fa1c58c0c3ab4556b3757ebdfe262de19d45cR611

... alongside other improvements. It was all tested w/ a fuzzer to the best of our abilities ❤️

FedeDP · 2024-01-25T16:04:05Z

userspace/libsinsp/utils.cpp

+				//   Example: "/foo/../a" -> "/a" BUT "foo/../a" -> "a"
+				//   -> Otherwise: "foo/../a" -> "/a"
+				//
+				if((tc > targetbase && *(tc - 1) == separator) || (tc == targetbase && !empty_base))


When cutting away ../ in the target string, we return to its start, avoid putting a /; this fixes edge cases like:

"foo/../a"

that previously returned /a instead of just a.
This case is now covered by tests.

FedeDP · 2024-01-25T16:05:47Z

Even if the diff is big, it should be seen as a (relatively small) patch against old code, ie: https://raw.githubusercontent.com/falcosecurity/libs/aa191bab837e9a7ab58bf0547addaad26aa7f05e/userspace/libsinsp/utils.cpp

incertum

/approve

Amazing work @FedeDP and @LucaGuerra !!!!

incertum · 2024-01-25T16:24:23Z

userspace/libsinsp/utils.cpp

-#endif // _WIN32
+	(*tc)--;
+
+	while((*tc) >= targetbase + 1 && *((*tc) - 1) != '/')


We discussed that this is a better inverted check than https://github.com/falcosecurity/libs/pull/1530/files#diff-3a461a173cc85eaeb297c9d4c40fa1c58c0c3ab4556b3757ebdfe262de19d45cR611

... alongside other improvements. It was all tested w/ a fuzzer to the best of our abilities ❤️

leogr · 2024-01-25T17:00:34Z

userspace/libsinsp/test/sinsp_utils.ut.cpp

@@ -17,13 +17,19 @@ limitations under the License.
 */

 #include <gtest/gtest.h>
-#include <libsinsp/utils.h>
+#include "utils.h"


Suggested change

#include "utils.h"

#include <libsinsp/utils.h>

This shouldn't be reverted. See 6f8cb47#diff-362102dc7b939ad80b6a97c6e36cb1c610f314172ebb33b9b1483b062e53c81d

cc @therealbobo

We should really add a CI to avoid them slipping in :/

leogr · 2024-01-25T17:01:30Z

/milestone 0.14.2

…ction for perf reasons. Also, added some small fixes and lots of new tests. Signed-off-by: Federico Di Pierro <[email protected]>

LucaGuerra · 2024-01-26T08:40:17Z

Adding context to this:

The invalid memory access identified by Melissa in [We are refactoring and getting rid of the custom logic, see PR 1533] fix(libsinsp): avoid possible underflow in rewind_to_parent_path util #1530 is not a vulnerability but definitely needs to be fixed, this PR takes care of it
Thanks to @deepskyblue86 we were able to measure performance of all approaches. Considering that this function has very few checks compared to a full fledged path concatenation implementation, better performance are expected

Honestly it's sad to see the cleaner implementation go, but maybe not everything is lost. Surely, this solution works well for this upcoming release. In the process we fuzzed both the old and new implementation and no memory errors were detected in this patch.

poiana · 2024-01-26T09:00:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, leogr, 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,incertum,leogr]

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

incertum · 2024-01-28T00:03:42Z

Just now got to test this a bit more and I noticed a regression for example for a UEK 4.14 kernel e.g. https://yum.oracle.com/repo/OracleLinux/OL7/UEKR5/x86_64/getPackage/ -> any should do e.g. kernel-uek-4.14.35-*.el7uek.x86_64.rpm

Logs now don't contain the file names anymore. Note this issue seems a corner case as I have seen it work on many kernels. Carefully verified that this commit is the root cause (did a cherry-pit on it).

{"container.id":"host","container.name":"host","evt.args":"fd=3(<d>/home/vagrant/<NA>) name=<NA> flags=13489(O_DIRECTORY|O_SYNC|O_EXCL|O_DSYNC|O_RDONLY|O_CLOEXEC|O_TMPFILE) mode=03164 dev=803 ino=68477313 ","evt.time":1706391799055670838,"fd.name":"/home/vagrant/<NA>","fs.path.name":"/home/vagrant/<NA>","proc.cmdline":"cat /etc/shadow","proc.exepath":"/usr/bin/cat","proc.name":"cat","proc.pname":"sudo","proc.sname":"bash","proc.tty":34817,"user.name":"root"}}

FedeDP · 2024-01-29T08:23:16Z

I just added a test case:

path1 = "/home/vagrant/";
path2 = "kernel-uek-4.14.35-*.el7uek.x86_64.rpm";
res = sinsp_utils::concatenate_paths(path1, path2);
EXPECT_EQ("/home/vagrant/kernel-uek-4.14.35-*.el7uek.x86_64.rpm", res);

and it is passing fine. Am i missing anything?

FedeDP · 2024-01-29T08:25:02Z

Also, i don't see how an userspace (small) patch could be affected by the running kernel.
Are you able to test on the same setup with the 0.13.0 version (ie: the original algorithm we used before #1533)?

FedeDP · 2024-01-29T16:12:32Z

Am i missing anything?

I was missing the actual bug 😆

incertum · 2024-01-29T20:00:41Z

@FedeDP de-escalating. I destroyed everything and kicked off new CI builds for both versions as well and now the gremlins disappeared. Sorry about that, but figured we are so close to the release I should at least share this observation as it was very unexpected and weird to me.

poiana added release-note-none kind/bug Something isn't working dco-signoff: yes area/libsinsp size/L labels Jan 25, 2024

poiana requested review from Andreagit97 and hbrueckner January 25, 2024 16:02

poiana added the approved label Jan 25, 2024

poiana requested review from incertum, leogr and LucaGuerra January 25, 2024 16:02

FedeDP commented Jan 25, 2024

View reviewed changes

userspace/libsinsp/test/sinsp_utils.ut.cpp

Copy link

Contributor Author

FedeDP Jan 25, 2024

Choose a reason for hiding this comment

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

Added and adapted tests.

FedeDP commented Jan 25, 2024

View reviewed changes

incertum previously approved these changes Jan 25, 2024

View reviewed changes

poiana assigned incertum Jan 25, 2024

poiana added the lgtm label Jan 25, 2024

leogr reviewed Jan 25, 2024

View reviewed changes

poiana added this to the 0.14.2 milestone Jan 25, 2024

fix(userspace/libsinsp): revert to old concatenate_paths helper fun…

8af72bd

…ction for perf reasons. Also, added some small fixes and lots of new tests. Signed-off-by: Federico Di Pierro <[email protected]>

FedeDP dismissed incertum’s stale review via 8af72bd January 26, 2024 07:46

FedeDP force-pushed the fix/revert_concatenate_paths branch from 18f3bc8 to 8af72bd Compare January 26, 2024 07:46

LucaGuerra approved these changes Jan 26, 2024

View reviewed changes

poiana assigned LucaGuerra Jan 26, 2024

leogr approved these changes Jan 26, 2024

View reviewed changes

poiana assigned leogr Jan 26, 2024

poiana merged commit 7f329c9 into master Jan 26, 2024
33 checks passed

poiana deleted the fix/revert_concatenate_paths branch January 26, 2024 09:01

FedeDP mentioned this pull request Jan 26, 2024

sync: cherry pick for 0.14.2 #1646

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(userspace/libsinsp): revert to old `concatenate_paths` helper funtion for perf reasons #1645

fix(userspace/libsinsp): revert to old `concatenate_paths` helper funtion for perf reasons #1645

FedeDP commented Jan 25, 2024 •

edited

Loading

FedeDP commented Jan 25, 2024

FedeDP Jan 25, 2024

FedeDP Jan 25, 2024

incertum Jan 25, 2024

FedeDP Jan 25, 2024 •

edited

Loading

FedeDP commented Jan 25, 2024 •

edited

Loading

incertum left a comment

incertum Jan 25, 2024

leogr Jan 25, 2024

FedeDP Jan 26, 2024

FedeDP Jan 26, 2024

leogr commented Jan 25, 2024

LucaGuerra commented Jan 26, 2024

poiana commented Jan 26, 2024

incertum commented Jan 28, 2024

FedeDP commented Jan 29, 2024

FedeDP commented Jan 29, 2024

FedeDP commented Jan 29, 2024

incertum commented Jan 29, 2024

fix(userspace/libsinsp): revert to old concatenate_paths helper funtion for perf reasons #1645

fix(userspace/libsinsp): revert to old concatenate_paths helper funtion for perf reasons #1645

Conversation

FedeDP commented Jan 25, 2024 • edited Loading

FedeDP commented Jan 25, 2024

FedeDP Jan 25, 2024

Choose a reason for hiding this comment

FedeDP Jan 25, 2024

Choose a reason for hiding this comment

incertum Jan 25, 2024

Choose a reason for hiding this comment

FedeDP Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

FedeDP commented Jan 25, 2024 • edited Loading

incertum left a comment

Choose a reason for hiding this comment

incertum Jan 25, 2024

Choose a reason for hiding this comment

leogr Jan 25, 2024

Choose a reason for hiding this comment

FedeDP Jan 26, 2024

Choose a reason for hiding this comment

FedeDP Jan 26, 2024

Choose a reason for hiding this comment

leogr commented Jan 25, 2024

LucaGuerra commented Jan 26, 2024

poiana commented Jan 26, 2024

incertum commented Jan 28, 2024

FedeDP commented Jan 29, 2024

FedeDP commented Jan 29, 2024

FedeDP commented Jan 29, 2024

incertum commented Jan 29, 2024

fix(userspace/libsinsp): revert to old `concatenate_paths` helper funtion for perf reasons #1645

fix(userspace/libsinsp): revert to old `concatenate_paths` helper funtion for perf reasons #1645

FedeDP commented Jan 25, 2024 •

edited

Loading

FedeDP Jan 25, 2024 •

edited

Loading

FedeDP commented Jan 25, 2024 •

edited

Loading