Repository navigation
fix(OSQUERY-002-2): CU-17tkuw5wp49 2 review findings across 2 files - #115
flamingo[bot] wants to merge 2 commits into
Conversation
| {"/libpod-", "podman"}, | ||
| }; | ||
|
|
||
| std::string getProcUptimeContents() { |
There was a problem hiding this comment.
🦩 🔴 getProcUptimeContents / getSystemBootTime throw std::runtime_error in production osquery code
In osquery/experimental/experiments/linuxevents/src/bpfprocesseventstable.cpp, getProcUptimeContents() and getSystemBootTime() were changed from throwing std::runtime_error to returning osquery::Status (success/failure) via an out-parameter, following the OSQUERY-002-2/OSQUERY-003 convention. The caller BPFProcessEventsTable::generate() now checks the Status returned by getSystemBootTime() and, on failure, returns an empty TableRows instead of letting an exception propagate out of TablePlugin::generate(). This removes the uncaught-exception crash path on /proc/uptime read/parse failures. One judgment call: on failure the function silently returns no rows rather than logging; a more complete fix might also log the Status message via LOG(ERROR), which was not added here to keep the change minimal.
🤖 Prompt for AI agents
In osquery/experimental/experiments/linuxevents/src/bpfprocesseventstable.cpp around line 34, review and complete this code-review fix: getProcUptimeContents / getSystemBootTime throw std::runtime_error in production osquery code.
What the draft fix changed: In `osquery/experimental/experiments/linuxevents/src/bpfprocesseventstable.cpp`, `getProcUptimeContents()` and `getSystemBootTime()` were changed from throwing `std::runtime_error` to returning `osquery::Status` (success/failure) via an out-parameter, following the OSQUERY-002-2/OSQUERY-003 convention. The caller `BPFProcessEventsTable::generate()` now checks the `Status` returned by `getSystemBootTime()` and, on failure, returns an empty `TableRows` instead of letting an exception propagate out of `TablePlugin::generate()`. This removes the uncaught-exception crash path on `/proc/uptime` read/parse failures. One judgment call: on failure the function silently returns no rows rather than logging; a more complete fix might also log the Status message via `LOG(ERROR)`, which was not added here to keep the change minimal.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -32,21 +33,26 @@ void configureBPFMemoryLimitsHelper() { | |||
|
|
|||
| auto err = setrlimit(RLIMIT_MEMLOCK, &rl); | |||
There was a problem hiding this comment.
🦩 🔵 configureBPFMemoryLimitsHelper throws std::runtime_error instead of returning Status
Changed configureBPFMemoryLimitsHelper() in osquery/events/linux/bpf/setrlimit.cpp to return bool instead of throwing std::runtime_error on setrlimit() failure. Added a static bool setrlimit_succeeded captured by the std::call_once lambda invocation inside configureBPFMemoryLimits(), which now checks the result directly and returns Status::failure(...) with the original message instead of relying on catching an exception. The try/catch around std::call_once was removed since no exception is thrown anymore. This preserves the "call once" semantics and the externally observed Status::failure behavior for callers.
🤖 Prompt for AI agents
In osquery/events/linux/bpf/setrlimit.cpp around line 33, review and complete this code-review fix: configureBPFMemoryLimitsHelper throws std::runtime_error instead of returning Status.
What the draft fix changed: Changed configureBPFMemoryLimitsHelper() in osquery/events/linux/bpf/setrlimit.cpp to return bool instead of throwing std::runtime_error on setrlimit() failure. Added a static bool setrlimit_succeeded captured by the std::call_once lambda invocation inside configureBPFMemoryLimits(), which now checks the result directly and returns Status::failure(...) with the original message instead of relying on catching an exception. The try/catch around std::call_once was removed since no exception is thrown anymore. This preserves the "call once" semantics and the externally observed Status::failure behavior for callers.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
Closes 2 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
osquery/experimental/experiments/linuxevents/src/bpfprocesseventstable.cpp:34osquery/events/linux/bpf/setrlimit.cpp:33What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
cb1fa104-eac6-46a7-aba8-90d794c59386Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-17tkuw5wp49 Osquery review findings sweep (8 PRs)