fix(OSQUERY-002-2): 22 review findings across 13 files

[flamingo]

Closes 22 review findings across 13 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	🔴 40 low — review closely	getDatabaseValue/setDatabaseValue/deleteDatabaseValue throw std::runtime_error instead of returning Status when DB not initialized	osquery/database/database.cpp:260
2	🟢 90 high	setDatabaseBatch throws std::runtime_error instead of returning a failure Status	osquery/database/database.cpp:324
3	🟢 90 high	deleteDatabaseValue throws std::runtime_error instead of returning a failure Status	osquery/database/database.cpp:350
4	🟢 90 high	deleteDatabaseRange throws std::runtime_error instead of returning a failure Status	osquery/database/database.cpp:373
5	🟢 90 high	scanDatabaseKeys throws std::runtime_error instead of returning a failure Status	osquery/database/database.cpp:402
6	🟡 75 medium	getDatabaseValue(int&) calls std::stoi without validating or catching exceptions	osquery/database/database.cpp:296
7	🟡 80 medium	SystemStateTracker::create() throws Status instead of returning it	osquery/events/linux/bpf/systemstatetracker.cpp:42
8	🟡 70 medium	SystemStateTracker private constructor throws Status on restart failure	osquery/events/linux/bpf/systemstatetracker.cpp:261
9	🔴 55 low — review closely	Uri constructor throws std::invalid_argument instead of returning osquery::Status	osquery/remote/uri.cpp:42
10	🔴 55 low — review closely	Uri constructor throws std::invalid_argument for invalid authority	osquery/remote/uri.cpp:62
11	🔴 40 low — review closely	carve() swallows partial-block POST failures without aborting the upload	osquery/carver/carver.cpp:247
12	🟡 80 medium	getService() throws std::runtime_error instead of returning Status	osquery/tables/system/windows/services.cpp:93
13	🟢 92 high	getYaraParser uses try/catch around dynamic_pointer_cast which cannot throw std::bad_cast	osquery/tables/yara/yara.cpp:96
14	🔴 55 low — review closely	EvtSubscription constructor throws Status instead of returning it	osquery/events/windows/evtsubscription.cpp:108
15	🟢 90 high	handleRuleFiles declared with pt::ptree parameter in header but defined with rapidjson::Value in .cpp	osquery/tables/yara/yara_utils.h:91
16	🟢 90 high	compileFromString uses positional argument mismatched with declared signature for YARACompilerCallback	osquery/tables/yara/yara_utils.h:76
17	🔴 40 low — review closely	std::bad_alloc thrown instead of Status::failure in getPathFromReferenceNumber	osquery/events/windows/ntfs_event_publisher.cpp:224
18	🔴 40 low — review closely	std::bad_alloc used instead of Status in resize-failure guard while rest of function uses Status	osquery/events/windows/ntfs_event_publisher.cpp:226
19	🟢 95 high	Duplicate #include of boost/filesystem.hpp and gtest/gtest.h in ssh_keys_tests.cpp	osquery/tables/system/tests/posix/ssh_keys_tests.cpp:10
20	🟡 75 medium	getOptions() falls back to always_verify_peer(false) unconditionally in openframe_mode	osquery/remote/transports/tls.cpp:122
21	🟡 88 medium	genPasswordPolicy leaks CFQueryRef and can double-release CFArrayRef records on empty result	osquery/tables/system/darwin/password_policy.cpp:119
22	🟢 90 high	formatTimestampToDate wraps non-throwing gmtime/put_time in try/catch, contrary to Status-based error handling used elsewhere	osquery/tables/system/windows/programs.cpp:70

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: 01e7aadc-0204-47ef-b373-fc5dbadf3ab5

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.

[flamingo]

🦩 What this fix changed, finding by finding

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

[flamingo]

🦩 🔴 getDatabaseValue/setDatabaseValue/deleteDatabaseValue throw std::runtime_error instead of returning Status when DB not initialized

In getDatabaseValue(domain, key, std::string&), replaced throw std::runtime_error("Cannot get database value: " + key); with return Status::failure("Cannot get database value: " + key); when kDBInitialized is false. This also indirectly fixes the corresponding case for setDatabaseValue/deleteDatabaseValue mentioned in this finding's title, each addressed individually below.

