fix(OSQUERY-002-2): CU-86akhf8u2 12 review findings across 11 files

[flamingo]

Closes 12 review findings across 11 files.

Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.

#	Fix confidence	Finding	Location
1	🟢 95 high	NonBlockingFStream::close() constructs Status via default constructor instead of Status::success()	osquery/events/linux/syslog.cpp:131
2	🟡 70 medium	SyslogEventPublisher::createPipe leaks lockFd_ file descriptor semantics unaffected but setUp does not close readStream_ on later failure paths	osquery/events/linux/syslog.cpp:161
3	🟢 97 high	Status::failure() return value discarded in queryRpmDb error path	osquery/tables/system/tests/linux/rpm_packages_tests.cpp:96
4	🟢 95 high	numSubscriptions relies on exception handling instead of Status for a normal not-found path	osquery/events/eventfactory.cpp:204
5	🟡 88 medium	getProcUptimeContents and getSystemBootTime throw std::runtime_error instead of returning osquery::Status	osquery/experimental/experiments/linuxevents/src/bpfprocesseventstable.cpp:34
6	🔴 55 low — review closely	dbusmethod.h throws Status via throw Status::failure(...) instead of returning it	osquery/tables/system/linux/dbus/methods/dbusmethod.h:119
7	🟢 95 high	rotDecode has undefined/duplicated behavior at ASCII boundary 'm'/'M' due to overlapping range checks	osquery/utils/rot13.cpp:19
8	🔴 25 low — review closely	throw std::runtime_error used instead of Status in pipe_channel_factory.cpp	osquery/worker/ipc/posix/pipe_channel_factory.cpp:26
9	🟡 80 medium	parseInode throws std::stoul exception instead of returning Status	osquery/tables/system/linux/process_open_pipes.cpp:53
10	🔴 45 low — review closely	Plugin::setName throws std::runtime_error instead of returning Status	osquery/core/plugins/plugin.cpp:14
11	🟡 70 medium	EvtSubscription constructor throws Status instead of returning it, breaking the Status-based error contract	osquery/events/windows/evtsubscription.cpp:96
12	🟢 90 high	processes.cpp genMemoryMap pushes a placeholder failure row even when OpenProcess fails, duplicating semantics of Status::failure	osquery/tables/system/windows/processes.cpp:184

What 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: 39233833-f1d6-416e-93a7-71af15e3e968

