fix(adhoc-sweep-fixes): CU-86aknprfh 11 review findings across 11 files

[flamingo]

Closes 11 review findings across 11 files.

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

#	Fix confidence	Finding	Location
1	🟢 95 high	azure_instance_metadata.cpp test file has syntax errors — missing comma and unclosed braces	tests/integration/tables/azure_instance_metadata.cpp:22
2	🟢 92 high	test_windows_service.py uses Python 2-only syntax and file modes, breaking under Python 3	tools/tests/test_windows_service.py:106
3	🟢 95 high	RocksDB removeRange performs a redundant and unsafe extra Delete after DeleteRange	plugins/database/rocksdb.cpp:428
4	🟢 95 high	Division by zero possible in cosineSimilarity when buffer is empty	osquery/events/windows/windowseventlogpublisher.cpp:206
5	🔴 40 low — review closely	disk_properties CFDictionaryRef leaked in DiskAppearedCallback early-return path before protocol_properties check	osquery/events/darwin/diskarbitration.cpp:96
6	🟢 95 high	Pidfile::readFile (Windows) does not break out of the retry loop or return an error on short read	osquery/utils/pidfile/pidfile_windows.cpp:147
7	🟢 95 high	init() silently discards the Status returned by logStatus(log)	plugins/logger/windows_event_log.cpp:59
8	🟢 92 high	MD::getDevName/getPathByDevName call atoi-style std::stoi on possibly-null udev property without null-check	osquery/tables/system/linux/md_tables.cpp:120
9	🟢 97 high	getSensorSettings() overwrites parsed collect_processes value with a hardcoded 0	osquery/tables/applications/posix/carbon_black.cpp:84
10	🟡 85 medium	EventSubscriberPlugin::addBatch silently drops rows whose JSON serialization fails without surfacing an error to the caller	osquery/events/eventsubscriberplugin.cpp:124
11	🟢 90 high	getUptime() has dead code path for DARWIN while runtime guard uses APPLE	osquery/utils/system/uptime.cpp:12

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: 116a6ce1-ae2a-4217-a4a6-046396b5281c

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-86aknprfh Osquery review findings sweep (14 PRs)

[flamingo]

🦩 What this fix changed, finding by finding

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

[flamingo]

🦩 🔴 azure_instance_metadata.cpp test file has syntax errors — missing comma and unclosed braces

In tests/integration/tables/azure_instance_metadata.cpp, fixed the TEST_F(azureInstanceMetadata, test_sanity) body: added the missing comma between {"sku", NormalType} and {"version", NormalType} in the ValidationMap row_map initializer list, closed the if (!data.empty()) block and the TEST_F function body with a closing }, and corrected the trailing namespace closures to } // namespace table_tests followed by } // namespace osquery, matching the pattern used in other integration test files like authorized_keys.cpp and autoexec.cpp.

🤖 Prompt for AI agents

In tests/integration/tables/azure_instance_metadata.cpp around line 22, review and complete this code-review fix: azure_instance_metadata.cpp test file has syntax errors — missing comma and unclosed braces.
What the draft fix changed: In `tests/integration/tables/azure_instance_metadata.cpp`, fixed the `TEST_F(azureInstanceMetadata, test_sanity)` body: added the missing comma between `{"sku", NormalType}` and `{"version", NormalType}` in the `ValidationMap row_map` initializer list, closed the `if (!data.empty())` block and the `TEST_F` function body with a closing `}`, and corrected the trailing namespace closures to `} // namespace table_tests` followed by `} // namespace osquery`, matching the pattern used in other integration test files like authorized_keys.cpp and autoexec.cpp.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 test_windows_service.py uses Python 2-only syntax and file modes, breaking under Python 3

In sc(), changed except subprocess.CalledProcessError, err: to except subprocess.CalledProcessError as err: (Python 2-only syntax causing a SyntaxError under Python 3), and changed if x.strip() is not '' to if x.strip() != '' as suggested. Also changed the two open(..., 'wb') calls in OsquerydTest.setUp (for self.config_path and self.flagfile) to 'w' text mode, since CONFIG_FILE and FLAGS_FILE are str literals and writing str to a binary-mode file raises a TypeError under Python 3. These are the only changes; no other code paths were touched.

🤖 Prompt for AI agents

In tools/tests/test_windows_service.py around line 106, review and complete this code-review fix: test_windows_service.py uses Python 2-only syntax and file modes, breaking under Python 3.
What the draft fix changed: In `sc()`, changed `except subprocess.CalledProcessError, err:` to `except subprocess.CalledProcessError as err:` (Python 2-only syntax causing a SyntaxError under Python 3), and changed `if x.strip() is not ''` to `if x.strip() != ''` as suggested. Also changed the two `open(..., 'wb')` calls in `OsquerydTest.setUp` (for `self.config_path` and `self.flagfile`) to `'w'` text mode, since `CONFIG_FILE` and `FLAGS_FILE` are `str` literals and writing `str` to a binary-mode file raises a `TypeError` under Python 3. These are the only changes; no other code paths were touched.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔴 RocksDB removeRange performs a redundant and unsafe extra Delete after DeleteRange