(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 260, review and complete this code-review fix: getDatabaseValue/setDatabaseValue/deleteDatabaseValue throw std::runtime_error instead of returning Status when DB not initialized.
What the draft fix changed: In `getDatabaseValue(domain, key, std::string&)`, replaced `throw std::runtime_error("Cannot get database value: " + key);` with `return Status::failure("Cannot get database value: " + key);` when `kDBInitialized` is false. This also indirectly fixes the corresponding case for `setDatabaseValue`/`deleteDatabaseValue` mentioned in this finding's title, each addressed individually below.

_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

[flamingo]

🦩 🔴 setDatabaseBatch throws std::runtime_error instead of returning a failure Status

In setDatabaseBatch, replaced throw std::runtime_error("Cannot set database values"); with return Status::failure("Cannot set database values"); when kDBInitialized is false. Since setDatabaseValue delegates to setDatabaseBatch, it now also correctly propagates a Status instead of throwing.

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 324, review and complete this code-review fix: setDatabaseBatch throws std::runtime_error instead of returning a failure Status.
What the draft fix changed: In `setDatabaseBatch`, replaced `throw std::runtime_error("Cannot set database values");` with `return Status::failure("Cannot set database values");` when `kDBInitialized` is false. Since `setDatabaseValue` delegates to `setDatabaseBatch`, it now also correctly propagates a Status instead of throwing.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 deleteDatabaseValue throws std::runtime_error instead of returning a failure Status

In deleteDatabaseValue, replaced throw std::runtime_error("Cannot delete database value: " + key); with return Status::failure("Cannot delete database value: " + key); when kDBInitialized is false.

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 350, review and complete this code-review fix: deleteDatabaseValue throws std::runtime_error instead of returning a failure Status.
What the draft fix changed: In `deleteDatabaseValue`, replaced `throw std::runtime_error("Cannot delete database value: " + key);` with `return Status::failure("Cannot delete database value: " + key);` when `kDBInitialized` is false.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 deleteDatabaseRange throws std::runtime_error instead of returning a failure Status

In deleteDatabaseRange, replaced throw std::runtime_error("Cannot delete database values: " + low + " - " + high); with return Status::failure("Cannot delete database values: " + low + " - " + high); when kDBInitialized is false.

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 373, review and complete this code-review fix: deleteDatabaseRange throws std::runtime_error instead of returning a failure Status.
What the draft fix changed: In `deleteDatabaseRange`, replaced `throw std::runtime_error("Cannot delete database values: " + low + " - " + high);` with `return Status::failure("Cannot delete database values: " + low + " - " + high);` when `kDBInitialized` is false.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 scanDatabaseKeys throws std::runtime_error instead of returning a failure Status

In scanDatabaseKeys(domain, keys, prefix, max), replaced throw std::runtime_error("Cannot scan database values: " + prefix); with return Status::failure("Cannot scan database values: " + prefix); when kDBInitialized is false. This is the function dumpDatabase() relies on, so it now degrades gracefully instead of crashing.

🤖 Prompt for AI agents

In osquery/database/database.cpp around line 402, review and complete this code-review fix: scanDatabaseKeys throws std::runtime_error instead of returning a failure Status.
What the draft fix changed: In `scanDatabaseKeys(domain, keys, prefix, max)`, replaced `throw std::runtime_error("Cannot scan database values: " + prefix);` with `return Status::failure("Cannot scan database values: " + prefix);` when `kDBInitialized` is false. This is the function `dumpDatabase()` relies on, so it now degrades gracefully instead of crashing.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 std::bad_alloc used instead of Status in resize-failure guard while rest of function uses Status

Same code change as finding 1 (both findings reference the identical line/pattern); the single throw std::bad_alloc(); statement in getPathFromReferenceNumber's buffer resize check was replaced with a Status::failure(...) return, restoring uniform Status-based error propagation so callers like getPathFromParentFRN can properly detect and handle the failure via .ok().

(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)

🤖 Prompt for AI agents

In osquery/events/windows/ntfs_event_publisher.cpp around line 226, review and complete this code-review fix: std::bad_alloc used instead of Status in resize-failure guard while rest of function uses Status.
What the draft fix changed: Same code change as finding 1 (both findings reference the identical line/pattern); the single `throw std::bad_alloc();` statement in `getPathFromReferenceNumber`'s buffer resize check was replaced with a `Status::failure(...)` return, restoring uniform Status-based error propagation so callers like `getPathFromParentFRN` can properly detect and handle the failure via `.ok()`.

_(Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.)_
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

[flamingo]

🦩 🟠 Duplicate #include of boost/filesystem.hpp and gtest/gtest.h in ssh_keys_tests.cpp

Removed the duplicate #include <boost/filesystem.hpp> and #include <gtest/gtest.h> lines that appeared a second time right before the osquery/sql/sql.h include block at the top of ssh_keys_tests.cpp. The single occurrence of each include (already present earlier in the file) is retained, preserving all functionality since include guards made the duplicates harmless no-ops.

🤖 Prompt for AI agents

In osquery/tables/system/tests/posix/ssh_keys_tests.cpp around line 10, review and complete this code-review fix: Duplicate #include of boost/filesystem.hpp and gtest/gtest.h in ssh_keys_tests.cpp.
What the draft fix changed: Removed the duplicate `#include <boost/filesystem.hpp>` and `#include <gtest/gtest.h>` lines that appeared a second time right before the `osquery/sql/sql.h` include block at the top of `ssh_keys_tests.cpp`. The single occurrence of each include (already present earlier in the file) is retained, preserving all functionality since include guards made the duplicates harmless no-ops.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 getOptions() falls back to always_verify_peer(false) unconditionally in openframe_mode

In getOptions(), replaced the unconditional if (FLAGS_openframe_mode) { options.always_verify_peer(false); return options; } early-return with a debug-only gated check #ifndef NDEBUG ... if (FLAGS_openframe_mode && FLAGS_openframe_mode_allow_unsafe) { ... } #endif, and introduced a new HIDDEN_FLAG(bool, openframe_mode_allow_unsafe, false, ...) (declared alongside tls_allow_unsafe under #ifndef NDEBUG) that must be explicitly enabled to disable peer verification in openframe_mode. In release builds and by default in debug builds, execution now falls through to the normal certificate pinning/verification logic below (server_certificate_file_, openssl_verify_path, openssl_certificate), matching the pattern used for tls_allow_unsafe. Risk: this changes default behavior for openframe_mode deployments — any existing production reliance on always_verify_peer(false) in openframe_mode will now perform full verification unless the new flag is set, which may break connectivity if server certs aren't properly configured; this is the intended security fix but should be validated against actual openframe deployment cert setups.

🤖 Prompt for AI agents

In osquery/remote/transports/tls.cpp around line 122, review and complete this code-review fix: getOptions() falls back to always_verify_peer(false) unconditionally in openframe_mode.
What the draft fix changed: In `getOptions()`, replaced the unconditional `if (FLAGS_openframe_mode) { options.always_verify_peer(false); return options; }` early-return with a debug-only gated check `#ifndef NDEBUG ... if (FLAGS_openframe_mode && FLAGS_openframe_mode_allow_unsafe) { ... } #endif`, and introduced a new `HIDDEN_FLAG(bool, openframe_mode_allow_unsafe, false, ...)` (declared alongside `tls_allow_unsafe` under `#ifndef NDEBUG`) that must be explicitly enabled to disable peer verification in openframe_mode. In release builds and by default in debug builds, execution now falls through to the normal certificate pinning/verification logic below (server_certificate_file_, openssl_verify_path, openssl_certificate), matching the pattern used for `tls_allow_unsafe`. Risk: this changes default behavior for openframe_mode deployments — any existing production reliance on always_verify_peer(false) in openframe_mode will now perform full verification unless the new flag is set, which may break connectivity if server certs aren't properly configured; this is the intended security fix but should be validated against actual openframe deployment cert setups.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 genPasswordPolicy leaks CFQueryRef and can double-release CFArrayRef records on empty result

In genPasswordPolicy's uid loop, added CFRelease(query) (guarded by a null check) after the records-derived work completes, fixing the per-iteration CFQueryRef leak from ODQueryCreateWithNode. Also guarded the existing CFRelease(records) with a null check to avoid relying on CFRelease(nullptr) being a no-op, making the cleanup explicit and defensive without changing control flow or behavior otherwise.

🤖 Prompt for AI agents

In osquery/tables/system/darwin/password_policy.cpp around line 119, review and complete this code-review fix: genPasswordPolicy leaks CFQueryRef and can double-release CFArrayRef records on empty result.
What the draft fix changed: In genPasswordPolicy's uid loop, added `CFRelease(query)` (guarded by a null check) after the `records`-derived work completes, fixing the per-iteration CFQueryRef leak from `ODQueryCreateWithNode`. Also guarded the existing `CFRelease(records)` with a null check to avoid relying on CFRelease(nullptr) being a no-op, making the cleanup explicit and defensive without changing control flow or behavior otherwise.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 formatTimestampToDate wraps non-throwing gmtime/put_time in try/catch, contrary to Status-based error handling used elsewhere

In formatTimestampToDate (osquery/tables/system/windows/programs.cpp), removed the dead try/catch wrapper around std::gmtime/std::put_time and replaced it with an explicit null-check on the std::tm* returned by std::gmtime, returning "" when it is nullptr before calling std::put_time, thereby avoiding the undefined-behavior crash path described in the finding.

🤖 Prompt for AI agents

In osquery/tables/system/windows/programs.cpp around line 70, review and complete this code-review fix: formatTimestampToDate wraps non-throwing gmtime/put_time in try/catch, contrary to Status-based error handling used elsewhere.
What the draft fix changed: In `formatTimestampToDate` (osquery/tables/system/windows/programs.cpp), removed the dead try/catch wrapper around `std::gmtime`/`std::put_time` and replaced it with an explicit null-check on the `std::tm*` returned by `std::gmtime`, returning `""` when it is `nullptr` before calling `std::put_time`, thereby avoiding the undefined-behavior crash path described in the finding.
Verify the change is correct and complete; do not refactor unrelated code.

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