Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e50e090
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
426054e
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
5229c87
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
1f746d4
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
49df5b1
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
4ee6514
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
12d45bb
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
9a84eab
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
e3b4487
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
b7bc1ef
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
915300d
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
f800510
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
a6a8b37
fix(OSQUERY-002-2): 22 review findings across 13 files
flamingo[bot] Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion osquery/carver/carver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,8 @@ Status Carver::postCarve(const boost::filesystem::path& path) {
auto contUri = TLSRequestHelper::makeURI(FLAGS_carver_continue_endpoint);
Request<TLSTransport, JSONSerializer> contRequest(contUri);
contRequest.setOption("hostname", FLAGS_tls_hostname);
bool anyBlockFailed = false;
Status blockFailureStatus;
for (size_t i = 0; i < blkCount; i++) {
std::vector<char> block(FLAGS_carver_block_size, 0);
auto r = pFile.read(block.data(), FLAGS_carver_block_size);
Comment on lines 334 to 341

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 carve() swallows partial-block POST failures without aborting the upload

In Carver::postCarve, added anyBlockFailed bool and blockFailureStatus Status tracked inside the per-block continue-request loop; when contRequest.call(params) fails, these are now set instead of only logging and continueing. After the loop, if anyBlockFailed is true, the function now calls updateCarveValue(carveGuid_, "status", "DATA POST FAILED") and returns a failure Status (instead of unconditionally marking success), so Carver::carve()'s caller correctly sees the post as failed. Removed the stale "TODO: Error sending files." comment since it's now addressed. Behavior for the fully-successful path is unchanged.

(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/carver/carver.cpp around line 247, review and complete this code-review fix: carve() swallows partial-block POST failures without aborting the upload.
What the draft fix changed: In Carver::postCarve, added `anyBlockFailed` bool and `blockFailureStatus` Status tracked inside the per-block continue-request loop; when `contRequest.call(params)` fails, these are now set instead of only logging and `continue`ing. After the loop, if `anyBlockFailed` is true, the function now calls `updateCarveValue(carveGuid_, "status", "DATA POST FAILED")` and returns a failure `Status` (instead of unconditionally marking success), so `Carver::carve()`'s caller correctly sees the post as failed. Removed the stale "TODO: Error sending files." comment since it's now addressed. Behavior for the fully-successful path is unchanged.

_(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

Expand All @@ -349,15 +351,23 @@ Status Carver::postCarve(const boost::filesystem::path& path) {
params.add("request_id", requestId_);
params.add("data", base64::encode(std::string(block.begin(), block.end())));

// TODO: Error sending files.
status = contRequest.call(params);
if (!status.ok()) {
VLOG(1) << "Post of carved block " << i
<< " failed: " << status.getMessage();
anyBlockFailed = true;
blockFailureStatus = status;
continue;
}
}

if (anyBlockFailed) {
updateCarveValue(carveGuid_, "status", "DATA POST FAILED");
return Status(1,
"Failed to post one or more carved blocks: " +
blockFailureStatus.getMessage());
}

updateCarveValue(carveGuid_, "status", kCarverStatusSuccess);
return Status::success();
};
Expand All @@ -369,3 +379,4 @@ void scheduleCarves() {
}
}
} // namespace osquery

19 changes: 12 additions & 7 deletions osquery/database/database.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ Status getDatabaseValue(const std::string& domain,

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot get database value: " + key);
return Status::failure("Cannot get database value: " + key);
} else {
auto plugin = getDatabasePlugin();
return plugin->get(domain, key, value);
Comment on lines 300 to 306

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Comment on lines 300 to 306

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 getDatabaseValue(int&) calls std::stoi without validating or catching exceptions

In getDatabaseValue(domain, key, int&), replaced the unguarded std::stoi(result) call with tryTo<int>(result) (already used elsewhere in this file, e.g. upgradeDatabase), checking ret.isError() and returning Status::failure("Invalid integer value for key: " + key) instead of letting an exception propagate; on success it assigns ret.get() to value. This mirrors the existing error-handling pattern in upgradeDatabase() in the same file. Risk: callers previously relying on std::stoi's leading-whitespace/partial-parse tolerance (e.g. "42abc" parsing as 42) will now get a failure Status instead — behavior differs slightly from the original permissive parsing, but this is intentional per the finding's request to validate the string.

🤖 Prompt for AI agents 
In osquery/database/database.cpp around line 296, review and complete this code-review fix: getDatabaseValue(int&) calls std::stoi without validating or catching exceptions.
What the draft fix changed: In `getDatabaseValue(domain, key, int&)`, replaced the unguarded `std::stoi(result)` call with `tryTo<int>(result)` (already used elsewhere in this file, e.g. `upgradeDatabase`), checking `ret.isError()` and returning `Status::failure("Invalid integer value for key: " + key)` instead of letting an exception propagate; on success it assigns `ret.get()` to `value`. This mirrors the existing error-handling pattern in `upgradeDatabase()` in the same file. Risk: callers previously relying on `std::stoi`'s leading-whitespace/partial-parse tolerance (e.g. "42abc" parsing as 42) will now get a failure Status instead — behavior differs slightly from the original permissive parsing, but this is intentional per the finding's request to validate the string.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -313,7 +313,11 @@ Status getDatabaseValue(const std::string& domain,
std::string result;
auto s = getDatabaseValue(domain, key, result);
if (s.ok()) {
value = std::stoi(result);
auto ret = tryTo<int>(result);
if (ret.isError()) {
return Status::failure("Invalid integer value for key: " + key);
}
value = ret.get();
}
return s;
}
Comment on lines 313 to 323

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Expand Down Expand Up @@ -342,7 +346,7 @@ Status setDatabaseBatch(const std::string& domain,

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot set database values");
return Status::failure("Cannot set database values");
}

auto plugin = getDatabasePlugin();
Comment on lines 346 to 352

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

Expand Down Expand Up @@ -370,7 +374,7 @@ Status deleteDatabaseValue(const std::string& domain, const std::string& key) {

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot delete database value: " + key);

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

return Status::failure("Cannot delete database value: " + key);
} else {
auto plugin = getDatabasePlugin();
return plugin->remove(domain, key);
Expand All @@ -396,8 +400,8 @@ Status deleteDatabaseRange(const std::string& domain,

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot delete database values: " + low + " - " +
high);
return Status::failure("Cannot delete database values: " + low + " - " +
high);
} else {
auto plugin = getDatabasePlugin();

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 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

return plugin->removeRange(domain, low, high);
Expand Down Expand Up @@ -439,7 +443,7 @@ Status scanDatabaseKeys(const std::string& domain,

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot scan database values: " + prefix);
return Status::failure("Cannot scan database values: " + prefix);
} else {
auto plugin = getDatabasePlugin();
return plugin->scan(domain, keys, prefix, max);
Expand Down Expand Up @@ -751,3 +755,4 @@ IDatabaseInterface& getOsqueryDatabase() {
return osquery_database;
}
} // namespace osquery

21 changes: 11 additions & 10 deletions osquery/events/linux/bpf/systemstatetracker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ SystemStateTracker::Ref SystemStateTracker::create() {
IProcessContextFactory::Ref process_context_factory;

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 SystemStateTracker::create() throws Status instead of returning it

In SystemStateTracker::create() (the no-argument overload), replaced throw status; on IProcessContextFactory::create() failure with a Status-based check that logs the error and returns nullptr, matching the pattern already used in the create(Ref) overload. No exception is thrown anymore for this fallible path.

🤖 Prompt for AI agents 
In osquery/events/linux/bpf/systemstatetracker.cpp around line 42, review and complete this code-review fix: SystemStateTracker::create() throws Status instead of returning it.
What the draft fix changed: In `SystemStateTracker::create()` (the no-argument overload), replaced `throw status;` on `IProcessContextFactory::create()` failure with a Status-based check that logs the error and returns `nullptr`, matching the pattern already used in the `create(Ref)` overload. No exception is thrown anymore for this fallible path.
Verify the change is correct and complete; do not refactor unrelated code.

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

auto status = IProcessContextFactory::create(process_context_factory);
if (!status) {
throw status;
LOG(ERROR) << "Failed to create the state tracker: " << status.getMessage();
return nullptr;
}

return create(std::move(process_context_factory));
Expand All @@ -51,12 +52,16 @@ SystemStateTracker::Ref SystemStateTracker::create() {
SystemStateTracker::Ref SystemStateTracker::create(
IProcessContextFactory::Ref process_context_factory) {
try {
return SystemStateTracker::Ref(
std::unique_ptr<SystemStateTracker> tracker(
new SystemStateTracker(std::move(process_context_factory)));

} catch (const Status& status) {
LOG(ERROR) << "Failed to create the state tracker: " << status.getMessage();
return nullptr;
auto status = tracker->restart();
if (!status.ok()) {
LOG(ERROR) << "Failed to create the state tracker: " << status.getMessage();
return nullptr;
}

return SystemStateTracker::Ref(tracker.release());

} catch (const std::bad_alloc&) {
return nullptr;
Expand Down Expand Up @@ -271,11 +276,6 @@ SystemStateTracker::SystemStateTracker(
: d(new PrivateData) {
d->last_expiration = getUnixTime();
d->process_context_factory = std::move(process_context_factory);

auto status = restart();
if (!status.ok()) {
throw status;
}
}

ProcessContext& SystemStateTracker::getProcessContext(
Comment on lines 276 to 281

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 SystemStateTracker private constructor throws Status on restart failure

Removed the throw status; from the private SystemStateTracker constructor entirely (constructor no longer calls restart() or can fail). The restart() call and its Status check were moved into SystemStateTracker::create(IProcessContextFactory::Ref), which now constructs the object via std::unique_ptr, calls restart(), checks .ok(), logs and returns nullptr on failure, otherwise releases and returns the Ref. This eliminates the exception-throwing constructor path while preserving equivalent behavior (failure to restart still yields a nullptr tracker from create). The catch (const Status&) clause was removed from create(Ref) since no code path now throws Status; catch (const std::bad_alloc&) is retained for the new allocation. Risk: any other internal callers of the private constructor (none visible in this file) that relied on construction-time restart-failure exceptions would need updating, but none exist in this translation unit.

🤖 Prompt for AI agents 
In osquery/events/linux/bpf/systemstatetracker.cpp around line 261, review and complete this code-review fix: SystemStateTracker private constructor throws Status on restart failure.
What the draft fix changed: Removed the `throw status;` from the private `SystemStateTracker` constructor entirely (constructor no longer calls `restart()` or can fail). The `restart()` call and its Status check were moved into `SystemStateTracker::create(IProcessContextFactory::Ref)`, which now constructs the object via `std::unique_ptr`, calls `restart()`, checks `.ok()`, logs and returns `nullptr` on failure, otherwise releases and returns the `Ref`. This eliminates the exception-throwing constructor path while preserving equivalent behavior (failure to restart still yields a `nullptr` tracker from `create`). The `catch (const Status&)` clause was removed from `create(Ref)` since no code path now throws `Status`; `catch (const std::bad_alloc&)` is retained for the `new` allocation. Risk: any other internal callers of the private constructor (none visible in this file) that relied on construction-time restart-failure exceptions would need updating, but none exist in this translation unit.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand Down Expand Up @@ -1357,3 +1357,4 @@ SystemStateTracker::Context SystemStateTracker::getContextCopy() const {
}

} // namespace osquery

22 changes: 15 additions & 7 deletions osquery/events/windows/evtsubscription.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,18 @@ Status EvtSubscription::create(EvtSubscription::Ref& obj,
obj.reset();

try {
obj.reset(new EvtSubscription(channel));
auto obj_ptr = std::unique_ptr<EvtSubscription>(new EvtSubscription(channel));

auto status = obj_ptr->init();
if (!status.ok()) {
return status;
}

obj = std::move(obj_ptr);
return Status::success();

} catch (const std::bad_alloc&) {
return Status::failure("Memory allocation failure");

} catch (const Status& status) {
return status;
}
}

Expand Down Expand Up @@ -96,7 +100,10 @@ EvtSubscription::EventList EvtSubscription::getEvents() {
EvtSubscription::EvtSubscription(const std::string& channel)
: d_(new PrivateData) {
d_->channel = channel;
auto channel_utf16 = stringToWstring(channel);
}

Status EvtSubscription::init() {
auto channel_utf16 = stringToWstring(d_->channel);

auto subscription = EvtSubscribe(nullptr,
nullptr,
Expand All @@ -109,11 +116,12 @@ EvtSubscription::EvtSubscription(const std::string& channel)

if (subscription == nullptr) {
auto error = GetLastError();
throw Status::failure("Failed to subscribe to the channel named " +
channel + ". Error " + std::to_string(error));
return Status::failure("Failed to subscribe to the channel named " +
d_->channel + ". Error " + std::to_string(error));
}

d_->handle = subscription;
return Status::success();
}

void EvtSubscription::processEvent(EVT_HANDLE event) {
Comment on lines 116 to 127

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 EvtSubscription constructor throws Status instead of returning it

In osquery/events/windows/evtsubscription.cpp, refactored EvtSubscription to two-phase initialization: the constructor now only stores the channel name (no longer performs EvtSubscribe or throws), and a new Status EvtSubscription::init() method performs the EvtSubscribe call and returns Status::failure(...) instead of throwing. EvtSubscription::create() was updated to construct the object with new EvtSubscription(channel) wrapped in std::unique_ptr, call init(), check its Status explicitly, and only assign to the output Ref on success; the catch (const Status&) clause was removed since Status is no longer thrown. This requires a corresponding declaration of Status init(); (and access to d_) to be added in osquery/events/windows/evtsubscription.h, which is not visible/editable here — the header must declare init() as a member (private or public) for this to compile, so this change is INCOMPLETE without that header edit. Risk: if the header cannot be modified to match, this file will fail to build.

🤖 Prompt for AI agents 
In osquery/events/windows/evtsubscription.cpp around line 108, review and complete this code-review fix: EvtSubscription constructor throws Status instead of returning it.
What the draft fix changed: In `osquery/events/windows/evtsubscription.cpp`, refactored `EvtSubscription` to two-phase initialization: the constructor now only stores the channel name (no longer performs `EvtSubscribe` or throws), and a new `Status EvtSubscription::init()` method performs the `EvtSubscribe` call and returns `Status::failure(...)` instead of throwing. `EvtSubscription::create()` was updated to construct the object with `new EvtSubscription(channel)` wrapped in `std::unique_ptr`, call `init()`, check its Status explicitly, and only assign to the output `Ref` on success; the `catch (const Status&)` clause was removed since Status is no longer thrown. This requires a corresponding declaration of `Status init();` (and access to `d_`) to be added in `osquery/events/windows/evtsubscription.h`, which is not visible/editable here — the header must declare `init()` as a member (private or public) for this to compile, so this change is INCOMPLETE without that header edit. Risk: if the header cannot be modified to match, this file will fail to build.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Expand Down
2 changes: 1 addition & 1 deletion osquery/events/windows/ntfs_event_publisher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ Status NTFSEventPublisher::getPathFromReferenceNumber(
buffer.resize(required_characters);
if (buffer.size() != required_characters) {
::CloseHandle(handle);
throw std::bad_alloc();
return Status::failure("Failed to allocate buffer for path resolution");
}

auto bytes_returned = static_cast<size_t>(
Comment on lines 251 to 257

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 std::bad_alloc thrown instead of Status::failure in getPathFromReferenceNumber

In getPathFromReferenceNumber, replaced throw std::bad_alloc(); with return Status::failure("Failed to allocate buffer for path resolution"); at the resize-failure guard following buffer.resize(required_characters), matching the suggested fix exactly and preserving the existing ::CloseHandle(handle); call before returning.

(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 224, review and complete this code-review fix: std::bad_alloc thrown instead of Status::failure in getPathFromReferenceNumber.
What the draft fix changed: In `getPathFromReferenceNumber`, replaced `throw std::bad_alloc();` with `return Status::failure("Failed to allocate buffer for path resolution");` at the resize-failure guard following `buffer.resize(required_characters)`, matching the suggested fix exactly and preserving the existing `::CloseHandle(handle);` call before returning.

_(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

Comment on lines 251 to 257

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down
14 changes: 11 additions & 3 deletions osquery/remote/transports/tls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ HIDDEN_FLAG(bool,
tls_allow_unsafe,
false,
"Allow TLS server certificate trust failures");

HIDDEN_FLAG(bool,
openframe_mode_allow_unsafe,
false,
"Allow disabling TLS peer verification in openframe_mode");
#endif

HIDDEN_FLAG(bool,
Expand Down Expand Up @@ -111,11 +116,13 @@ http::Client::Options TLSTransport::getOptions() {

options.follow_redirects(true).timeout(16);

if (FLAGS_openframe_mode) {
#ifndef NDEBUG
if (FLAGS_openframe_mode && FLAGS_openframe_mode_allow_unsafe) {
options.always_verify_peer(false);
return options;
}

}
#endif

options.always_verify_peer(verify_peer_);
if (server_certificate_file_.size() > 0) {
if (!osquery::isReadable(server_certificate_file_).ok()) {
Comment on lines 116 to 128

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down Expand Up @@ -308,3 +315,4 @@ Status TLSTransport::sendRequest(const std::string& params, bool compress) {
return response_status_;
}
} // namespace osquery

39 changes: 25 additions & 14 deletions osquery/remote/uri.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,16 @@ static inline void toLower(String& s) {
}
}

Uri::Uri() : hasAuthority_(false), port_(0) {}

Uri::Uri(const std::string& str) : hasAuthority_(false), port_(0) {
auto status = Uri::parse(str, *this);
if (!status.ok()) {
throw std::invalid_argument(status.getMessage());
}
}

Status Uri::parse(const std::string& str, Uri& uri) {
static const std::regex uriRegex(
"([a-zA-Z][a-zA-Z0-9+.-]*):" // scheme:
"([^?#]*)" // authority and path
Expand All @@ -40,19 +49,19 @@ Uri::Uri(const std::string& str) : hasAuthority_(false), port_(0) {

std::smatch match;
if (!std::regex_match(str, match, uriRegex)) {

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Uri constructor throws std::invalid_argument instead of returning osquery::Status

Added a static Uri::parse(const std::string&, Uri&) factory method in uri.cpp that performs the parsing logic and returns Status::failure("Invalid URL") instead of throwing at the first std::regex_match check. The existing Uri(const std::string&) constructor is preserved for backward compatibility and now delegates to parse(), rethrowing std::invalid_argument only if parsing fails, so existing callers relying on the throwing constructor still work. NOTE: this requires corresponding declarations of Uri() (default constructor) and static Status parse(const std::string&, Uri&) to be added to osquery/remote/uri.h, which is not shown/editable here — the header must declare these members and make parse a friend or use public setters for this to compile, since parse currently accesses private members (scheme_, path_, etc.) directly, implying it must be declared as a static member function in the class. This is the main risk: without the header change, this file will not compile.

🤖 Prompt for AI agents 
In osquery/remote/uri.cpp around line 42, review and complete this code-review fix: Uri constructor throws std::invalid_argument instead of returning osquery::Status.
What the draft fix changed: Added a static `Uri::parse(const std::string&, Uri&)` factory method in uri.cpp that performs the parsing logic and returns `Status::failure("Invalid URL")` instead of throwing at the first `std::regex_match` check. The existing `Uri(const std::string&)` constructor is preserved for backward compatibility and now delegates to `parse()`, rethrowing `std::invalid_argument` only if parsing fails, so existing callers relying on the throwing constructor still work. NOTE: this requires corresponding declarations of `Uri()` (default constructor) and `static Status parse(const std::string&, Uri&)` to be added to `osquery/remote/uri.h`, which is not shown/editable here — the header must declare these members and make `parse` a friend or use public setters for this to compile, since `parse` currently accesses private members (`scheme_`, `path_`, etc.) directly, implying it must be declared as a static member function in the class. This is the main risk: without the header change, this file will not compile.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

throw std::invalid_argument("Invalid URL");
return Status::failure("Invalid URL");
}

scheme_ = submatch(match, 1);
toLower(scheme_);
uri.scheme_ = submatch(match, 1);
toLower(uri.scheme_);

std::string authorityAndPath(match[2].first, match[2].second);
std::smatch authorityAndPathMatch;
if (!std::regex_match(
authorityAndPath, authorityAndPathMatch, authorityAndPathRegex)) {
// Does not start with //, doesn't have authority
hasAuthority_ = false;
path_ = authorityAndPath;
uri.hasAuthority_ = false;
uri.path_ = authorityAndPath;
} else {
static const std::regex authorityRegex(
"(?:([^@:]*)(?::([^@]*))?@)?" // username, password
Comment on lines 49 to 67

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🔴 Uri constructor throws std::invalid_argument for invalid authority

The second throw at the authority-parsing regex mismatch was replaced with return Status::failure("Invalid URI authority") inside the new Uri::parse static method, consistent with finding 1's fix. Same header-dependency risk applies: the change is only complete once uri.h declares Uri::parse as a static member returning Status and exposes a default constructor, and callers of the old throwing constructor are migrated (not done here, out of scope for this single-file fix) to use Uri::parse for full convention compliance.

🤖 Prompt for AI agents 
In osquery/remote/uri.cpp around line 62, review and complete this code-review fix: Uri constructor throws std::invalid_argument for invalid authority.
What the draft fix changed: The second throw at the authority-parsing regex mismatch was replaced with `return Status::failure("Invalid URI authority")` inside the new `Uri::parse` static method, consistent with finding 1's fix. Same header-dependency risk applies: the change is only complete once `uri.h` declares `Uri::parse` as a static member returning `Status` and exposes a default constructor, and callers of the old throwing constructor are migrated (not done here, out of scope for this single-file fix) to use `Uri::parse` for full convention compliance.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

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

Expand All @@ -66,26 +75,28 @@ Uri::Uri(const std::string& str) : hasAuthority_(false), port_(0) {
authority.second,
authorityMatch,
authorityRegex)) {
throw std::invalid_argument("Invalid URI authority");
return Status::failure("Invalid URI authority");
}

std::string port(authorityMatch[4].first, authorityMatch[4].second);
if (!port.empty()) {
int iport = std::stoi(port);
if (iport < UINT16_MAX && iport >= 0) {
port_ = static_cast<uint16_t>(iport);
uri.port_ = static_cast<uint16_t>(iport);
}
}

hasAuthority_ = true;
username_ = submatch(authorityMatch, 1);
password_ = submatch(authorityMatch, 2);
host_ = submatch(authorityMatch, 3);
path_ = submatch(authorityAndPathMatch, 2);
uri.hasAuthority_ = true;
uri.username_ = submatch(authorityMatch, 1);
uri.password_ = submatch(authorityMatch, 2);
uri.host_ = submatch(authorityMatch, 3);
uri.path_ = submatch(authorityAndPathMatch, 2);
}

query_ = submatch(match, 3);
fragment_ = submatch(match, 4);
uri.query_ = submatch(match, 3);
uri.fragment_ = submatch(match, 4);

return Status::success();
}

std::string Uri::authority() const {
Expand Down
7 changes: 6 additions & 1 deletion osquery/tables/system/darwin/password_policy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,12 @@ QueryData genPasswordPolicy(QueryContext& context) {
CFRelease(user_policy);
}
}
CFRelease(records);
if (records != nullptr) {
CFRelease(records);
}
if (query != nullptr) {
CFRelease(query);
}
CFRelease(uid_string);
}

Comment on lines 136 to 147

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down
2 changes: 0 additions & 2 deletions osquery/tables/system/tests/posix/ssh_keys_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@

#include <gtest/gtest.h>

#include <boost/filesystem.hpp>
#include <gtest/gtest.h>
#include <osquery/sql/sql.h>
#include <osquery/tables/system/posix/ssh_keys.h>
#include <osquery/utils/scope_guard.h>
Comment on lines 13 to 18

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down
18 changes: 9 additions & 9 deletions osquery/tables/system/windows/programs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,17 +68,16 @@ std::string findSelfClosingTag(const std::string& xml,

// Convert a Unix timestamp to a date in YYYYMMDD format
std::string formatTimestampToDate(time_t timestamp) {

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

try {
// Convert the timestamp to a tm structure
std::tm* timeInfo = std::gmtime(&timestamp);

// Format the date as YYYYMMDD
std::ostringstream oss;
oss << std::put_time(timeInfo, "%Y%m%d");
return oss.str();
} catch (...) {
// Convert the timestamp to a tm structure
std::tm* timeInfo = std::gmtime(&timestamp);
if (timeInfo == nullptr) {
return "";
}

// Format the date as YYYYMMDD
std::ostringstream oss;
oss << std::put_time(timeInfo, "%Y%m%d");
return oss.str();
}

std::string packageFamilyNameFromPackageFullName(
Expand Down Expand Up @@ -457,3 +456,4 @@ QueryData genPrograms(QueryContext& context) {
}
} // namespace tables
} // namespace osquery

Loading