In RocksDBDatabasePlugin::removeRange, removed the redundant/unsafe if (low <= high) { s = getDB()->Delete(options, cfh, high); } block that followed getDB()->DeleteRange(...). The function now simply returns the status of the DeleteRange call, restoring exclusive-upper-bound semantics and no longer masking DeleteRange failures, matching the suggested fix exactly.

🤖 Prompt for AI agents

In plugins/database/rocksdb.cpp around line 428, review and complete this code-review fix: RocksDB removeRange performs a redundant and unsafe extra Delete after DeleteRange.
What the draft fix changed: In `RocksDBDatabasePlugin::removeRange`, removed the redundant/unsafe `if (low <= high) { s = getDB()->Delete(options, cfh, high); }` block that followed `getDB()->DeleteRange(...)`. The function now simply returns the status of the `DeleteRange` call, restoring exclusive-upper-bound semantics and no longer masking `DeleteRange` failures, matching the suggested fix exactly.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 Division by zero possible in cosineSimilarity when buffer is empty

In cosineSimilarity (osquery/events/windows/windowseventlogpublisher.cpp), added an early if (buffer_size == 0) return 0.0; guard right after computing buffer_size, preventing the 1.0 / buffer_size division-by-zero/inf pollution of buffer_freqs. Also added if (mag1 * mag2 == 0.0) return 0.0; before the final return dot / (mag1 * mag2); to guard against an all-zero global_freqs (or degenerate buffer_freqs) causing a NaN/Inf result. Both guards return a neutral similarity of 0.0 instead of propagating NaN/Inf downstream.

🤖 Prompt for AI agents

In osquery/events/windows/windowseventlogpublisher.cpp around line 206, review and complete this code-review fix: Division by zero possible in cosineSimilarity when buffer is empty.
What the draft fix changed: In `cosineSimilarity` (osquery/events/windows/windowseventlogpublisher.cpp), added an early `if (buffer_size == 0) return 0.0;` guard right after computing `buffer_size`, preventing the `1.0 / buffer_size` division-by-zero/inf pollution of `buffer_freqs`. Also added `if (mag1 * mag2 == 0.0) return 0.0;` before the final `return dot / (mag1 * mag2);` to guard against an all-zero `global_freqs` (or degenerate `buffer_freqs`) causing a NaN/Inf result. Both guards return a neutral similarity of 0.0 instead of propagating NaN/Inf downstream.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 disk_properties CFDictionaryRef leaked in DiskAppearedCallback early-return path before protocol_properties check

In DiskAppearedCallback (osquery/events/darwin/diskarbitration.cpp), the early-return branch after IORegistryEntryFromPath fails (entry == MACH_PORT_NULL) now calls CFRelease(disk_properties) before return;, preventing the CFDictionaryRef leak described in the finding. The file already contained this release call in the provided source; verified it is present and correctly placed before the return statement, matching the exact fix requested.