Merging 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-86akhf8u2 Osquery review findings sweep (15 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

12 finding(s) fixed in this draft — 12 explained inline on the diff; 3 low-confidence hunk(s) need close review before merging.

[flamingo]

🦩 🔴 NonBlockingFStream::close() constructs Status via default constructor instead of Status::success()

In NonBlockingFStream::close() (osquery/events/linux/syslog.cpp), changed return Status(); to return Status::success(); to match the codebase convention used elsewhere in the file.

🤖 Prompt for AI agents

In osquery/events/linux/syslog.cpp around line 131, review and complete this code-review fix: NonBlockingFStream::close() constructs Status via default constructor instead of Status::success().
What the draft fix changed: In `NonBlockingFStream::close()` (osquery/events/linux/syslog.cpp), changed `return Status();` to `return Status::success();` to match the codebase convention used elsewhere in the file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 SyslogEventPublisher::createPipe leaks lockFd_ file descriptor semantics unaffected but setUp does not close readStream_ on later failure paths

In SyslogEventPublisher::setUp(), added a call to unlockPipe() before returning the failure status when readStream_.openReadOnly fails after lockPipe succeeded, so the lock file descriptor is released instead of leaking/staying locked. This directly addresses the described leak path; it does not change behavior when setUp succeeds or when lockPipe itself fails (unlockPipe is a no-op when lockFd_ is -1).

🤖 Prompt for AI agents

In osquery/events/linux/syslog.cpp around line 161, review and complete this code-review fix: SyslogEventPublisher::createPipe leaks lockFd_ file descriptor semantics unaffected but setUp does not close readStream_ on later failure paths.
What the draft fix changed: In `SyslogEventPublisher::setUp()`, added a call to `unlockPipe()` before returning the failure status when `readStream_.openReadOnly` fails after `lockPipe` succeeded, so the lock file descriptor is released instead of leaking/staying locked. This directly addresses the described leak path; it does not change behavior when setUp succeeds or when lockPipe itself fails (unlockPipe is a no-op when lockFd_ is -1).
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 Status::failure() return value discarded in queryRpmDb error path

In queryRpmDb (osquery/tables/system/tests/linux/rpm_packages_tests.cpp), changed the discarded Status::failure("Cannot read configuration"); statement to return Status::failure("Cannot read configuration"); in the if (rpmReadConfigFiles(nullptr, nullptr) != 0) error branch, so the function now returns the failure status instead of falling through and continuing execution with an unconfigured rpm environment.

🤖 Prompt for AI agents

In osquery/tables/system/tests/linux/rpm_packages_tests.cpp around line 96, review and complete this code-review fix: Status::failure() return value discarded in queryRpmDb error path.
What the draft fix changed: In queryRpmDb (osquery/tables/system/tests/linux/rpm_packages_tests.cpp), changed the discarded `Status::failure("Cannot read configuration");` statement to `return Status::failure("Cannot read configuration");` in the `if (rpmReadConfigFiles(nullptr, nullptr) != 0)` error branch, so the function now returns the failure status instead of falling through and continuing execution with an unconfigured rpm environment.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 97 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 numSubscriptions relies on exception handling instead of Status for a normal not-found path

Removed the dead try/catch(std::out_of_range&) block in EventFactory::numSubscriptions() (osquery/events/eventfactory.cpp). getEventPublisher() never throws; it already returns nullptr on a missing entry via the .count()==0 guard. The function now simply calls getEventPublisher() directly and relies solely on the existing nullptr check below it, matching the codebase's Status/nullptr convention. Behavior is unchanged since the catch branch was unreachable dead code.

🤖 Prompt for AI agents

In osquery/events/eventfactory.cpp around line 204, review and complete this code-review fix: numSubscriptions relies on exception handling instead of Status for a normal not-found path.
What the draft fix changed: Removed the dead try/catch(std::out_of_range&) block in EventFactory::numSubscriptions() (osquery/events/eventfactory.cpp). getEventPublisher() never throws; it already returns nullptr on a missing entry via the .count()==0 guard. The function now simply calls getEventPublisher() directly and relies solely on the existing nullptr check below it, matching the codebase's Status/nullptr convention. Behavior is unchanged since the catch branch was unreachable dead code.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 getProcUptimeContents and getSystemBootTime throw std::runtime_error instead of returning osquery::Status

Changed getProcUptimeContents() to return osquery::Status and output the uptime string via an out-parameter, and changed getSystemBootTime() to return osquery::Status while outputting the boot time via an out-parameter, replacing all throw std::runtime_error(...) calls with Status::failure(...). Updated the sole call site in BPFProcessEventsTable::generate() to check the returned Status and return an empty TableRows on failure instead of letting an exception propagate, eliminating the crash risk during a SELECT against bpf_process_events_v2 when /proc/uptime is unreadable or malformed.

🤖 Prompt for AI agents

In osquery/experimental/experiments/linuxevents/src/bpfprocesseventstable.cpp around line 34, review and complete this code-review fix: getProcUptimeContents and getSystemBootTime throw std::runtime_error instead of returning osquery::Status.
What the draft fix changed: Changed `getProcUptimeContents()` to return `osquery::Status` and output the uptime string via an out-parameter, and changed `getSystemBootTime()` to return `osquery::Status` while outputting the boot time via an out-parameter, replacing all `throw std::runtime_error(...)` calls with `Status::failure(...)`. Updated the sole call site in `BPFProcessEventsTable::generate()` to check the returned `Status` and return an empty `TableRows` on failure instead of letting an exception propagate, eliminating the crash risk during a `SELECT` against `bpf_process_events_v2` when `/proc/uptime` is unreadable or malformed.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 throw std::runtime_error used instead of Status in pipe_channel_factory.cpp

In pipe_channel_factory.cpp, PipeChannelFactory::createChannelTicket() was changed from throwing std::runtime_error on pipe() failure to returning osquery::Status (via Status::failure(...) / Status::success()), with the ticket now passed by reference (PipeChannelTicket& ticket) as an out-parameter instead of being returned by value, so callers must be updated to pre-construct a PipeChannelTicket and check the returned Status. This is a mechanically sound fix for the specific throw-vs-Status finding, but it is INCOMPLETE and RISKY as delivered: it requires a corresponding change to pipe_channel_factory.h (declaration of createChannelTicket, likely changing its signature from PipeChannelTicket createChannelTicket() to Status createChannelTicket(PipeChannelTicket&)) which was not provided to me and which I did not emit as a NEWFILE because I cannot see its current content and risk breaking it further; and it requires updating every caller (e.g. LinuxTableContainerIPC::connectToContainer, mentioned in the finding) to match the new signature, which I also cannot see or modify since they are in other files. Without those companion changes this file will fail to compile against the existing header. A complete fix requires: (a) updating the header declaration, (b) updating all call sites to pass a ticket by reference and check the Status, and (c) verifying Status is included/available in scope (I assumed osquery/logger/logger.h-style includes but did not add the actual Status header, e.g. osquery/utils/status/status.h, which may also be missing and would need to be added to this file's includes).

🤖 Prompt for AI agents

In osquery/worker/ipc/posix/pipe_channel_factory.cpp around line 26, review and complete this code-review fix: throw std::runtime_error used instead of Status in pipe_channel_factory.cpp.
What the draft fix changed: In `pipe_channel_factory.cpp`, `PipeChannelFactory::createChannelTicket()` was changed from throwing `std::runtime_error` on `pipe()` failure to returning `osquery::Status` (via `Status::failure(...)` / `Status::success()`), with the ticket now passed by reference (`PipeChannelTicket& ticket`) as an out-parameter instead of being returned by value, so callers must be updated to pre-construct a `PipeChannelTicket` and check the returned `Status`. This is a mechanically sound fix for the specific throw-vs-Status finding, but it is INCOMPLETE and RISKY as delivered: it requires a corresponding change to `pipe_channel_factory.h` (declaration of `createChannelTicket`, likely changing its signature from `PipeChannelTicket createChannelTicket()` to `Status createChannelTicket(PipeChannelTicket&)`) which was not provided to me and which I did not emit as a NEWFILE because I cannot see its current content and risk breaking it further; and it requires updating every caller (e.g. `LinuxTableContainerIPC::connectToContainer`, mentioned in the finding) to match the new signature, which I also cannot see or modify since they are in other files. Without those companion changes this file will fail to compile against the existing header. A complete fix requires: (a) updating the header declaration, (b) updating all call sites to pass a ticket by reference and check the `Status`, and (c) verifying `Status` is included/available in scope (I assumed `osquery/logger/logger.h`-style includes but did not add the actual `Status` header, e.g. `osquery/utils/status/status.h`, which may also be missing and would need to be added to this file's includes).
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 25 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 parseInode throws std::stoul exception instead of returning Status

Changed parseInode in osquery/tables/system/linux/process_open_pipes.cpp from returning int (throwing on std::stoul overflow) to returning Status and writing the parsed inode into an out-parameter (unsigned long& inode), wrapping the std::stoul call in a try/catch that converts std::out_of_range/std::invalid_argument into Status::failure. Updated the sole call site in getPipeInfo to declare inode, call parseInode(desc.second, inode), log a warning via LOG(WARNING) on failure (falling back to inode 0, matching prior behavior for the "no match" branch), and pass inode to createPipeInfoStruct. This removes the uncaught-exception path while preserving existing control flow and output for the non-error case.

🤖 Prompt for AI agents

In osquery/tables/system/linux/process_open_pipes.cpp around line 53, review and complete this code-review fix: parseInode throws std::stoul exception instead of returning Status.
What the draft fix changed: Changed `parseInode` in `osquery/tables/system/linux/process_open_pipes.cpp` from returning `int` (throwing on `std::stoul` overflow) to returning `Status` and writing the parsed inode into an out-parameter (`unsigned long& inode`), wrapping the `std::stoul` call in a try/catch that converts `std::out_of_range`/`std::invalid_argument` into `Status::failure`. Updated the sole call site in `getPipeInfo` to declare `inode`, call `parseInode(desc.second, inode)`, log a warning via `LOG(WARNING)` on failure (falling back to inode `0`, matching prior behavior for the "no match" branch), and pass `inode` to `createPipeInfoStruct`. This removes the uncaught-exception path while preserving existing control flow and output for the non-error case.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 Plugin::setName throws std::runtime_error instead of returning Status

Changed Plugin::setName in osquery/core/plugins/plugin.cpp to return Status instead of void, replacing the throw std::runtime_error(error) with return Status(1, error) on the rename-conflict path and return Status::success() on the normal path, consistent with osquery's Status-based error handling convention. This requires the corresponding declaration in osquery/core/plugins/plugin.h (not shown/provided) to also be updated to Status setName(const std::string& name);, and any existing callers of setName that ignore the return value or expect a void call to be updated to check the returned Status. Since plugin.h and caller sites are outside this file and were not provided, those changes could not be made here, and the build will fail to link/compile until the header signature is updated to match; a complete fix additionally requires editing plugin.h and auditing all call sites of setName.

🤖 Prompt for AI agents

In osquery/core/plugins/plugin.cpp around line 14, review and complete this code-review fix: Plugin::setName throws std::runtime_error instead of returning Status.
What the draft fix changed: Changed `Plugin::setName` in osquery/core/plugins/plugin.cpp to return `Status` instead of `void`, replacing the `throw std::runtime_error(error)` with `return Status(1, error)` on the rename-conflict path and `return Status::success()` on the normal path, consistent with osquery's Status-based error handling convention. This requires the corresponding declaration in `osquery/core/plugins/plugin.h` (not shown/provided) to also be updated to `Status setName(const std::string& name);`, and any existing callers of `setName` that ignore the return value or expect a `void` call to be updated to check the returned `Status`. Since plugin.h and caller sites are outside this file and were not provided, those changes could not be made here, and the build will fail to link/compile until the header signature is updated to match; a complete fix additionally requires editing plugin.h and auditing all call sites of `setName`.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🔴 EvtSubscription constructor throws Status instead of returning it, breaking the Status-based error contract

Changed EvtSubscription::EvtSubscription(const std::string&) so it no longer throws Status::failure(...). The constructor is now a trivial, non-throwing default constructor (EvtSubscription::EvtSubscription()), and the fallible subscription logic (channel assignment, EvtSubscribe call, and error handling) was moved into a new Status EvtSubscription::initialize(const std::string& channel) method that returns Status::failure(...) instead of throwing it. EvtSubscription::create was updated to construct the object with the default constructor, call initialize(), check the returned Status, and only assign to obj on success; the catch (const Status&) clause was removed since Status is no longer thrown, while catch (const std::bad_alloc&) is retained for allocation failures. Note: this requires the header osquery/events/windows/evtsubscription.h (not shown/editable here) to declare the new default constructor and the initialize method with matching signatures — since I cannot see or modify that file, this change assumes such declarations can/will be added; if the header only declares the old explicit EvtSubscription(const std::string&) constructor, this file will fail to compile until the header is updated accordingly.

🤖 Prompt for AI agents

In osquery/events/windows/evtsubscription.cpp around line 96, review and complete this code-review fix: EvtSubscription constructor throws Status instead of returning it, breaking the Status-based error contract.
What the draft fix changed: Changed `EvtSubscription::EvtSubscription(const std::string&)` so it no longer throws `Status::failure(...)`. The constructor is now a trivial, non-throwing default constructor (`EvtSubscription::EvtSubscription()`), and the fallible subscription logic (channel assignment, `EvtSubscribe` call, and error handling) was moved into a new `Status EvtSubscription::initialize(const std::string& channel)` method that returns `Status::failure(...)` instead of throwing it. `EvtSubscription::create` was updated to construct the object with the default constructor, call `initialize()`, check the returned `Status`, and only assign to `obj` on success; the `catch (const Status&)` clause was removed since Status is no longer thrown, while `catch (const std::bad_alloc&)` is retained for allocation failures. Note: this requires the header `osquery/events/windows/evtsubscription.h` (not shown/editable here) to declare the new default constructor and the `initialize` method with matching signatures — since I cannot see or modify that file, this change assumes such declarations can/will be added; if the header only declares the old `explicit EvtSubscription(const std::string&)` constructor, this file will fail to compile until the header is updated accordingly.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer

[flamingo]

🦩 🟠 processes.cpp genMemoryMap pushes a placeholder failure row even when OpenProcess fails, duplicating semantics of Status::failure

In genMemoryMap (osquery/tables/system/windows/processes.cpp), removed the placeholder Row r construction and results.push_back(r) from the proc == nullptr branch (OpenProcess failure). The function now only returns Status::failure(...) in that case, matching the asymmetry-free pattern used elsewhere in the file (e.g. the CreateToolhelp32Snapshot failure branch just below, getProcList, getUserProcessParameters), so no bogus all-(-1) row is appended to results for callers like genProcessMemoryMap.

🤖 Prompt for AI agents

In osquery/tables/system/windows/processes.cpp around line 184, review and complete this code-review fix: processes.cpp genMemoryMap pushes a placeholder failure row even when OpenProcess fails, duplicating semantics of Status::failure.
What the draft fix changed: In `genMemoryMap` (osquery/tables/system/windows/processes.cpp), removed the placeholder `Row r` construction and `results.push_back(r)` from the `proc == nullptr` branch (OpenProcess failure). The function now only returns `Status::failure(...)` in that case, matching the asymmetry-free pattern used elsewhere in the file (e.g. the `CreateToolhelp32Snapshot` failure branch just below, `getProcList`, `getUserProcessParameters`), so no bogus all-(-1) row is appended to `results` for callers like `genProcessMemoryMap`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer