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

refactor(libsinsp): rewrite concatenate_paths with std::filesystem #1533

Merged
merged 10 commits into from
Dec 12, 2023
Merged

refactor(libsinsp): rewrite concatenate_paths with std::filesystem #1533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c8d86eb
refactor(libsinsp): rewrite concatenate_paths with std::filesystem
LucaGuerra Dec 1, 2023
6c129a3
fix(libsinp): correct new concatenate_paths for openat* parsing
incertum Dec 2, 2023
35f3b60
fix(libsinp): correct new concatenate_paths for set_cwd
incertum Dec 2, 2023
31d4a9f
cleanup(test): adjust new concatenate_paths test
incertum Dec 2, 2023
20b253f
cleanup(libsinsp): apply reviewers suggestions
incertum Dec 6, 2023
61a0f9a
cleanup(libsinsp): apply reviewers suggestions
incertum Dec 6, 2023
1d03301
fix(libsinsp): adjust g_invalidchar to allow emojis, cyrillic, chines…
incertum Dec 10, 2023
640d945
fix(libsinsp): update concatenate_paths signature, avoid ssize_t and …
LucaGuerra Dec 11, 2023
13cd8a2
refactor(libsinsp): use a std::string_view in sinsp_threadinfo::set_cwd
LucaGuerra Dec 11, 2023
217bcee
update(libsinsp): add windows path workaround
LucaGuerra Dec 12, 2023
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
13 changes: 7 additions & 6 deletions userspace/libsinsp/event.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ limitations under the License.
#include <string>
#include <optional>
#include <functional>
#include <filesystem>

#include "sinsp.h"
#include "sinsp_int.h"
Expand Down Expand Up @@ -1039,15 +1040,15 @@ const char* sinsp_evt::get_param_as_str(uint32_t id, OUT const char** resolved_s
m_resolved_paramstr_storage.resize(path.length() + cwd.length() + 2, 0);
}

if(!sinsp_utils::concatenate_paths(&m_resolved_paramstr_storage[0],
(uint32_t)m_resolved_paramstr_storage.size(),
(char*)cwd.c_str(),
(uint32_t)cwd.length(),
path.data(),
path.length()))
if(path.empty() || std::filesystem::path(path).is_absolute())
{
m_resolved_paramstr_storage[0] = 0;
}
else
{
std::string concatenated_path = sinsp_utils::concatenate_paths(cwd, path);
strcpy_sanitized(&m_paramstr_storage[0], concatenated_path.data(), std::min(concatenated_path.size() + 1, m_paramstr_storage.size()));
}
}
}
else
Expand Down
61 changes: 18 additions & 43 deletions userspace/libsinsp/parsers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2247,7 +2247,7 @@ void sinsp_parser::parse_execve_exit(sinsp_evt *evt)
&&
retrieve_enter_event(enter_evt, evt))
{
char fullpath[SCAP_MAX_PATH_SIZE] = {0};
std::string fullpath;

/* We need to manage the 2 possible cases:
* - enter event is an `EXECVE`
Expand All @@ -2259,24 +2259,20 @@ void sinsp_parser::parse_execve_exit(sinsp_evt *evt)
/*
* Get filename
*/
parinfo = enter_evt->get_param(0);
std::string_view filename = enter_evt->get_param(0)->as<std::string_view>();
/* This could happen only if we are not able to get the info from the kernel,
* because if the syscall was successful the pathname was surely here the problem
* is that for some reason we were not able to get it with our instrumentation,
* for example when the `bpf_probe_read()` call fails in BPF.
*/
if(strncmp(parinfo->m_val, "<NA>", 5) == 0)
if(filename == "<NA>")
{
strlcpy(fullpath, "<NA>", 5);
fullpath = "<NA>";
}
else
{
/* Here the filename can be relative or absolute. */
sinsp_utils::concatenate_paths(fullpath, SCAP_MAX_PATH_SIZE,
evt->m_tinfo->m_cwd.c_str(),
(uint32_t)evt->m_tinfo->m_cwd.size(),
parinfo->m_val,
(uint32_t)parinfo->m_len);
fullpath = sinsp_utils::concatenate_paths(evt->m_tinfo->m_cwd, filename);
}
}
else if(enter_evt->get_type() == PPME_SYSCALL_EXECVEAT_E)
Expand Down Expand Up @@ -2322,35 +2318,24 @@ void sinsp_parser::parse_execve_exit(sinsp_evt *evt)
sdir.compare("<UNKNOWN>") == 0)
{
/* we copy also the string terminator `\0`. */
strlcpy(fullpath, "<NA>", 5);
fullpath = "<NA>";
}
/* (3) In this case we have already obtained the `exepath` and it is `sdir`, we just need
* to sanitize it.
*/
else if(flags & PPM_EXVAT_AT_EMPTY_PATH)
{
/* We explicitly set the `pathlen` to `0`, since `pathname` is `<NA>`
* as we said in case (3), and we don't want to consider it as a valid
* part of the final path. In this case `sdir` will always be
* an absolute path.
/* In this case `sdir` will always be an absolute path.
*/
sinsp_utils::concatenate_paths(fullpath, SCAP_MAX_PATH_SIZE,
"\0",
0,
sdir.c_str(),
(uint32_t)sdir.length());
fullpath = sinsp_utils::concatenate_paths("", sdir);

}
/* (2)/(1) If it is relative or absolute we craft the `fullpath` as usual:
* - `sdir` + `pathname`
*/
else
{
sinsp_utils::concatenate_paths(fullpath, SCAP_MAX_PATH_SIZE,
sdir.c_str(),
(uint32_t)sdir.length(),
pathname.data(),
pathname.size());
fullpath = sinsp_utils::concatenate_paths(sdir, pathname);
}
}
evt->m_tinfo->m_exepath = fullpath;
Expand Down Expand Up @@ -2766,10 +2751,7 @@ void sinsp_parser::parse_open_openat_creat_exit(sinsp_evt *evt)
//ASSERT(parinfo->m_len == sizeof(uint32_t));
//mode = *(uint32_t*)parinfo->m_val;

