fix(adhoc-sweep-fixes): 21 review findings across 21 files - #40
fix(adhoc-sweep-fixes): 21 review findings across 21 files#40flamingo[bot] wants to merge 21 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| } | ||
|
|
||
| std::string output; | ||
| bool wroteAny = false; | ||
|
|
||
| for (auto i = starting; i < argc; i++) { | ||
| if (SQLITE_NULL == sqlite3_value_type(argv[i])) { | ||
| continue; | ||
| } | ||
|
|
||
| output.append(reinterpret_cast<const char*>(sqlite3_value_text(argv[i]))); | ||
|
|
||
| if (sep != "" && i + 1 < argc) { | ||
| if (wroteAny && sep != "") { | ||
| output.append(sep); | ||
| } | ||
|
|
||
| output.append(reinterpret_cast<const char*>(sqlite3_value_text(argv[i]))); | ||
| wroteAny = true; | ||
| } | ||
|
|
||
| // Give up if the output is so large it's length overflows int |
There was a problem hiding this comment.
🦩 🟠 concatFunc silently produces incorrect separator placement when trailing NULL args are skipped
In concatFunc (osquery/sql/sqlite_string.cpp), replaced the position-based separator lookahead (sep != "" && i + 1 < argc) with a wroteAny flag that tracks whether a value has actually been written to output. The separator is now prepended before appending a non-null value only if a previous non-null value was already written, so trailing (or interior) NULL args no longer cause a dangling/incorrect separator. E.g. concat_ws('-', 'a', NULL) now yields "a" instead of "a-", and concat_ws('-', NULL, 'a') yields "a" instead of "-a".
(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/sql/sqlite_string.cpp around line 196, review and complete this code-review fix: concatFunc silently produces incorrect separator placement when trailing NULL args are skipped.
What the draft fix changed: In concatFunc (osquery/sql/sqlite_string.cpp), replaced the position-based separator lookahead (`sep != "" && i + 1 < argc`) with a `wroteAny` flag that tracks whether a value has actually been written to `output`. The separator is now prepended before appending a non-null value only if a previous non-null value was already written, so trailing (or interior) NULL args no longer cause a dangling/incorrect separator. E.g. concat_ws('-', 'a', NULL) now yields "a" instead of "a-", and concat_ws('-', NULL, 'a') yields "a" instead of "-a".
_(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
| auto subscriber = ([this, ec]() { | ||
| auto msc = createSubscriptionContext(); | ||
| msc->path = ec->path + "/*"; | ||
| msc->category = "tmp"; |
There was a problem hiding this comment.
🦩 🟠 Lambda captures ec by reference and is executed asynchronously on a detached thread, risking use-after-free
In FileEventSubscriber::Callback, changed the lambda capture from [this, &ec] to [this, ec] so the FSEventsEventContextRef (a shared_ptr) is captured by value, giving the detached thread its own owning reference and eliminating the use-after-free risk of accessing ec->path after Callback returns.
🤖 Prompt for AI agents
In osquery/tables/events/darwin/file_events.cpp around line 90, review and complete this code-review fix: Lambda captures `ec` by reference and is executed asynchronously on a detached thread, risking use-after-free.
What the draft fix changed: In `FileEventSubscriber::Callback`, changed the lambda capture from `[this, &ec]` to `[this, ec]` so the `FSEventsEventContextRef` (a shared_ptr) is captured by value, giving the detached thread its own owning reference and eliminating the use-after-free risk of accessing `ec->path` after `Callback` returns.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
|
|
||
| } else if (!selected_gids.empty()) { | ||
| auto selected_gids = gid_it->second.getAll(EQUALS); | ||
|
|
There was a problem hiding this comment.
🦩 🟠 groups.cpp genGroups() shadows outer selected_gids variable inside the gid branch
Removed the redundant, shadowing local declaration auto selected_gids = gid_it->second.getAll(EQUALS); inside the else if (!selected_gids.empty()) branch of genGroups(), so the loop now uses the outer-scope selected_gids that was already computed and checked earlier in the function.
🤖 Prompt for AI agents
In osquery/tables/system/windows/groups.cpp around line 69, review and complete this code-review fix: groups.cpp genGroups() shadows outer selected_gids variable inside the gid branch.
What the draft fix changed: Removed the redundant, shadowing local declaration `auto selected_gids = gid_it->second.getAll(EQUALS);` inside the `else if (!selected_gids.empty())` branch of `genGroups()`, so the loop now uses the outer-scope `selected_gids` that was already computed and checked earlier in the function.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -24,10 +24,10 @@ constexpr auto kFullRegPath = | |||
| "\\Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\UserAssist"; | |||
|
|
|||
| // Get execution count | |||
There was a problem hiding this comment.
🦩 🟠 userassist executionNum() returns -1 cast to std::size_t (huge unsigned value) on error paths, propagated into INTEGER() column
Changed executionNum()'s return type from std::size_t to long long (osquery/tables/system/windows/userassist.cpp), so the error sentinel -1 is preserved as a genuine negative value instead of being implicitly converted to SIZE_MAX. Updated both return -1; statements to return -1LL; and changed the success path to return static_cast<long long>(count.get());. The caller (genUserAssist) is unaffected since auto count = executionNum(assist_data); now deduces long long, and INTEGER(count) correctly renders -1 on error paths instead of a bogus huge unsigned count.
🤖 Prompt for AI agents
In osquery/tables/system/windows/userassist.cpp around line 26, review and complete this code-review fix: userassist executionNum() returns -1 cast to std::size_t (huge unsigned value) on error paths, propagated into INTEGER() column.
What the draft fix changed: Changed executionNum()'s return type from `std::size_t` to `long long` (osquery/tables/system/windows/userassist.cpp), so the error sentinel `-1` is preserved as a genuine negative value instead of being implicitly converted to SIZE_MAX. Updated both `return -1;` statements to `return -1LL;` and changed the success path to `return static_cast<long long>(count.get());`. The caller (`genUserAssist`) is unaffected since `auto count = executionNum(assist_data);` now deduces `long long`, and `INTEGER(count)` correctly renders -1 on error paths instead of a bogus huge unsigned count.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| remaining_bytes -= static_cast<std::size_t>(bytes_read); | ||
| } | ||
|
|
||
| if (remaining_bytes != 0) { | ||
| return createError(Pidfile::Error::IOError); | ||
| } | ||
|
|
||
| return buffer; | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 pidfile_windows.cpp readFile ignores retry counter's remaining_bytes check and can return truncated buffer without error
In Pidfile::readFile (Windows), added a check after the retry loop: if (remaining_bytes != 0) { return createError(Pidfile::Error::IOError); } before return buffer;, matching the POSIX version's behavior and the suggested fix. This ensures a read failure or incomplete read (e.g. ReadFile failing on every retry, leaving remaining_bytes equal to buffer_size) results in an IOError instead of silently returning a zero-filled/truncated buffer.
(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/utils/pidfile/pidfile_windows.cpp around line 132, review and complete this code-review fix: pidfile_windows.cpp readFile ignores retry counter's remaining_bytes check and can return truncated buffer without error.
What the draft fix changed: In Pidfile::readFile (Windows), added a check after the retry loop: `if (remaining_bytes != 0) { return createError(Pidfile::Error::IOError); }` before `return buffer;`, matching the POSIX version's behavior and the suggested fix. This ensures a read failure or incomplete read (e.g. ReadFile failing on every retry, leaving remaining_bytes equal to buffer_size) results in an IOError instead of silently returning a zero-filled/truncated buffer.
_(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
| auto uri = "https://" + FLAGS_tls_hostname; | ||
|
|
||
| // Add the prefix "/tools/agent/fleetmdm-server" to all requests only if openframe mode is enabled | ||
| if (FLAGS_openframe_mode) { |
There was a problem hiding this comment.
🦩 🟠 TLS enroll hardcodes OpenFrame-specific URL path segment inline in shared plugin logic
In TLSEnrollPlugin::enroll() (plugins/remote/enroll/tls_enroll.cpp), removed the hardcoded literal "/tools/agent/fleetmdm-server" and the FLAGS_openframe_mode gate, replacing them with a new configurable FLAGS_enroll_tls_endpoint_prefix string CLI flag (empty by default, preserving standard enroll behavior). The DECLARE_bool(openframe_mode) declaration was also removed since it's no longer referenced in this file. This decouples core TLS enroll logic from the OpenFrame-specific magic string; the OpenFrame subsystem is expected to set --enroll_tls_endpoint_prefix=/tools/agent/fleetmdm-server at its own configuration layer. Unverified: whether openframe_mode flag is still defined/used elsewhere (e.g. in an openframe/ subsystem file) and whether that code needs updating to set the new flag instead — that is outside this single file's scope and would require a companion change there to fully preserve OpenFrame's existing behavior.
🤖 Prompt for AI agents
In plugins/remote/enroll/tls_enroll.cpp around line 78, review and complete this code-review fix: TLS enroll hardcodes OpenFrame-specific URL path segment inline in shared plugin logic.
What the draft fix changed: In `TLSEnrollPlugin::enroll()` (plugins/remote/enroll/tls_enroll.cpp), removed the hardcoded literal `"/tools/agent/fleetmdm-server"` and the `FLAGS_openframe_mode` gate, replacing them with a new configurable `FLAGS_enroll_tls_endpoint_prefix` string CLI flag (empty by default, preserving standard enroll behavior). The `DECLARE_bool(openframe_mode)` declaration was also removed since it's no longer referenced in this file. This decouples core TLS enroll logic from the OpenFrame-specific magic string; the OpenFrame subsystem is expected to set `--enroll_tls_endpoint_prefix=/tools/agent/fleetmdm-server` at its own configuration layer. Unverified: whether `openframe_mode` flag is still defined/used elsewhere (e.g. in an openframe/ subsystem file) and whether that code needs updating to set the new flag instead — that is outside this single file's scope and would require a companion change there to fully preserve OpenFrame's existing behavior.
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
| * SPDX-License-Identifier: (Apache-2.0 OR GPL-2.0-only) | ||
| */ | ||
|
|
||
| // Sanity check integration test for wmi_tpm_info |
There was a problem hiding this comment.
🦩 🟠 tpm_info integration test file header references wrong spec file (wmi_script_event_consumers.table)
Changed the stale header comment at line 10 in tests/integration/tables/tpm_info.cpp from "Spec file: specs/windows/wmi_script_event_consumers.table" to "Spec file: specs/windows/tpm_info.table", correcting the copy-paste error so the comment matches the actual table under test (tpm_info) referenced by the test class TpmInfo and its ValidationMap.
🤖 Prompt for AI agents
In tests/integration/tables/tpm_info.cpp around line 10, review and complete this code-review fix: tpm_info integration test file header references wrong spec file (wmi_script_event_consumers.table).
What the draft fix changed: Changed the stale header comment at line 10 in tests/integration/tables/tpm_info.cpp from "Spec file: specs/windows/wmi_script_event_consumers.table" to "Spec file: specs/windows/tpm_info.table", correcting the copy-paste error so the comment matches the actual table under test (tpm_info) referenced by the test class TpmInfo and its ValidationMap.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| @@ -10,7 +10,6 @@ | |||
| #include <osquery/utils/info/version.h> | |||
| #include <osquery/utils/conversions/split.h> | |||
|
|
|||
There was a problem hiding this comment.
🦩 🔵 Duplicate #include in version.cpp
Removed the duplicate #include <stdexcept> line near the top of version.cpp, leaving a single instance; no other lines changed.
🤖 Prompt for AI agents
In osquery/utils/info/version.cpp around line 12, review and complete this code-review fix: Duplicate #include <stdexcept> in version.cpp.
What the draft fix changed: Removed the duplicate `#include <stdexcept>` line near the top of version.cpp, leaving a single instance; no other lines changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 100 high — react 👍/👎 to teach the reviewer
| event_data.syscall_number, | ||
| *syscall_event_record); | ||
| if (!s.ok()) { | ||
| VLOG(1) << "Malformed AUDIT_SYSCALL event: " << status.getMessage(); | ||
| VLOG(1) << "Malformed AUDIT_SYSCALL event: " << s.getMessage(); | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔵 Stale status variable referenced in log message after being shadowed by a new local s in GetProcessIDs error path
In AuditProcessEventSubscriber::ProcessEvents, changed the log statement in the GetProcessIDs failure branch from status.getMessage() to s.getMessage(), so it now logs the message from the actual GetProcessIDs result (s) instead of the stale outer status variable set by the earlier IsThreadClone call.
🤖 Prompt for AI agents
In osquery/tables/events/linux/process_events.cpp around line 267, review and complete this code-review fix: Stale `status` variable referenced in log message after being shadowed by a new local `s` in GetProcessIDs error path.
What the draft fix changed: In `AuditProcessEventSubscriber::ProcessEvents`, changed the log statement in the `GetProcessIDs` failure branch from `status.getMessage()` to `s.getMessage()`, so it now logs the message from the actual `GetProcessIDs` result (`s`) instead of the stale outer `status` variable set by the earlier `IsThreadClone` call.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| def get_file_hash(path): | ||
| try: | ||
| hasher = hashlib.sha256() | ||
| input_file = open(path, "rb") | ||
| with open(path, "rb") as input_file: | ||
|
|
||
| while True: | ||
| buffer = input_file.read(1048576) | ||
| if not buffer: | ||
| break | ||
| while True: | ||
| buffer = input_file.read(1048576) | ||
| if not buffer: | ||
| break | ||
|
|
||
| hasher.update(buffer) | ||
| hasher.update(buffer) | ||
|
|
||
| return hasher.hexdigest() | ||
|
|
There was a problem hiding this comment.
🦩 🔵 downloader.py get_file_hash never closes the opened file handle
In get_file_hash, replaced input_file = open(path, "rb") with with open(path, "rb") as input_file: and indented the read loop under the context manager, ensuring the file handle is closed on both the success path and any exception raised during reading.
🤖 Prompt for AI agents
In tools/cmake/downloader.py around line 38, review and complete this code-review fix: downloader.py get_file_hash never closes the opened file handle.
What the draft fix changed: In `get_file_hash`, replaced `input_file = open(path, "rb")` with `with open(path, "rb") as input_file:` and indented the read loop under the context manager, ensuring the file handle is closed on both the success path and any exception raised during reading.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
Closes 21 review findings across 21 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
osquery/sql/sqlite_string.cpp:196ecby reference and is executed asynchronously on a detached thread, risking use-after-freeosquery/tables/events/darwin/file_events.cpp:90osquery/tables/system/windows/groups.cpp:69osquery/tables/system/windows/userassist.cpp:26osquery/utils/pidfile/pidfile_windows.cpp:132osquery/utils/system/uptime.cpp:9tools/analysis/profile.py:65osquery/filesystem/posix/fileops.cpp:249osquery/tables/events/darwin/user_interaction_events.cpp:34osquery/tables/system/linux/memory_map.cpp:23osquery/tables/system/windows/bitlocker_info.cpp:41osquery/tables/system/windows/disk_info.cpp:22osquery/tables/system/windows/shared_resources.cpp:24osquery/utils/pidfile/pidfile_posix.cpp:108osquery/worker/ipc/posix/tests/worker_ipc_channels_test.cpp:26plugins/database/rocksdb.cpp:366plugins/remote/enroll/tls_enroll.cpp:78tests/integration/tables/tpm_info.cpp:10osquery/utils/info/version.cpp:12statusvariable referenced in log message after being shadowed by a new localsin GetProcessIDs error pathosquery/tables/events/linux/process_events.cpp:267tools/cmake/downloader.py:38What 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-fc5dbadf3ab5Merging 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.