Fix crash in execve filename parameter parsing - #98
Open
Stringy wants to merge 4 commits into
Open
Conversation
5 tasks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe invalid parameter length path now logs raw event diagnostics before throwing. Auxiliary-map storage now uses task-based LRU entries, initializes missing entries from a template, sizes the map for multiple tasks, and releases entries after submission. ChangesInvalid parameter length diagnostics
Task-scoped auxiliary maps
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Event parameter data can have a recorded length that doesn't match the null-terminated string length. This has been observed on some kernels for both string and integer parameters (e.g. clone3 exe param_len=5 vs strnlen=1, clone flags param_len=597 vs expected 4). Use the full parameter length instead of throwing sinsp_exception, which propagated uncaught through sinsp::next() and crashed the collector via std::unexpected() -> abort().
Stringy
force-pushed
the
giles/fix-execve-filename-crash
branch
from
August 5, 2026 12:11
9d74644 to
50ee906
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@userspace/libsinsp/event.h`:
- Around line 216-223: Update the tests around invalid_string_len and
advance_ts_get_event so an embedded-NUL length mismatch is accepted and
processed successfully instead of expecting an exception. Preserve coverage for
inputs that remain invalid by adding a separate test for a missing terminator or
zero recorded length, using the existing event-parameter test helpers and
assertions.
- Around line 216-223: The PT_CHARBUF formatting path around the
parameter-length fallback must not pass a non-NUL-terminated buffer via s.data()
to %s. Update the formatting logic used by event.cpp to use a length-aware
formatter with the string view’s size, or reject the no-NUL case before
formatting, while preserving normal NUL-terminated string handling.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5d55e7b2-af52-4d9a-afb6-6fbd85956912
📒 Files selected for processing (1)
userspace/libsinsp/event.h
Comment on lines
+216
to
+223
| // We expect the parameter to be exactly one null-terminated string. | ||
| // When it doesn't match, use the full parameter length instead. | ||
| // Event parameter data can have a recorded length that doesn't match | ||
| // the null-terminated string length. This has been observed on some | ||
| // kernels for both string and integer parameters (e.g. clone3 exe | ||
| // param_len=5 vs strnlen=1, clone flags param_len=597 vs expected 4). | ||
| if(param_len != string_len + 1) { | ||
| // By moving this error string building operation to a separate function | ||
| // the compiler is more likely to inline this entire function. | ||
| param.throw_invalid_len_error(string_len + 1); | ||
| string_len = param_len - 1; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update invalid_string_len for the new contract.
userspace/libsinsp/test/events_param.ut.cpp Lines 515-539 corrupt an embedded NUL and expect advance_ts_get_event to throw. This branch now accepts that mismatch, so the test still asserts the removed exception path. Change it to assert successful processing. Add a separate test for missing terminators or zero recorded lengths if those inputs remain invalid.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@userspace/libsinsp/event.h` around lines 216 - 223, Update the tests around
invalid_string_len and advance_ts_get_event so an embedded-NUL length mismatch
is accepted and processed successfully instead of expecting an exception.
Preserve coverage for inputs that remain invalid by adding a separate test for a
missing terminator or zero recorded length, using the existing event-parameter
test helpers and assertions.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 'strnlen|param_len|get_event_param_as' userspace/libsinsp/event.h
rg -n -C 8 'as<std::string_view>|snprintf.*%s' userspace/libsinsp/event.cppRepository: stackrox/falcosecurity-libs
Length of output: 9343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- event.h relevant declarations and implementation ---'
ast-grep outline userspace/libsinsp/event.h
sed -n '70,185p' userspace/libsinsp/event.h
sed -n '189,243p' userspace/libsinsp/event.h
echo '--- data_and_len_with_legacy_null_encoding definitions/usages ---'
rg -n -C 12 'data_and_len_with_legacy_null_encoding|sinsp_evt_param::sinsp_evt_param|m_data|m_len' userspace/libsinsp userspace/libscap test 2>/dev/null | head -n 320
echo '--- formatter callers and parameter types ---'
sed -n '820,900p' userspace/libsinsp/event.cpp
rg -n -C 8 'PT_CHARBUF|as<std::string_view>|as<std::string>' userspace/libsinsp userspace/libscap test 2>/dev/null | head -n 360
echo '--- tests around mismatched lengths ---'
sed -n '470,560p' userspace/libsinsp/test/events_param.ut.cppRepository: stackrox/falcosecurity-libs
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- event parameter construction and loading ---'
rg -n -C 18 'sinsp_evt_param\(|m_params\.|load_params|get_param\(' userspace/libsinsp/event.h userspace/libsinsp/event.cpp userspace/libsinsp/*.cpp | head -n 420
echo '--- raw event/scap length definitions and parser code ---'
rg -n -C 16 'PPM_EVENT_HEADER|len.*param|param.*len|scap_evt|nparams|parse.*param|next.*param' userspace/libsinsp userspace/libscap | head -n 500
echo '--- tests for string conversion and malformed lengths ---'
rg -n -C 12 'strnlen|std::string_view|mismatched|param_len|invalid_len|as<std::string' userspace/libsinsp/test test 2>/dev/null | head -n 500
echo '--- standalone bounded-scan and formatter probe ---'
python3 - <<'PY'
import ctypes
libc = ctypes.CDLL(None)
libc.strnlen.argtypes = [ctypes.c_char_p, ctypes.c_size_t]
libc.strnlen.restype = ctypes.c_size_t
cases = [
(b"abc\0", 4),
(b"abc", 3),
(b"abc\0XYZ", 7),
(b"\0", 1),
]
for data, length in cases:
n = libc.strnlen(data, length)
print({"data": data, "param_len": length, "strnlen": n,
"fallback_len": length - 1 if length != n + 1 else n,
"has_nul": n < length})
PY
cat >/tmp/probe_percent_s.c <<'C'
`#include` <stdio.h>
`#include` <string.h>
int main(void) {
char data[] = {'a','b','c','X','Y','Z','\0'};
char out[32];
int n = snprintf(out, sizeof out, "%s", data);
printf("formatted=%s length=%zu count=%d\n", out, strlen(out), n);
return 0;
}
C
cc /tmp/probe_percent_s.c -o /tmp/probe_percent_s
/tmp/probe_percent_sRepository: stackrox/falcosecurity-libs
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -u
echo '--- event.h event loading ---'
sed -n '560,635p' userspace/libsinsp/event.h
echo '--- event.cpp parameter APIs ---'
rg -n -C 20 'get_param_as_str|get_param\(|load_params|sinsp_evt_param\(' userspace/libsinsp/event.cpp userspace/libsinsp/event.h || true
echo '--- parameter parser symbols ---'
rg -n -C 12 'm_params|ppm_param_info|param_info|nparams|event.*param|param.*event' userspace/libsinsp/event.cpp userspace/libsinsp/event.h | head -n 420 || true
echo '--- exact parameter tests ---'
rg -n -C 18 'throw|invalid|mismatch|param_len|strnlen|as<std::string_view>|as<std::string>' userspace/libsinsp/test/events_param.ut.cpp || true
echo '--- capture format and event length declarations ---'
rg -n -C 10 'struct scap_evt|scap_evt.*len|len.*scap_evt|PPM_EVENT_HEADER|ppm_evt_hdr' userspace/libscap userspace/libsinsp | head -n 320 || true
echo '--- standalone bounded-scan probe ---'
python3 - <<'PY'
import ctypes
libc = ctypes.CDLL(None)
libc.strnlen.argtypes = [ctypes.c_char_p, ctypes.c_size_t]
libc.strnlen.restype = ctypes.c_size_t
for data, length in [(b"abc\0", 4), (b"abc", 3), (b"abc\0XYZ", 7), (b"\0", 1)]:
n = libc.strnlen(data, length)
print(f"data={data!r} param_len={length} strnlen={n} "
f"has_nul={n < length} fallback_len={length - 1 if length != n + 1 else n}")
PY
cat >/tmp/probe_percent_s.c <<'C'
`#include` <stdio.h>
`#include` <string.h>
int main(void) {
char data[] = {'a','b','c','X','Y','Z','\0'};
char out[32];
int n = snprintf(out, sizeof out, "%s", data);
printf("formatted=%s length=%zu count=%d\n", out, strlen(out), n);
return 0;
}
C
cc /tmp/probe_percent_s.c -o /tmp/probe_percent_s
/tmp/probe_percent_sRepository: stackrox/falcosecurity-libs
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -u
echo '--- decoder locations ---'
rg -l 'scap_event_decode_params' . | head -n 80
echo '--- decoder implementation and declarations ---'
for f in $(rg -l 'scap_event_decode_params' userspace); do
echo "### $f"
rg -n -C 35 'scap_event_decode_params' "$f" || true
done
echo '--- focused tests ---'
sed -n '480,555p' userspace/libsinsp/test/events_param.ut.cpp
rg -n -C 14 'std::string_view|as<std::string>|param_len|strnlen|throw_invalid_len' userspace/libsinsp/test/events_param.ut.cpp || true
echo '--- string parameter metadata ---'
rg -n -C 8 'PT_CHARBUF|PT_FSPATH|PT_FSRELPATH' driver userspace/libscap userspace/libsinsp | head -n 360 || true
echo '--- changed-file diff summary and nearby test changes ---'
git diff --stat -- userspace/libsinsp/event.h userspace/libsinsp/test/events_param.ut.cpp
git diff -- userspace/libsinsp/event.h userspace/libsinsp/test/events_param.ut.cpp | sed -n '1,260p'Repository: stackrox/falcosecurity-libs
Length of output: 41125
🏁 Script executed:
#!/bin/bash
set -u
echo '--- driver string length and ring encoding ---'
sed -n '450,555p' driver/ppm_events.c
rg -n -C 18 'ppm_strncpy_from_user|strnlen\(args->str_storage|val_to_ring\(args' driver/ppm_fillers.c driver/ppm_events.c | head -n 360
echo '--- event buffer ownership and allocation boundaries ---'
rg -n -C 16 'scap_evt.*len|m_pevt_storage|from_scap_evt|event_size|malloc.*evt|realloc.*evt' userspace/libscap userspace/libsinsp | head -n 420
echo '--- PT_CHARBUF formatter context ---'
sed -n '697,890p' userspace/libsinsp/event.cpp
echo '--- test event construction helper declarations ---'
rg -n -C 16 'add_event\(|add_event_advance_ts|scap_const_sized_buffer|scap_event_encode' userspace/libsinsp/test/events_param.ut.cpp test/libscap/test_suites/userspace/scap_event.cpp | head -n 360Repository: stackrox/falcosecurity-libs
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -u
echo '--- malformed-length test call path ---'
rg -n -C 24 'advance_ts_get_event|invalid_string_len|EXPECT_THROW' userspace/libsinsp/test userspace/libsinsp | head -n 420 || true
echo '--- all direct PT_CHARBUF string formatting paths ---'
rg -n -C 8 'as<std::string_view>|"%s".*s\.data|snprintf.*s\.data' userspace/libsinsp userspace/libscap | head -n 300 || true
echo '--- decoder boundary and sentinel definitions ---'
sed -n '88,132p' userspace/libscap/scap_event.c
rg -n -C 10 'PPM_ENABLE_SENTINEL|sentinel_begin|sentinel_end|struct ppm_evt_hdr' driver/ppm_events_public.h userspace/libscap | head -n 320 || trueRepository: stackrox/falcosecurity-libs
Length of output: 50383
Make PT_CHARBUF formatting length-aware.
Malformed or capture-file input can produce strnlen(param_data, param_len) == param_len. The fallback then returns a std::string_view without a NUL, but userspace/libsinsp/event.cpp:877 passes s.data() to %s. The destination bound does not limit source reads. Use a length-aware formatter or reject the no-NUL case before formatting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@userspace/libsinsp/event.h` around lines 216 - 223, The PT_CHARBUF formatting
path around the parameter-length fallback must not pass a non-NUL-terminated
buffer via s.data() to %s. Update the formatting logic used by event.cpp to use
a length-aware formatter with the string view’s size, or reject the no-NUL case
before formatting, while preserving normal NUL-terminated string handling.
Revert the param_len-1 workaround and restore the original throwing behaviour, but add comprehensive diagnostic logging before the throw to identify the root cause of event parameter corruption seen on Fedora CoreOS (kernel 7.1.x). When a string parameter length doesn't match strnlen+1, the error handler now dumps: - Event header fields (nparams from both header and table, event_len, event_type, header size) - The full raw parameter length array from the event - Hex dump of the event header + length array - First 128 bytes of the parameter data region This will show whether the corruption is caused by an nparams mismatch, shifted length array, or something else entirely.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@userspace/libsinsp/event.cpp`:
- Around line 1750-1792: Validate the raw event bounds before diagnostic reads
in the invalid-length handling block: ensure raw->len covers the event header,
cap decoded length entries by both PPM_MAX_EVENT_PARAMS and the available
length-array bytes, and avoid reading any length entry beyond raw->len. Cap the
header/length and parameter-data hex dumps to raw->len, and only derive
data_region from len_array_bytes when the complete declared length array fits;
otherwise skip that data-region calculation or use only the verified available
bytes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 216c4469-cab4-426c-97f2-505ae07a1953
📒 Files selected for processing (1)
userspace/libsinsp/event.cpp
Comment on lines
+1750
to
+1792
| const char *evt_base = reinterpret_cast<const char *>(raw); | ||
| const char *len_array = evt_base + sizeof(struct ppm_evt_hdr); | ||
| bool is_large = (evtinfo->flags & EF_LARGE_PAYLOAD) != 0; | ||
| uint32_t len_entry_size = is_large ? sizeof(uint32_t) : sizeof(uint16_t); | ||
| uint32_t len_array_bytes = raw->nparams * len_entry_size; | ||
|
|
||
| // Dump all param lengths from the raw length array. | ||
| std::stringstream lens; | ||
| lens << "raw param lengths (" << (is_large ? "large" : "u16") << "):"; | ||
| for(uint32_t i = 0; i < raw->nparams && i < PPM_MAX_EVENT_PARAMS; i++) { | ||
| uint32_t plen = 0; | ||
| if(is_large) { | ||
| memcpy(&plen, len_array + i * sizeof(uint32_t), sizeof(uint32_t)); | ||
| } else { | ||
| uint16_t plen16 = 0; | ||
| memcpy(&plen16, len_array + i * sizeof(uint16_t), sizeof(uint16_t)); | ||
| plen = plen16; | ||
| } | ||
| lens << " [" << i << "]=" << plen; | ||
| } | ||
| libsinsp_logger()->log(lens.str(), sinsp_logger::SEV_ERROR); | ||
|
|
||
| // Dump the raw event header + length array as hex. | ||
| size_t hdr_and_lens = sizeof(struct ppm_evt_hdr) + len_array_bytes; | ||
| size_t dump_len = std::min(hdr_and_lens, (size_t)256); | ||
| libsinsp_logger()->log( | ||
| "raw header+lengths (" + std::to_string(dump_len) + " bytes):\n" + | ||
| buffer_to_multiline_hex(evt_base, dump_len), | ||
| sinsp_logger::SEV_ERROR); | ||
|
|
||
| // Dump the first 128 bytes of the param data region. | ||
| const char *data_region = len_array + len_array_bytes; | ||
| size_t data_avail = 0; | ||
| if(raw->len > hdr_and_lens) { | ||
| data_avail = raw->len - hdr_and_lens; | ||
| } | ||
| size_t data_dump = std::min(data_avail, (size_t)128); | ||
| if(data_dump > 0) { | ||
| libsinsp_logger()->log( | ||
| "param data region (first " + std::to_string(data_dump) + | ||
| " of " + std::to_string(data_avail) + " bytes):\n" + | ||
| buffer_to_multiline_hex(data_region, data_dump), | ||
| sinsp_logger::SEV_ERROR); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate raw-event bounds before reading diagnostic fields.
The invalid-length path trusts raw->nparams and reads len_array before it verifies that the declared length array fits in raw->len. A truncated event can therefore cause memcpy() at Lines 1762-1765 or buffer_to_multiline_hex() at Lines 1775-1777 to read beyond the event buffer. This can crash the collector while it handles the malformed event.
Limit decoded entries to (raw->len - sizeof(ppm_evt_hdr)) / len_entry_size. Limit every hex dump to raw->len. Do not calculate data_region from the declared length-array size unless that full array fits in the event.
As per path instructions, focus on major issues impacting performance, readability, maintainability and security.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@userspace/libsinsp/event.cpp` around lines 1750 - 1792, Validate the raw
event bounds before diagnostic reads in the invalid-length handling block:
ensure raw->len covers the event header, cap decoded length entries by both
PPM_MAX_EVENT_PARAMS and the available length-array bytes, and avoid reading any
length entry beyond raw->len. Cap the header/length and parameter-data hex dumps
to raw->len, and only derive data_region from len_array_bytes when the complete
declared length array fits; otherwise skip that data-region calculation or use
only the verified available bytes.
Source: Path instructions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@driver/modern_bpf/helpers/store/auxmap_store_params.h`:
- Around line 152-156: Ensure maps__release_auxiliary_map() is executed on every
exit path of the helper, including the !rb, !counter, and oversized-event
branches before their returns. Route these branches and the
post-bpf_ringbuf_output path through a shared out cleanup block, preserving each
branch’s existing return behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d0f6a5d8-6f4f-4ab9-8b46-61924d5a6a0a
📒 Files selected for processing (3)
driver/modern_bpf/helpers/base/maps_getters.hdriver/modern_bpf/helpers/store/auxmap_store_params.huserspace/libpman/src/maps.c
🚧 Files skipped from review as they are similar to previous changes (1)
- driver/modern_bpf/helpers/base/maps_getters.h
Comment on lines
+152
to
+156
|
|
||
| /* The event has been handed to the ring buffer (or dropped); release this | ||
| * task's auxiliary map entry so the LRU hash is not filled with one entry | ||
| * per task that has ever produced an event. See falcosecurity/libs#2719. */ | ||
| maps__release_auxiliary_map(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release the task entry on every exit path.
maps__release_auxiliary_map() runs only after bpf_ringbuf_output(). The !rb branch at Line 127, the !counter branch at Line 132, and the oversized-event branch at Line 141 return before this block. Those paths leave the current task's entry in auxiliary_maps.
Repeated dropped events can fill the LRU and evict an entry still used by another in-flight event. Route all early returns through a common out: cleanup block, or release the entry before each return.
Proposed cleanup path
if(!rb) {
...
- return;
+ goto out;
}
if(!counter) {
- return;
+ goto out;
}
if(auxmap->payload_pos > MAX_EVENT_SIZE) {
...
- return;
+ goto out;
}
+out:
maps__release_auxiliary_map();
return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@driver/modern_bpf/helpers/store/auxmap_store_params.h` around lines 152 -
156, Ensure maps__release_auxiliary_map() is executed on every exit path of the
helper, including the !rb, !counter, and oversized-event branches before their
returns. Route these branches and the post-bpf_ringbuf_output path through a
shared out cleanup block, preserving each branch’s existing return behavior.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Use lenient extraction instead of asstd::string_view() which throws when BPF probe data has unexpected trailing bytes after the null terminator. The uncaught exception propagates through sinsp::next() and aborts the collector.