fix(adhoc-sweep-fixes): 58 review findings across 40 files - #39
fix(adhoc-sweep-fixes): 58 review findings across 40 files#39flamingo[bot] wants to merge 40 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| Status SQLiteDatabasePlugin::get(const std::string& domain, | ||
| const std::string& key, | ||
| std::string& value) const { | ||
| QueryData results; | ||
| char* err = nullptr; | ||
| std::string q = "select value from " + domain + " where key = '" + key + "';"; | ||
| sqlite3_exec(db_, q.c_str(), getData, &results, &err); | ||
| if (err != nullptr) { | ||
| sqlite3_free(err); | ||
| sqlite3_stmt* stmt = nullptr; | ||
| std::string q = "select value from " + domain + " where key = ?1;"; | ||
| auto rc = sqlite3_prepare_v2(db_, q.c_str(), -1, &stmt, nullptr); | ||
| if (rc != SQLITE_OK || stmt == nullptr) { | ||
| if (stmt != nullptr) { | ||
| sqlite3_finalize(stmt); | ||
| } | ||
| return Status(1); | ||
| } | ||
|
|
||
| // Only assign value if the query found a result. | ||
| if (results.size() > 0) { | ||
| value = std::move(results[0]["value"]); | ||
| sqlite3_bind_text(stmt, 1, key.c_str(), -1, SQLITE_STATIC); | ||
|
|
||
| rc = sqlite3_step(stmt); | ||
| if (rc == SQLITE_ROW) { | ||
| const auto* text = sqlite3_column_text(stmt, 0); | ||
| value = (text != nullptr) ? reinterpret_cast<const char*>(text) : ""; | ||
| sqlite3_finalize(stmt); | ||
| return Status(0); | ||
| } | ||
|
|
||
| sqlite3_finalize(stmt); | ||
| return Status(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 SQL injection in SQLiteDatabasePlugin::get() via unsanitized key concatenation
Rewrote SQLiteDatabasePlugin::get(const std::string&, const std::string&, std::string&) to use sqlite3_prepare_v2 + sqlite3_bind_text with a ?1 placeholder for key instead of concatenating it into the SQL string, eliminating the injection vector. Added prepare-failure and null-stmt checks with finalize, and reads the result via sqlite3_column_text/sqlite3_step (SQLITE_ROW) instead of the sqlite3_exec callback.
🤖 Prompt for AI agents
In plugins/database/sqlite.cpp around line 111, review and complete this code-review fix: SQL injection in SQLiteDatabasePlugin::get() via unsanitized key concatenation.
What the draft fix changed: Rewrote SQLiteDatabasePlugin::get(const std::string&, const std::string&, std::string&) to use sqlite3_prepare_v2 + sqlite3_bind_text with a `?1` placeholder for `key` instead of concatenating it into the SQL string, eliminating the injection vector. Added prepare-failure and null-stmt checks with finalize, and reads the result via sqlite3_column_text/sqlite3_step (SQLITE_ROW) instead of the sqlite3_exec callback.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| } | ||
| return Status(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🔴 sqlite3_stmt leaked on failure paths in putBatch, remove, and removeRange
In putBatch, remove, and removeRange, added sqlite3_finalize(stmt) on the rc != SQLITE_DONE error path before returning Status(1), fixing the statement leak. Also added checks on the return value of sqlite3_prepare_v2 (and null stmt) in all three functions plus the rewritten get(), finalizing and returning Status(1) if preparation fails, preventing binds/steps on an invalid/null statement.
🤖 Prompt for AI agents
In plugins/database/sqlite.cpp around line 199, review and complete this code-review fix: sqlite3_stmt leaked on failure paths in putBatch, remove, and removeRange.
What the draft fix changed: In putBatch, remove, and removeRange, added sqlite3_finalize(stmt) on the `rc != SQLITE_DONE` error path before returning Status(1), fixing the statement leak. Also added checks on the return value of sqlite3_prepare_v2 (and null stmt) in all three functions plus the rewritten get(), finalizing and returning Status(1) if preparation fails, preventing binds/steps on an invalid/null statement.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| const std::string& key) { | ||
| sqlite3_stmt* stmt = nullptr; | ||
| std::string q = "delete from " + domain + " where key IN (?1);"; | ||
| sqlite3_prepare_v2(db_, q.c_str(), -1, &stmt, nullptr); | ||
| auto prc = sqlite3_prepare_v2(db_, q.c_str(), -1, &stmt, nullptr); | ||
| if (prc != SQLITE_OK || stmt == nullptr) { | ||
| if (stmt != nullptr) { | ||
| sqlite3_finalize(stmt); | ||
| } | ||
| return Status(1); | ||
| } | ||
|
|
||
| sqlite3_bind_text(stmt, 1, key.c_str(), -1, SQLITE_STATIC); | ||
| auto rc = sqlite3_step(stmt); | ||
| if (rc != SQLITE_DONE) { | ||
| sqlite3_finalize(stmt); | ||
| return Status(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 SQL injection risk in SQLiteDatabasePlugin::scan() via unsanitized prefix in LIKE clause
Rewrote SQLiteDatabasePlugin::scan() to use a prepared statement with key LIKE ?1 || '%' and sqlite3_bind_text for prefix, replacing the string-concatenated LIKE clause and sqlite3_exec/getData callback. Results are now collected by stepping through the prepared statement with sqlite3_column_text. This changes the internal query mechanism (bound LIKE pattern rather than literal concatenation with max appended after prepare-time limit clause); behavior for max/limit is preserved but the switch away from sqlite3_exec's callback-based error string (err) means prepare failures are now handled by returning success with no results instead of setting err — worth a reviewer's confirmation that silently returning empty results on a prepare failure is acceptable versus surfacing an error status.
🤖 Prompt for AI agents
In plugins/database/sqlite.cpp around line 226, review and complete this code-review fix: SQL injection risk in SQLiteDatabasePlugin::scan() via unsanitized prefix in LIKE clause.
What the draft fix changed: Rewrote SQLiteDatabasePlugin::scan() to use a prepared statement with `key LIKE ?1 || '%'` and sqlite3_bind_text for `prefix`, replacing the string-concatenated LIKE clause and sqlite3_exec/getData callback. Results are now collected by stepping through the prepared statement with sqlite3_column_text. This changes the internal query mechanism (bound LIKE pattern rather than literal concatenation with `max` appended after prepare-time limit clause); behavior for `max`/limit is preserved but the switch away from sqlite3_exec's callback-based error string (`err`) means prepare failures are now handled by returning success with no results instead of setting `err ` — worth a reviewer's confirmation that silently returning empty results on a prepare failure is acceptable versus surfacing an error status.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| case AUT_SUBJECT64: | ||
| r["auid"] = INTEGER(tok.tt.subj64.auid); | ||
| r["pid"] = INTEGER(tok.tt.subj64.pid); | ||
| pid = tok.tt.subj32.pid; | ||
| pid = tok.tt.subj64.pid; | ||
| break; | ||
| case AUT_SUBJECT32_EX: | ||
| r["auid"] = INTEGER(tok.tt.subj32_ex.auid); |
There was a problem hiding this comment.
🦩 🔴 Wrong subject token copied into pid for AUT_SUBJECT64 in socket_events.cpp
In OpenBSMNetEvSubscriber::Callback, AUT_SUBJECT64 case: changed pid = tok.tt.subj32.pid; to pid = tok.tt.subj64.pid;, matching the correct 64-bit union member consistent with the preceding r["pid"] assignment on the same case.
🤖 Prompt for AI agents
In osquery/tables/events/darwin/socket_events.cpp around line 163, review and complete this code-review fix: Wrong subject token copied into pid for AUT_SUBJECT64 in socket_events.cpp.
What the draft fix changed: In `OpenBSMNetEvSubscriber::Callback`, `AUT_SUBJECT64` case: changed `pid = tok.tt.subj32.pid;` to `pid = tok.tt.subj64.pid;`, matching the correct 64-bit union member consistent with the preceding `r["pid"]` assignment on the same case.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| r["action"] = "bind"; | ||
| } else if (tok.tt.hdr32.e_type == AUE_ACCEPT) { | ||
| } else if (tok.tt.hdr32_ex.e_type == AUE_ACCEPT) { | ||
| r["action"] = "accept"; |
There was a problem hiding this comment.
🦩 🟠 Typo bug in AUT_HEADER32_EX branch: AUE_ACCEPT check reads from wrong header token (hdr32 instead of hdr32_ex)
In OpenBSMNetEvSubscriber::Callback, AUT_HEADER32_EX case: changed the AUE_ACCEPT comparison from tok.tt.hdr32.e_type to tok.tt.hdr32_ex.e_type, matching the other two comparisons in the same branch.
🤖 Prompt for AI agents
In osquery/tables/events/darwin/socket_events.cpp around line 109, review and complete this code-review fix: Typo bug in AUT_HEADER32_EX branch: AUE_ACCEPT check reads from wrong header token (hdr32 instead of hdr32_ex).
What the draft fix changed: In `OpenBSMNetEvSubscriber::Callback`, `AUT_HEADER32_EX` case: changed the AUE_ACCEPT comparison from `tok.tt.hdr32.e_type` to `tok.tt.hdr32_ex.e_type`, matching the other two comparisons in the same branch.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| // The cache will always know the address. | ||
| r["address"] = ipAsString(addr_map[RTAX_DST]); | ||
|
|
||
| auto sdl = (struct sockaddr_dl *)addr_map[RTA_DST]; | ||
| auto sdl = (struct sockaddr_dl *)addr_map[RTAX_DST]; | ||
| if (sdl->sdl_alen > 0) { | ||
| r["mac"] = macAsString(LLADDR(sdl)); | ||
| } else { |
There was a problem hiding this comment.
🦩 🟠 genArp indexes addr_map with RTA_DST (a bitmask flag) instead of RTAX_DST (an array index)
In genArp (osquery/tables/networking/darwin/routes.cpp), changed auto sdl = (struct sockaddr_dl *)addr_map[RTA_DST]; to auto sdl = (struct sockaddr_dl *)addr_map[RTAX_DST];, replacing the bitmask constant RTA_DST with the correct array index constant RTAX_DST, consistent with all other addr_map accesses in the file.
🤖 Prompt for AI agents
In osquery/tables/networking/darwin/routes.cpp around line 111, review and complete this code-review fix: genArp indexes addr_map with RTA_DST (a bitmask flag) instead of RTAX_DST (an array index).
What the draft fix changed: In genArp (osquery/tables/networking/darwin/routes.cpp), changed `auto sdl = (struct sockaddr_dl *)addr_map[RTA_DST];` to `auto sdl = (struct sockaddr_dl *)addr_map[RTAX_DST];`, replacing the bitmask constant RTA_DST with the correct array index constant RTAX_DST, consistent with all other addr_map accesses in the file.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| } | ||
| } // namespace tables | ||
| } // namespace osquery | ||
|
|
There was a problem hiding this comment.
🦩 🟠 sqlite3_prepare_v2 return code not checked in genGateKeeperApprovedApps before calling sqlite3_step
In genGateKeeperApprovedApps, added a check if (rc != SQLITE_OK || stmt == nullptr) immediately after sqlite3_prepare_v2, mirroring the pattern in isGateKeeperDevIdEnabled. On failure, finalizes stmt if non-null, closes db, and returns the (empty) results before reaching sqlite3_step, preventing use of a null/invalid statement.
🤖 Prompt for AI agents
In osquery/tables/system/darwin/gatekeeper.cpp around line 191, review and complete this code-review fix: sqlite3_prepare_v2 return code not checked in genGateKeeperApprovedApps before calling sqlite3_step.
What the draft fix changed: In `genGateKeeperApprovedApps`, added a check `if (rc != SQLITE_OK || stmt == nullptr)` immediately after `sqlite3_prepare_v2`, mirroring the pattern in `isGateKeeperDevIdEnabled`. On failure, finalizes `stmt` if non-null, closes `db`, and returns the (empty) `results` before reaching `sqlite3_step`, preventing use of a null/invalid statement.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| genSingleVariable(options, key, results); | ||
| })); | ||
| } else { | ||
| CFMutableDictionaryRef options_dict; | ||
| CFMutableDictionaryRef options_dict = nullptr; | ||
| kr = IORegistryEntryCreateCFProperties( | ||
| options, &options_dict, kCFAllocatorDefault, 0); | ||
| if (kr != KERN_SUCCESS) { |
There was a problem hiding this comment.
🦩 🟠 genNVRAM leaks options_dict when IORegistryEntryCreateCFProperties succeeds
In genNVRAM, initialized options_dict to nullptr and guarded the trailing CFRelease(options_dict) with if (options_dict != nullptr), so when IORegistryEntryCreateCFProperties fails (kr != KERN_SUCCESS), no CFRelease is called on an uninitialized/garbage pointer, eliminating the UB/crash path while still releasing the dictionary when creation succeeds.
🤖 Prompt for AI agents
In osquery/tables/system/darwin/nvram.cpp around line 138, review and complete this code-review fix: genNVRAM leaks options_dict when IORegistryEntryCreateCFProperties succeeds.
What the draft fix changed: In genNVRAM, initialized `options_dict` to `nullptr` and guarded the trailing `CFRelease(options_dict)` with `if (options_dict != nullptr)`, so when `IORegistryEntryCreateCFProperties` fails (kr != KERN_SUCCESS), no CFRelease is called on an uninitialized/garbage pointer, eliminating the UB/crash path while still releasing the dictionary when creation succeeds.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| } | ||
|
|
||
| // Checking that input pointer points to an accessible SceProfileInfo layout | ||
| if (IsBadReadPtr(&profileData, sizeof(SceProfileInfo))) { | ||
| if (IsBadReadPtr(profileData, sizeof(SceProfileInfo))) { | ||
| return Status::failure("profileData layout is invalid."); | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 SceClientHelper::isValidSceProfileData checks readability of the wrong pointer (&profileData instead of profileData)
In SceClientHelper::isValidSceProfileData, changed IsBadReadPtr(&profileData, sizeof(SceProfileInfo)) to IsBadReadPtr(profileData, sizeof(SceProfileInfo)), so the readability check now validates the actual pointer value received from the SCE RPC call instead of the address of the local reference/parameter, matching the function's intended purpose.
🤖 Prompt for AI agents
In osquery/tables/system/windows/security_profile_info_utils.cpp around line 155, review and complete this code-review fix: SceClientHelper::isValidSceProfileData checks readability of the wrong pointer (&profileData instead of profileData).
What the draft fix changed: In `SceClientHelper::isValidSceProfileData`, changed `IsBadReadPtr(&profileData, sizeof(SceProfileInfo))` to `IsBadReadPtr(profileData, sizeof(SceProfileInfo))`, so the readability check now validates the actual pointer value received from the SCE RPC call instead of the address of the local reference/parameter, matching the function's intended purpose.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| continue; | ||
| } | ||
|
|
||
| bool directory; | ||
| bool is_directory; | ||
| if (entry->d_type == DT_DIR) { | ||
| directory = true; | ||
| is_directory = true; | ||
| } else if (entry->d_type == DT_LNK || entry->d_type == DT_REG) { | ||
| directory = false; | ||
| is_directory = false; | ||
| } else { | ||
| continue; | ||
| } | ||
|
|
||
| callback(string_fd, directory); | ||
| callback(string_fd, is_directory); | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
🦩 🟠 Filesystem::enumFiles shadows outer-scope directory local unique_ptr with a bool of the same name
Renamed the inner bool directory; local to bool is_directory; inside the for(;;) loop in Filesystem::enumFiles, and updated its two assignment sites and the callback(string_fd, is_directory); call to match, eliminating the shadowing of the outer UniqueDir directory variable.
🤖 Prompt for AI agents
In osquery/events/linux/bpf/filesystem.cpp around line 121, review and complete this code-review fix: Filesystem::enumFiles shadows outer-scope `directory` local unique_ptr with a bool of the same name.
What the draft fix changed: Renamed the inner `bool directory;` local to `bool is_directory;` inside the `for(;;)` loop in `Filesystem::enumFiles`, and updated its two assignment sites and the `callback(string_fd, is_directory);` call to match, eliminating the shadowing of the outer `UniqueDir directory` variable.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
🦩 What this fix changed, finding by finding58 finding(s) fixed in this draft. (Inline placement was rejected by GitHub for this PR.) 🔴 1. SQL injection in SQLiteDatabasePlugin::get() via unsanitized key concatenation — 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🔴 2. sqlite3_stmt leaked on failure paths in putBatch, remove, and removeRange — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 3. SQL injection risk in SQLiteDatabasePlugin::scan() via unsanitized prefix in LIKE clause — 🤖 Prompt for AI agentsfix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer 🔴 4. Wrong subject token copied into pid for AUT_SUBJECT64 in socket_events.cpp — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 5. Typo bug in AUT_HEADER32_EX branch: AUE_ACCEPT check reads from wrong header token (hdr32 instead of hdr32_ex) — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 6. Same header-union typo repeated in AUT_HEADER64/HEADER64_EX branch — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🔴 7. azure_instance_metadata.cpp integration test fails to compile: missing comma and unclosed brace — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 8. Closing namespace comment mismatched — says 'table_tests' instead of 'osquery' — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🔴 9. genDnsCache: no NULL check on LoadLibraryExW/GetProcAddress before invoking DnsGetCacheDataTable — 🤖 Prompt for AI agentsfix confidence: 🟡 88 medium — react 👍/👎 to teach the reviewer 🟠 10. genDnsCache never calls FreeLibrary(hLib) after LoadLibraryExW — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🔴 11. prefetch.cpp parsePrefetch: compressed_data.size() checked against sizeof(PPREFETCH_COMPRESSED_HEADER) (a pointer type), not sizeof(PREFETCH_COMPRESSED_HEADER) (the struct), allowing undersized buffers through — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🔴 12. prefetch.cpp parsePrefetch: second undersized-buffer check also uses sizeof(pointer typedef) instead of sizeof(struct) — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🔴 13. Stray 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 14. std::cerr used for diagnostic output instead of LOG() macro — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🔴 15. compress()/decompress() use std::vector<void> as raw byte buffers, wasting 8x memory and risking undefined behavior with read()/write()* — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🔴 16. decompress() repeats the same std::vector<void> buffer sizing bug as compress()* — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🔴 17. logon_sessions.cpp: unchecked map::find()->second on kLogonTypeToStr can dereference end() iterator — 🤖 Prompt for AI agentsfix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer 🟠 18. logon_sessions.cpp never frees session_data or the sessions array returned by LSA — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🔴 19. Inverted existence check causes SCNetwork subscription targets to never be re-added during configure() — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🔴 20. SCNetworkEventPublisher::Callback never fires the event to subscribers — 🤖 Prompt for AI agentsfix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer 🔴 21. Inverted boolean logic makes validateSocketDescriptor always fail when domain is set — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🔴 22. EtwProviderConfig::isValid() checks getPostProcessor() twice instead of also validating getPreProcessor() — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🔴 23. genPortageKeywordSummary never actually calls the parser when unmasked file is missing/empty — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🔴 24. requireAction uses bitwise-AND assignment instead of OR, silently discarding previously-set mask bits — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🔴 25. Off-by-one/logic bug: hostname port-delimiter check uses unsigned wraparound when ':' is not found — 🤖 Prompt for AI agentsfix confidence: 🟢 98 high — react 👍/👎 to teach the reviewer 🟠 26. cleanOldAggregationCacheEntries erases the current iterator then increments it, causing use-after-erase — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 27. processImageCache_ in EtwPublisherProcesses grows unbounded with no eviction — 🤖 Prompt for AI agentsfix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer 🟠 28. Insecure/deprecated os.makedirs mode literal uses Python 2 octal syntax (0755) — will fail to parse under Python 3 — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 29. JSON parse failure in gentargets.py is logged critical but execution continues, leading to a later crash — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 30. queries_from_pack references undefined variable config_path in error message — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🔵 31. profile_cmd() calls p.wait() twice, second call after process already reaped — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 32. getProcessProperties() copies egid into euid field, duplicating egid lookup and never reading ruid for euid — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 33. getCDHash() uses find_first_not_of(s.front()) which is a character-search bug, not an all-zero-check — 🤖 Prompt for AI agentsfix confidence: 🟡 70 medium — react 👍/👎 to teach the reviewer 🟠 34. isOpenSSHKeyEncrypted can read out-of-bounds via substr on short key content — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 35. BIO_new result checked for null after being used, and passed to guard before null-check — 🤖 Prompt for AI agentsfix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer 🟠 36. Kafka topic_conf leaked when topic conf set fails in initTopic — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agentsfix confidence: 🔴 40 low — review closely — react 👍/👎 to teach the reviewer 🟠 37. configureTopics() base-topic branch overwrites queryToTopics_[kKafkaBaseTopic] with nullptr on failed initTopic, silently breaking base-topic fallback — 🤖 Prompt for AI agentsfix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer 🟠 38. Shadowed variable name 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🟠 39. Potential division by zero in cosineSimilarity when buffer_size is 0 — 🤖 Prompt for AI agentsfix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer 🟠 40. executeCarve() truncates carve-status message to a hardcoded length of 13, corrupting output — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🔴 41. pidsFromContext copies procs into itself instead of the newly-queried proc result — 🤖 Prompt for AI agentsfix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer 🔴 42. FakeEventSubscriber(bool skip_name) constructor calls FakeEventSubscriber() as a temporary, not delegating construction — (Automatically downgraded: no change in this fix lands near this finding's line — verify whether it was actually addressed.) 🤖 Prompt for AI agents |
Closes 58 review findings across 40 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
plugins/database/sqlite.cpp:111plugins/database/sqlite.cpp:199plugins/database/sqlite.cpp:226osquery/tables/events/darwin/socket_events.cpp:163osquery/tables/events/darwin/socket_events.cpp:109osquery/tables/events/darwin/socket_events.cpp:120tests/integration/tables/azure_instance_metadata.cpp:22tests/integration/tables/azure_instance_metadata.cpp:40osquery/tables/system/windows/dns_cache.cpp:143osquery/tables/system/windows/dns_cache.cpp:144osquery/tables/system/windows/prefetch.cpp:290osquery/tables/system/windows/prefetch.cpp:315} // namespace osqueryclosing brace inside function body causes malformed nesting in validateIMDSV2RequestAttemptsosquery/utils/aws/aws_util.cpp:178osquery/utils/aws/aws_util.cpp:178osquery/filesystem/file_compression.cpp:1osquery/filesystem/file_compression.cpp:1osquery/tables/system/windows/logon_sessions.cpp:65osquery/tables/system/windows/logon_sessions.cpp:44osquery/events/darwin/scnetwork.cpp:108osquery/events/darwin/scnetwork.cpp:26osquery/events/tests/linux/bpf/utils.cpp:170osquery/events/windows/etw/etw_provider_config.cpp:15osquery/tables/system/linux/portage.cpp:288osquery/events/darwin/fsevents.cpp:48osquery/tables/networking/curl_certificate.cpp:285osquery/events/windows/etw/etw_publisher_processes.cpp:353osquery/events/windows/etw/etw_publisher_processes.cpp:320tools/codegen/gentargets.py:128tools/codegen/gentargets.py:112tools/tests/utils.py:116tools/tests/utils.py:190osquery/events/darwin/es_utils.cpp:152osquery/events/darwin/es_utils.cpp:120osquery/tables/system/ssh_keys.cpp:44osquery/tables/system/ssh_keys.cpp:62plugins/logger/kafka_producer.cpp:267plugins/logger/kafka_producer.cpp:314event_listin WindowsEventLogPublisher::run looposquery/events/windows/windowseventlogpublisher.cpp:155osquery/events/windows/windowseventlogpublisher.cpp:214osquery/sql/sqlite_operations.cpp:53osquery/tables/system/system_utils.cpp:29osquery/events/tests/events_tests.cpp:297osquery/tables/system/windows/scheduled_tasks.cpp:108osquery/core/plugins/logger.h:249osquery/tables/applications/chrome/utils.cpp:138osquery/tables/system/cpuid.cpp:176osquery/tables/system/darwin/sharing_preferences.cpp:161osquery/system/usersgroups/windows/users_groups_cache.cpp:100osquery/tables/events/darwin/openbsm_events.cpp:168osquery/tables/system/linux/block_devices.cpp:95tests/integration/tables/safari_extensions.cpp:26yellow()function, will raise NameError on unknown column optiontools/codegen/gentable.py:218osquery/profiler/windows/code_profiler.cpp:47osquery/tables/networking/darwin/routes.cpp:111osquery/tables/system/darwin/gatekeeper.cpp:191osquery/tables/system/darwin/nvram.cpp:138osquery/tables/system/windows/security_profile_info_utils.cpp:155directorylocal unique_ptr with a bool of the same nameosquery/events/linux/bpf/filesystem.cpp:121What 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.