char fullpath[SCAP_MAX_PATH_SIZE];

sinsp_utils::concatenate_paths(fullpath, SCAP_MAX_PATH_SIZE, sdir.c_str(), (uint32_t)sdir.length(),
name.data(), name.size());
std::string fullpath = sinsp_utils::concatenate_paths(sdir, name);

if(fd >= 0)
{
Expand All @@ -2790,7 +2772,7 @@ void sinsp_parser::parse_open_openat_creat_exit(sinsp_evt *evt)
fdi.m_dev = dev;
fdi.m_ino = ino;
fdi.add_filename_raw(name.data());
fdi.add_filename(fullpath);
fdi.add_filename(fullpath.c_str());

//
// Add the fd to the table.
Expand Down Expand Up @@ -4409,11 +4391,8 @@ void sinsp_parser::parse_chdir_exit(sinsp_evt *evt)
//
if(retval >= 0)
{
const sinsp_evt_param *parinfo;

// Update the thread working directory
parinfo = evt->get_param(1);
evt->m_tinfo->set_cwd(parinfo->m_val, parinfo->m_len);
evt->m_tinfo->set_cwd(evt->get_param(1)->as<std::string_view>());
}
}

Expand All @@ -4440,14 +4419,12 @@ void sinsp_parser::parse_fchdir_exit(sinsp_evt *evt)
}

// Update the thread working directory
evt->m_tinfo->set_cwd((char *)evt->m_fdinfo->m_name.c_str(),
(uint32_t)evt->m_fdinfo->m_name.size());
evt->m_tinfo->set_cwd(evt->m_fdinfo->m_name);
}
}

void sinsp_parser::parse_getcwd_exit(sinsp_evt *evt)
{
const sinsp_evt_param *parinfo;
int64_t retval;

//
Expand All @@ -4470,14 +4447,12 @@ void sinsp_parser::parse_getcwd_exit(sinsp_evt *evt)
return;
}

