fix(src,cmake,go-worker): multiple fixes. #6
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FedeDP
commented
Dec 3, 2024
.github/workflows/ci.yml
Outdated
| - name: Install deps | ||
| run: | ||
| sudo apt-get install -y --no-install-recommends libbtrfs-dev libgpgme-dev | ||
| sudo apt-get install -y --no-install-recommends libbtrfs-dev |
There was a problem hiding this comment.
Only dep left, but is compile time only.
FedeDP
commented
Dec 3, 2024
| @@ -70,46 +70,49 @@ void from_json(const nlohmann::json& j, container_port_mapping& port) { | |||
| } | |||
|
|
|||
| void from_json(const nlohmann::json& j, std::shared_ptr<container_info>& cinfo) { | |||
| std::shared_ptr<container_info> info = std::make_shared<container_info>(); | |||
There was a problem hiding this comment.
Fixed cinfo filling to avoid dereferencing null ptr.
FedeDP
commented
Dec 3, 2024
| info->m_host_ipc = container.value("host_ipc", false); | ||
| info->m_host_network = container.value("host_network", false); | ||
| info->m_host_pid = container.value("host_pid", false); | ||
| info->m_container_ip = container.value("ip", ""); |
There was a problem hiding this comment.
Fixed default value for m_container_ip that is actually a string.
FedeDP
commented
Dec 3, 2024
src/container_type.h
Outdated
FedeDP
commented
Dec 3, 2024
| int32_t(evt_reader.get_type())); | ||
| return false; | ||
| } | ||
| auto thread_id = evt_reader.get_tid(); |
There was a problem hiding this comment.
thread_id is actually an uint64_t, no need to check for <= 0.
FedeDP
commented
Dec 3, 2024
| SPDLOG_ERROR("cannot extract the container_id for the thread id '{}': {}", | ||
| } catch(falcosecurity::plugin_exception e) { | ||
| // Debug here since many events do not have thread id info (eg: schedswitch) | ||
| SPDLOG_DEBUG("cannot extract the container_id for the thread id '{}': {}", |
There was a problem hiding this comment.
Just a debug here: there can be events (eg: schedswitch) that do not have any threadinfo associated.
FedeDP
commented
Dec 3, 2024
| @@ -138,15 +135,18 @@ std::string my_plugin::compute_container_id_for_thread(const falcosecurity::tabl | |||
| tr, | |||
| [&](const falcosecurity::table_entry& e) | |||
| { | |||
| if (!container_id.empty()) { | |||
There was a problem hiding this comment.
I thought that returning false would stop the loop, instead it raised an exception 💀
FedeDP
commented
Dec 3, 2024
| } | ||
| return true; | ||
| } catch (falcosecurity::plugin_exception &e) { | ||
| SPDLOG_ERROR("cannot attach container_id to new process event for the thread id '{}': {}", |
There was a problem hiding this comment.
This is actually an error since we are parsing a process event (ie: execve, clone and so on), therefore thread_id MUST be present.
FedeDP
commented
Dec 3, 2024
| @@ -4,6 +4,7 @@ | |||
| "license": "MIT", | |||
| "dependencies": [ | |||
| "re2", | |||
| "spdlog" | |||
| "spdlog", | |||
| "gpgme" | |||
First of all, podman had a runtime dep on `gpgme` (`libgpgme-dev`). Install it through vcpkg (as static) to avoid linking issues for the library consumer. Second, in go-worker Container struct, omit empty structured fields (slices, maps...), to avoid wasting memory space (plus all the back and forth copies). Finally, fixed some bugs all around the C++ implementation. Signed-off-by: Federico Di Pierro <[email protected]>
FedeDP
force-pushed
the
fix/multiple_fixes
branch
from
0809359 to
178eb89
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First of all, podman had a runtime dep on
gpgme(libgpgme-dev). Install it through vcpkg (as static) to avoid linking issues for the library consumer.Second, in go-worker Container struct, omit empty structured fields (slices, maps...), to avoid wasting memory space (plus all the back and forth copies).
Finally, fixed some bugs all around the C++ implementation.
All of these were spotted thanks to the newly added possibility to load plugins in sinsp-example: falcosecurity/libs#2179.