fix(adhoc-sweep-fixes): CU-17tkuw5wp49 38 review findings across 30 files

[flamingo]

Closes 38 review findings across 30 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	compressed_data size check compares against pointer size, not struct size	osquery/tables/system/windows/prefetch.cpp:349
2	🟢 95 high	data size check uses sizeof(pointer typedef) instead of sizeof(struct) for file header	osquery/tables/system/windows/prefetch.cpp:369
3	🟢 90 high	parseFileInfo uses v30v2.OtherRunTimes for Windows 8 instead of v26.OtherRunTimes	osquery/tables/system/windows/prefetch.cpp:170
4	🟢 92 high	getTLSCertificate leaks the socket descriptor on early-return error paths before the scope guard is installed	osquery/tables/networking/curl_certificate.cpp:363
5	🟢 90 high	Macro-style constant DEFAULT_READ_TIMEOUT violates kPascalCase naming convention	osquery/tables/networking/curl_certificate.cpp:45
6	🟢 95 high	openframe_token_extractor.h lacks the required copyright/SPDX header	openframe/openframe_token_extractor.h:1
7	🟢 97 high	windows_firewall_rules_tests.cpp mutates the shared base rule 'r' instead of 'rule' in several test cases, producing incorrect test fixtures	osquery/tables/networking/tests/windows/windows_firewall_rules_tests.cpp:46
8	🟡 85 medium	NonBlockingFStream::getline mutates offset_ and buffer_ outside the fd_mutex_ lock	osquery/events/linux/syslog.cpp:74
9	🟡 85 medium	exclude_paths_.find() result discarded as boolean instead of comparing against end()	osquery/events/darwin/fsevents.cpp:352
10	🟢 90 high	gen_diff output_file construction uses Python 2 style bytes handling for subprocess output	tools/codegen/genapi.py:277
11	🟢 95 high	Dead comparison branch uses misspelled key "quarantime_timestamp" instead of "quarantine_timestamp"	osquery/tables/system/tests/darwin/extended_attributes_tests.cpp:159
12	🟢 95 high	IsValidSid() failure path does not return nullptr, continues to use invalid SID	osquery/utils/system/windows/users_groups_helpers.cpp:84
13	🟡 60 medium	shouldFire() always returns true, nullifying the sc->physical_disks filter logic and dead code path	osquery/events/darwin/diskarbitration.cpp:144
14	🟢 90 high	Seccomp arch/syscall symbolic decoding uses field iteration order, not guaranteed arch-before-syscall	osquery/tables/events/linux/seccomp_events.cpp:76
15	🟢 92 high	iptc_proxy.c get_chain_data does not clear policy/policy_data when chain has no policy	osquery/tables/networking/linux/iptc_proxy.c:57
16	🔴 40 low — review closely	MockMD test never asserts on getDrivesForArray output	osquery/tables/system/tests/linux/md_tables_tests.cpp:171
17	🟡 85 medium	moveFiles loop underflows size_t when max_files is 0 or when offsets interact badly	plugins/logger/filesystem_logger.cpp:165
18	🔴 55 low — review closely	process() swallows OpenframeAuthorizationManagerProvider singleton exceptions but holds no lock around running_ check in the refresh loop	openframe/openframe_token_refresher.cpp:17
19	🟢 95 high	original_mnt_fd_ guarded by '> 0' check skips closing fd 0	osquery/worker/ipc/linux/linux_table_container_ipc.cpp:153
20	🟢 95 high	decompress() reports wrong file ('in') in its outFile-open error message	osquery/filesystem/file_compression.cpp:108
21	🟡 80 medium	archive() ignores return codes of archive_write_header/archive_write_data and leaks entry/arch on error paths	osquery/filesystem/file_compression.cpp:180
22	🟢 90 high	extensionPathActive() ignores the Status returned by client.ping() and always treats a successful connection as success	osquery/extensions/extensions.cpp:181
23	🔴 20 low — review closely	extensionPathActive() relies on try/catch around ExtensionManagerClient instead of Status propagation	osquery/extensions/extensions.cpp:180
24	🟢 90 high	utf_converter::from_bytes under-allocates and under-estimates buffer sizes for MultiByteToWideChar, risking buffer overrun or corrupt truncation	osquery/utils/conversions/windows/strings.cpp:26
25	🟢 90 high	to_bytes() does not check WideCharToMultiByte return value before resizing, underflow on failure	osquery/utils/conversions/windows/strings.cpp:38
26	🟢 95 high	Stray empty statement after static member initialization	osquery/core/windows/global_users_groups_cache.cpp:20
27	🔴 55 low — review closely	Dereferencing cache pointer after shutdown without checking future validity	osquery/core/windows/global_users_groups_cache.cpp:25
28	🟡 85 medium	StopEventLoop busy-waits on hasStarted() with no timeout, risking test hangs	osquery/events/tests/linux/inotify_tests.cpp:82
29	🟡 75 medium	fopen() result not checked for NULL before use in StartEventLoop and TriggerEvent	osquery/events/tests/linux/inotify_tests.cpp:75
30	🟢 97 high	Python 2 except syntax breaks execution on Python 3	tools/tests/test_windows_service.py:101
31	🟢 97 high	gtest_prod.h header included twice in packs.h	osquery/config/packs.h:17
32	🟢 95 high	dynamic_pointer_cast cannot throw std::bad_cast but code catches it as if it could	osquery/tables/yara/yara_events.cpp:156
33	🟢 95 high	sqliteCidrBlockFunc leaves result unset when address is neither v4 nor v6	osquery/sql/sqlite_network.cpp:48
34	🟢 90 high	NormalizePath reads path[0]/cwd[0] without checking for empty strings, risking OOB read	osquery/tables/events/linux/process_file_events.cpp:202
35	🔴 55 low — review closely	initialize() returns success when the process IS WoW64, inverted condition	osquery/tables/system/windows/security_profile_info_utils.cpp:51
36	🟢 92 high	assert() used for release-mode safety check before unchecked map lookup result use	osquery/tables/yara/windows/yara_events.cpp:142
37	🟢 90 high	genLoadAverage leaves results[i] default-constructed on getloadavg failure	osquery/tables/system/posix/load_average.cpp:26

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: cb1fa104-eac6-46a7-aba8-90d794c59386

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-17tkuw5wp49 Osquery review findings sweep (8 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

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

[flamingo]

🦩 🔴 compressed_data size check compares against pointer size, not struct size

In parsePrefetch, changed compressed_data.size() < sizeof(PPREFETCH_COMPRESSED_HEADER) to sizeof(PREFETCH_COMPRESSED_HEADER) so the size check validates against the actual struct size rather than a pointer size, preventing out-of-bounds reads when dereferencing compressed_header.

🤖 Prompt for AI agents

In osquery/tables/system/windows/prefetch.cpp around line 349, review and complete this code-review fix: compressed_data size check compares against pointer size, not struct size.
What the draft fix changed: In `parsePrefetch`, changed `compressed_data.size() < sizeof(PPREFETCH_COMPRESSED_HEADER)` to `sizeof(PREFETCH_COMPRESSED_HEADER)` so the size check validates against the actual struct size rather than a pointer size, preventing out-of-bounds reads when dereferencing `compressed_header`.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 data size check uses sizeof(pointer typedef) instead of sizeof(struct) for file header

In parsePrefetch, changed data.size() < sizeof(PPREFETCH_FILE_HEADER) to sizeof(PREFETCH_FILE_HEADER) so the buffer is validated against the actual struct size before parsePrefetchData dereferences prefetch_header->Signature and computes pointer arithmetic using sizeof(PREFETCH_FILE_HEADER).

🤖 Prompt for AI agents

In osquery/tables/system/windows/prefetch.cpp around line 369, review and complete this code-review fix: data size check uses sizeof(pointer typedef) instead of sizeof(struct) for file header.
What the draft fix changed: In `parsePrefetch`, changed `data.size() < sizeof(PPREFETCH_FILE_HEADER)` to `sizeof(PREFETCH_FILE_HEADER)` so the buffer is validated against the actual struct size before `parsePrefetchData` dereferences `prefetch_header->Signature` and computes pointer arithmetic using `sizeof(PREFETCH_FILE_HEADER)`.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 parseFileInfo uses v30v2.OtherRunTimes for Windows 8 instead of v26.OtherRunTimes

In parseFileInfo, under the kPrefetchVersionWindows8 case, changed prefetch_file_info->ext.v30v2.OtherRunTimes to prefetch_file_info->ext.v26.OtherRunTimes so the loop consistently reads from the same union member (v26) used for LastRunTime and RunCount in that branch.

🤖 Prompt for AI agents

In osquery/tables/system/windows/prefetch.cpp around line 170, review and complete this code-review fix: parseFileInfo uses v30v2.OtherRunTimes for Windows 8 instead of v26.OtherRunTimes.
What the draft fix changed: In `parseFileInfo`, under the `kPrefetchVersionWindows8` case, changed `prefetch_file_info->ext.v30v2.OtherRunTimes` to `prefetch_file_info->ext.v26.OtherRunTimes` so the loop consistently reads from the same union member (`v26`) used for `LastRunTime` and `RunCount` in that branch.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 getTLSCertificate leaks the socket descriptor on early-return error paths before the scope guard is installed

In getTLSCertificate, moved the scope_guard::create for sock to immediately after the successful socket(...) call (before the timeout > 0 / setsockopt block and before connect), so that early returns from the setsockopt failure branch and the connect failure branch now run the guard and close the socket, eliminating the fd leak. The guard lambda body is unchanged.

🤖 Prompt for AI agents

In osquery/tables/networking/curl_certificate.cpp around line 363, review and complete this code-review fix: getTLSCertificate leaks the socket descriptor on early-return error paths before the scope guard is installed.
What the draft fix changed: In `getTLSCertificate`, moved the `scope_guard::create` for `sock` to immediately after the successful `socket(...)` call (before the `timeout > 0` / `setsockopt` block and before `connect`), so that early returns from the `setsockopt` failure branch and the `connect` failure branch now run the guard and close the socket, eliminating the fd leak. The guard lambda body is unchanged.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 Macro-style constant DEFAULT_READ_TIMEOUT violates kPascalCase naming convention

Replaced #define DEFAULT_READ_TIMEOUT 4 with static constexpr int kDefaultReadTimeout = 4; at the top of the osquery::tables namespace, and updated all three usages (genTLSCertificate's default timeout initialization, and both takeOr/out-of-range fallback assignments) to reference kDefaultReadTimeout instead of the macro.

🤖 Prompt for AI agents

In osquery/tables/networking/curl_certificate.cpp around line 45, review and complete this code-review fix: Macro-style constant DEFAULT_READ_TIMEOUT violates kPascalCase naming convention.
What the draft fix changed: Replaced `#define DEFAULT_READ_TIMEOUT 4` with `static constexpr int kDefaultReadTimeout = 4;` at the top of the `osquery::tables` namespace, and updated all three usages (`genTLSCertificate`'s default `timeout` initialization, and both `takeOr`/out-of-range fallback assignments) to reference `kDefaultReadTimeout` instead of the macro.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 sqliteCidrBlockFunc leaves result unset when address is neither v4 nor v6

Added an else branch to the if/else-if chain in sqliteCidrBlockFunc (osquery/sql/sqlite_network.cpp) that calls sqlite3_result_error and returns when ipaddr is neither v4 nor v6, ensuring the sqlite3_context result is always set before the function exits.

🤖 Prompt for AI agents

In osquery/sql/sqlite_network.cpp around line 48, review and complete this code-review fix: sqliteCidrBlockFunc leaves result unset when address is neither v4 nor v6.
What the draft fix changed: Added an `else` branch to the if/else-if chain in `sqliteCidrBlockFunc` (osquery/sql/sqlite_network.cpp) that calls `sqlite3_result_error` and returns when `ipaddr` is neither v4 nor v6, ensuring the sqlite3_context result is always set before the function exits.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 NormalizePath reads path[0]/cwd[0] without checking for empty strings, risking OOB read

In NormalizePath() (anonymous namespace, osquery/tables/events/linux/process_file_events.cpp), added explicit !path.empty() and !cwd.empty() checks before indexing path[0]/cwd[0], and guarded the substr(1, size - 2) calls with a size() >= 2 check (falling back to using the string as-is when it is only a single " character or shorter) to prevent the size_t underflow that could throw std::out_of_range/std::length_error. Also changed translated_path[0] != '/' to translated_path.empty() || translated_path[0] != '/' to avoid indexing an empty translated path. This eliminates the OOB/underflow risk from malformed or empty name/cwd audit fields while preserving existing behavior for well-formed input.

🤖 Prompt for AI agents

In osquery/tables/events/linux/process_file_events.cpp around line 202, review and complete this code-review fix: NormalizePath reads path[0]/cwd[0] without checking for empty strings, risking OOB read.
What the draft fix changed: In `NormalizePath()` (anonymous namespace, `osquery/tables/events/linux/process_file_events.cpp`), added explicit `!path.empty()` and `!cwd.empty()` checks before indexing `path[0]`/`cwd[0]`, and guarded the `substr(1, size - 2)` calls with a `size() >= 2` check (falling back to using the string as-is when it is only a single `"` character or shorter) to prevent the size_t underflow that could throw `std::out_of_range`/`std::length_error`. Also changed `translated_path[0] != '/'` to `translated_path.empty() || translated_path[0] != '/'` to avoid indexing an empty translated path. This eliminates the OOB/underflow risk from malformed or empty `name`/`cwd` audit fields while preserving existing behavior for well-formed input.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 initialize() returns success when the process IS WoW64, inverted condition

Changed SceClientHelper::isWow64Process() to take a bool& isWow64 out-param and return Status::success() only when the underlying IsWow64Process WinAPI call succeeds, returning Status::failure() only on an actual query failure; the WoW64 boolean result is now communicated via the out-param instead of being encoded in Status.ok(). Updated the caller in SceClientHelper::initialize() to check !wow64Status.ok() for real query failure, and to separately check isWow64 to bail out with a failure message when the process is WoW64. This removes the inverted/overloaded Status semantics described in the finding. Note: the header file security_profile_info_utils.h (not shown/provided) declares isWow64Process() with the old signature (Status isWow64Process()) and must be updated to match the new signature (Status isWow64Process(bool& isWow64)) for this to compile; since that file was not included in the material provided, I could not edit it here, so this fix is incomplete until the header is updated correspondingly.

🤖 Prompt for AI agents

In osquery/tables/system/windows/security_profile_info_utils.cpp around line 51, review and complete this code-review fix: initialize() returns success when the process IS WoW64, inverted condition.
What the draft fix changed: Changed `SceClientHelper::isWow64Process()` to take a `bool& isWow64` out-param and return `Status::success()` only when the underlying `IsWow64Process` WinAPI call succeeds, returning `Status::failure()` only on an actual query failure; the WoW64 boolean result is now communicated via the out-param instead of being encoded in Status.ok(). Updated the caller in `SceClientHelper::initialize()` to check `!wow64Status.ok()` for real query failure, and to separately check `isWow64` to bail out with a failure message when the process is WoW64. This removes the inverted/overloaded Status semantics described in the finding. Note: the header file `security_profile_info_utils.h` (not shown/provided) declares `isWow64Process()` with the old signature (`Status isWow64Process()`) and must be updated to match the new signature (`Status isWow64Process(bool& isWow64)`) for this to compile; since that file was not included in the material provided, I could not edit it here, so this fix is incomplete until the header is updated correspondingly.
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

[flamingo]

🦩 🔵 assert() used for release-mode safety check before unchecked map lookup result use

In YARAEventSubscriber::Callback (osquery/tables/yara/windows/yara_events.cpp), replaced the debug-only assert(action_description_it != kNTFSEventToStringMap.end()); with a release-safe runtime check if (action_description_it == kNTFSEventToStringMap.end()) { VLOG(1) << ...; continue; } so an unmapped event.type now causes the loop to skip that event (continuing to the next item in ec->event_list) instead of dereferencing end() in release builds.

🤖 Prompt for AI agents

In osquery/tables/yara/windows/yara_events.cpp around line 142, review and complete this code-review fix: assert() used for release-mode safety check before unchecked map lookup result use.
What the draft fix changed: In `YARAEventSubscriber::Callback` (osquery/tables/yara/windows/yara_events.cpp), replaced the debug-only `assert(action_description_it != kNTFSEventToStringMap.end());` with a release-safe runtime check `if (action_description_it == kNTFSEventToStringMap.end()) { VLOG(1) << ...; continue; }` so an unmapped `event.type` now causes the loop to skip that event (continuing to the next item in `ec->event_list`) instead of dereferencing `end()` in release builds.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 genLoadAverage leaves results[i] default-constructed on getloadavg failure

In genLoadAverage (osquery/tables/system/posix/load_average.cpp), changed QueryData results(3) (which pre-allocates 3 default-constructed Row entries) to QueryData results (empty), and replaced results[i] = r; with results.push_back(r); inside the success branch. Now on getloadavg() failure the function returns an empty QueryData instead of 3 bogus empty rows, and on success it returns exactly the 3 populated rows as before.

🤖 Prompt for AI agents

In osquery/tables/system/posix/load_average.cpp around line 26, review and complete this code-review fix: genLoadAverage leaves results[i] default-constructed on getloadavg failure.
What the draft fix changed: In genLoadAverage (osquery/tables/system/posix/load_average.cpp), changed `QueryData results(3)` (which pre-allocates 3 default-constructed Row entries) to `QueryData results` (empty), and replaced `results[i] = r;` with `results.push_back(r);` inside the success branch. Now on getloadavg() failure the function returns an empty QueryData instead of 3 bogus empty rows, and on success it returns exactly the 3 populated rows as before.
Verify the change is correct and complete; do not refactor unrelated code.

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