parinfo = evt->get_param(1);
std::string cwd = std::string(evt->get_param(1)->as<std::string_view>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: the type name need not be repeated. Just noticed, but it can be left as it is.

		std::string cwd(evt->get_param(1)->as<std::string_view>());


#ifdef _DEBUG
std::string chkstr = std::string(parinfo->m_val);

if(chkstr != "/")
if(cwd != "/")
{
if(chkstr + "/" != evt->m_tinfo->get_cwd())
if(cwd + "/" != evt->m_tinfo->get_cwd())
{
//
// This shouldn't happen, because we should be able to stay in synch by
Expand All @@ -4488,7 +4463,7 @@ void sinsp_parser::parse_getcwd_exit(sinsp_evt *evt)
#ifdef _DEBUG
int target_res;
char target_name[1024];
target_res = readlink((chkstr + "/").c_str(),
target_res = readlink((cwd + "/").c_str(),
target_name,
sizeof(target_name) - 1);

Expand All @@ -4508,7 +4483,7 @@ void sinsp_parser::parse_getcwd_exit(sinsp_evt *evt)
}
#endif

evt->m_tinfo->set_cwd(parinfo->m_val, parinfo->m_len);
evt->m_tinfo->set_cwd(cwd);
}
}

Expand Down
17 changes: 4 additions & 13 deletions userspace/libsinsp/sinsp_filtercheck_event.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,8 +439,7 @@ uint8_t* extract_argraw(sinsp_evt *evt, OUT uint32_t* len, const char *argname)

uint8_t *sinsp_filter_check_event::extract_abspath(sinsp_evt *evt, OUT uint32_t *len)
{
const sinsp_evt_param *parinfo;
string spath;
std::string spath;

if(evt->m_tinfo == NULL)
{
Expand Down Expand Up @@ -476,7 +475,7 @@ uint8_t *sinsp_filter_check_event::extract_abspath(sinsp_evt *evt, OUT uint32_t
else if(etype == PPME_SYSCALL_OPEN_BY_HANDLE_AT_X)
{
int fd = 0;
char fullname[SCAP_MAX_PATH_SIZE];
std::string fullname;

//
// We can extract the file path only in case of a successful file opening (fd>0).
Expand All @@ -488,12 +487,8 @@ uint8_t *sinsp_filter_check_event::extract_abspath(sinsp_evt *evt, OUT uint32_t
//
// Get the file path directly from the ring buffer.
//
parinfo = evt->get_param(3);
m_strstorage = sinsp_utils::concatenate_paths("", evt->get_param(3)->as<std::string_view>());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why concatenate when one item is an empty path? Is it because the function enforces a final slash? If so, wouldn't it be better to put the slash when the path is finally used for its purpose? I know this logic was already there, but it seems a lot of work to check the path at any point it is manipulated when it could be done only once at the final step. End of rant.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also does the directory traversal if applicable. We wouldn't need this here? for this one perhaps let's do a 2 step, first get this one in, before risking breaking things and take the time to check more.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was only curious. I understand this would be a larger change.


sinsp_utils::concatenate_paths(fullname, SCAP_MAX_PATH_SIZE, "", 0,
parinfo->m_val, parinfo->m_len);

m_strstorage = fullname;
RETURN_EXTRACT_STRING(m_strstorage);
}
}
Expand Down Expand Up @@ -598,11 +593,7 @@ uint8_t *sinsp_filter_check_event::extract_abspath(sinsp_evt *evt, OUT uint32_t
}
}

char fullname[SCAP_MAX_PATH_SIZE];
sinsp_utils::concatenate_paths(fullname, SCAP_MAX_PATH_SIZE, sdir.c_str(),
(uint32_t)sdir.length(), path.data(), path.size());

m_strstorage = fullname;
m_strstorage = sinsp_utils::concatenate_paths(sdir, path);

RETURN_EXTRACT_STRING(m_strstorage);
}
Expand Down
23 changes: 6 additions & 17 deletions userspace/libsinsp/sinsp_filtercheck_fd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,7 @@ bool sinsp_filter_check_fd::extract_fdname_from_creator(sinsp_evt *evt, OUT uint
{
sinsp_evt enter_evt;
const sinsp_evt_param *parinfo;
const char *name;
uint32_t namelen;
string sdir;
std::string sdir;

if(etype == PPME_SYSCALL_OPENAT_X)
{
Expand All @@ -232,30 +230,21 @@ bool sinsp_filter_check_fd::extract_fdname_from_creator(sinsp_evt *evt, OUT uint
}

parinfo = etype == PPME_SYSCALL_OPENAT_X ? enter_evt.get_param(1) : evt->get_param(2);
name = parinfo->m_val;
namelen = parinfo->m_len;
std::string_view name = parinfo->as<std::string_view>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Perhaps we can avoid repeating the type in this kind of definitions as it is already on the RHS. Just a thought for the future. Leave it as it is for the moment.

			auto name = parinfo->as<std::string_view>();


parinfo = etype == PPME_SYSCALL_OPENAT_X ? enter_evt.get_param(0) : evt->get_param(1);
ASSERT(parinfo->m_len == sizeof(int64_t));
int64_t dirfd = *(int64_t *)parinfo->m_val;
int64_t dirfd = parinfo->as<int64_t>();

sinsp_parser::parse_dirfd(evt, name, dirfd, &sdir);

char fullpath[SCAP_MAX_PATH_SIZE];

sinsp_utils::concatenate_paths(fullpath, SCAP_MAX_PATH_SIZE,
sdir.c_str(),
(uint32_t)sdir.length(),
name,
namelen);
sinsp_parser::parse_dirfd(evt, name.data(), dirfd, &sdir);

if(fd_nameraw)
{
m_tstr = name;
}
else
{
m_tstr = fullpath;
// fullpath
m_tstr = sinsp_utils::concatenate_paths(sdir, name); // here we'd like a string
}

if(sanitize_strings)
Expand Down
4 changes: 1 addition & 3 deletions userspace/libsinsp/sinsp_filtercheck_fspath.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ limitations under the License.
*/

#include <filesystem>
#include "sinsp_filtercheck_fspath.h"
#include "sinsp_filtercheck_event.h"
#include "sinsp_filtercheck_fd.h"
Expand Down Expand Up @@ -350,8 +349,7 @@ uint8_t* sinsp_filter_check_fspath::extract(sinsp_evt* evt, OUT uint32_t* len, b
return NULL;
}

std::filesystem::path tstr = tinfo->get_cwd() + m_tstr;
m_tstr = std::filesystem::absolute(tstr).lexically_normal().string();
m_tstr = sinsp_utils::concatenate_paths(tinfo->get_cwd(), m_tstr);
}

// If m_tstr ends in a c-style \0, remove it to be
Expand Down
Loading