(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/darwin/diskarbitration.cpp around line 96, review and complete this code-review fix: disk_properties CFDictionaryRef leaked in DiskAppearedCallback early-return path before protocol_properties check.
What the draft fix changed: In DiskAppearedCallback (osquery/events/darwin/diskarbitration.cpp), the early-return branch after `IORegistryEntryFromPath` fails (`entry == MACH_PORT_NULL`) now calls `CFRelease(disk_properties)` before `return;`, preventing the CFDictionaryRef leak described in the finding. The file already contained this release call in the provided source; verified it is present and correctly placed before the return statement, matching the exact fix requested.

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

🦩 🟠 init() silently discards the Status returned by logStatus(log)

In WindowsEventLoggerPlugin::init(), the previously discarded return value of logStatus(log) is now captured into log_status and checked with !log_status.ok(), logging the error message via LOG(ERROR), matching the existing pattern used for acquireHandle's status just above it.

🤖 Prompt for AI agents

In plugins/logger/windows_event_log.cpp around line 59, review and complete this code-review fix: init() silently discards the Status returned by logStatus(log).
What the draft fix changed: In WindowsEventLoggerPlugin::init(), the previously discarded return value of logStatus(log) is now captured into log_status and checked with !log_status.ok(), logging the error message via LOG(ERROR), matching the existing pattern used for acquireHandle's status just above it.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 MD::getDevName/getPathByDevName call atoi-style std::stoi on possibly-null udev property without null-check

In MD::getDevName (and the related MD::getPathByDevName, MD::getSuperblkVersion helpers used by the same udev-walking code path), added null checks before using the result of udev_device_get_property_value. getDevName now checks devMajor == nullptr || devMinor == nullptr and returns false (skip this device) before calling std::stoi, and guards the subsequent DEVNAME lookup with a null check before assigning to devName. getPathByDevName now checks the DEVNAME property pointer for null before constructing std::string from it, returning false to continue the device walk instead of constructing from nullptr (which would otherwise throw/UB). getSuperblkVersion, which has the identical pattern on DEVNAME/MD_METADATA, was also given the same null checks since it is the same unguarded-property-to-std::string pattern described by the finding and reachable from the same table-generation code paths; no behavior changes occur for the non-null case.

🤖 Prompt for AI agents

In osquery/tables/system/linux/md_tables.cpp around line 120, review and complete this code-review fix: MD::getDevName/getPathByDevName call atoi-style std::stoi on possibly-null udev property without null-check.
What the draft fix changed: In `MD::getDevName` (and the related `MD::getPathByDevName`, `MD::getSuperblkVersion` helpers used by the same udev-walking code path), added null checks before using the result of `udev_device_get_property_value`. `getDevName` now checks `devMajor == nullptr || devMinor == nullptr` and returns `false` (skip this device) before calling `std::stoi`, and guards the subsequent `DEVNAME` lookup with a null check before assigning to `devName`. `getPathByDevName` now checks the `DEVNAME` property pointer for null before constructing `std::string` from it, returning `false` to continue the device walk instead of constructing from nullptr (which would otherwise throw/UB). `getSuperblkVersion`, which has the identical pattern on `DEVNAME`/`MD_METADATA`, was also given the same null checks since it is the same unguarded-property-to-std::string pattern described by the finding and reachable from the same table-generation code paths; no behavior changes occur for the non-null case.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 getSensorSettings() overwrites parsed collect_processes value with a hardcoded 0

Removed the duplicate r["collect_processes"] = INTEGER(0); line in getSensorSettings() (osquery/tables/applications/posix/carbon_black.cpp) that overwrote the correctly parsed CB.CollectProcesses value from the INI file with a hardcoded 0. The earlier assignment using pt.get<std::string>("CB.CollectProcesses", int_default) now remains the sole, authoritative value for that column.

🤖 Prompt for AI agents

In osquery/tables/applications/posix/carbon_black.cpp around line 84, review and complete this code-review fix: getSensorSettings() overwrites parsed collect_processes value with a hardcoded 0.
What the draft fix changed: Removed the duplicate `r["collect_processes"] = INTEGER(0);` line in getSensorSettings() (osquery/tables/applications/posix/carbon_black.cpp) that overwrote the correctly parsed `CB.CollectProcesses` value from the INI file with a hardcoded 0. The earlier assignment using `pt.get<std::string>("CB.CollectProcesses", int_default)` now remains the sole, authoritative value for that column.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🟠 EventSubscriberPlugin::addBatch silently drops rows whose JSON serialization fails without surfacing an error to the caller

In EventSubscriberPlugin::addBatch, moved the event_id_list.push_back(event_identifier) call from immediately after getEventID() to after the serializeRowJSON call succeeds (right before the newline-trimming logic). Now, when serialization fails the code still continues and skips the row's data as before, but the event id is no longer pushed to event_id_list, so it is never committed into context.event_index. This keeps event_id_list and database_data consistent — every id recorded in the index has a corresponding stored row — eliminating the latent lookup-failure bug described in the finding. The generated-but-unused event identifier is simply skipped (ids are not required to be contiguous, since generateEventIdentifier only increments a counter), so no other invariant is broken.

🤖 Prompt for AI agents

In osquery/events/eventsubscriberplugin.cpp around line 124, review and complete this code-review fix: EventSubscriberPlugin::addBatch silently drops rows whose JSON serialization fails without surfacing an error to the caller.
What the draft fix changed: In EventSubscriberPlugin::addBatch, moved the `event_id_list.push_back(event_identifier)` call from immediately after `getEventID()` to after the `serializeRowJSON` call succeeds (right before the newline-trimming logic). Now, when serialization fails the code still `continue`s and skips the row's data as before, but the event id is no longer pushed to `event_id_list`, so it is never committed into `context.event_index`. This keeps `event_id_list` and `database_data` consistent — every id recorded in the index has a corresponding stored row — eliminating the latent lookup-failure bug described in the finding. The generated-but-unused event identifier is simply skipped (ids are not required to be contiguous, since `generateEventIdentifier` only increments a counter), so no other invariant is broken.
Verify the change is correct and complete; do not refactor unrelated code.

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

[flamingo]

🦩 🔵 getUptime() has dead code path for DARWIN while runtime guard uses APPLE

Changed the preprocessor guard in getUptime() in osquery/utils/system/uptime.cpp from #if defined(DARWIN) to #if defined(__APPLE__) so it matches the include guard above, ensuring the sysctl-based uptime implementation actually compiles and executes on macOS builds instead of falling through to return -1;.

🤖 Prompt for AI agents

In osquery/utils/system/uptime.cpp around line 12, review and complete this code-review fix: getUptime() has dead code path for DARWIN while runtime guard uses __APPLE__.
What the draft fix changed: Changed the preprocessor guard in getUptime() in osquery/utils/system/uptime.cpp from `#if defined(DARWIN)` to `#if defined(__APPLE__)` so it matches the include guard above, ensuring the sysctl-based uptime implementation actually compiles and executes on macOS builds instead of falling through to `return -1;`.
Verify the change is correct and complete; do not refactor unrelated code.

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