fix(adhoc-sweep-fixes): CU-86akhf8u2 34 review findings across 27 files - #72
flamingo[bot] wants to merge 27 commits into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| int result = static_cast<int>(syscall(SYS_setns, original_mnt_fd_, 0)); | ||
|
|
||
| if (result < 0) { | ||
| // We failed to restore the original mount namespace. Since this | ||
| // worker process may be reused for subsequent queries while | ||
| // keep_process_open_ is still true, force it to be treated as | ||
| // unusable so the caller will not keep it around in a stale | ||
| // namespace. | ||
| keep_process_open_ = false; | ||
|
|
||
| auto status = Status::failure( | ||
| "Failed to restore the original mount namespace, due to error: " + | ||
| std::to_string(errno)); |
There was a problem hiding this comment.
🦩 🔴 setns() switches namespace without restoring on early-return error paths, leaving worker in wrong mount namespace
In LinuxTableContainerIPC::handleJob() (worker-side, linux_table_container_ipc.cpp), when the final restore setns(original_mnt_fd_, ...) fails inside the keep_process_open_ block, the code now sets keep_process_open_ = false before returning the failure Status. This flag is also checked inside executeQueryJobs()'s loop (added a if (!keep_process_open_) break; after processing a message), so that once a job leaves the worker in a bad/unrestored namespace state, the worker process will exit its keep-open loop and terminate (via the existing std::_Exit at the end of executeQueryJobs) rather than being reused for a subsequent query while stuck in a stale mount namespace. This relies on keep_process_open_ being a plain member checked synchronously in the same thread/process (true here, since worker is single-threaded per process), so it should correctly stop reuse. Residual risk: the parent-side caller in generateInNamespace/retrieveQueryDataFromContainer does not know the worker self-terminated early beyond the failure Status already returned by handleJob over IPC, but since the child process actually exits after the loop breaks, any subsequent connectToContainer call will detect the process is gone and fork a fresh one, which is the safe behavior. A fully robust fix might also want to explicitly signal an immediate exit rather than waiting for the next message-processing iteration, but this covers the finding's core concern (stale-namespace reuse) with a minimal change.
🤖 Prompt for AI agents
In osquery/worker/ipc/linux/linux_table_container_ipc.cpp around line 345, review and complete this code-review fix: setns() switches namespace without restoring on early-return error paths, leaving worker in wrong mount namespace.
What the draft fix changed: In `LinuxTableContainerIPC::handleJob()` (worker-side, `linux_table_container_ipc.cpp`), when the final restore `setns(original_mnt_fd_, ...)` fails inside the `keep_process_open_` block, the code now sets `keep_process_open_ = false` before returning the failure Status. This flag is also checked inside `executeQueryJobs()`'s loop (added a `if (!keep_process_open_) break;` after processing a message), so that once a job leaves the worker in a bad/unrestored namespace state, the worker process will exit its keep-open loop and terminate (via the existing `std::_Exit` at the end of `executeQueryJobs`) rather than being reused for a subsequent query while stuck in a stale mount namespace. This relies on `keep_process_open_` being a plain member checked synchronously in the same thread/process (true here, since worker is single-threaded per process), so it should correctly stop reuse. Residual risk: the parent-side caller in `generateInNamespace`/`retrieveQueryDataFromContainer` does not know the worker self-terminated early beyond the failure Status already returned by `handleJob` over IPC, but since the child process actually exits after the loop breaks, any subsequent `connectToContainer` call will detect the process is gone and fork a fresh one, which is the safe behavior. A fully robust fix might also want to explicitly signal an immediate exit rather than waiting for the next message-processing iteration, but this covers the finding's core concern (stale-namespace reuse) with a minimal change.
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
| std::string original_mnt_path = | ||
| kProc + "/" + std::to_string(current_pid) + kMountNamespace; | ||
|
|
||
| if (original_mnt_fd_ > 0) { |
There was a problem hiding this comment.
🦩 🟠 original_mnt_fd_ leaked/overwritten check uses wrong sentinel value
In connectToContainer(), changed the guard if (original_mnt_fd_ > 0) to if (original_mnt_fd_ >= 0) exactly as suggested, so a previously opened fd valued 0 is no longer leaked/skipped when closing before reassigning original_mnt_fd_.
🤖 Prompt for AI agents
In osquery/worker/ipc/linux/linux_table_container_ipc.cpp around line 153, review and complete this code-review fix: original_mnt_fd_ leaked/overwritten check uses wrong sentinel value.
What the draft fix changed: In `connectToContainer()`, changed the guard `if (original_mnt_fd_ > 0)` to `if (original_mnt_fd_ >= 0)` exactly as suggested, so a previously opened fd valued 0 is no longer leaked/skipped when closing before reassigning `original_mnt_fd_`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| return Status::success(); | ||
| } | ||
| void LinuxTableContainerIPC::stopContainerWorker() { |
There was a problem hiding this comment.
🦩 🟠 stopContainerWorker moves out of the file-scope global current_running_process without any synchronization
Added #include <mutex> and a file-scope std::mutex current_running_process_mutex guarding all accesses to the global current_running_process in connectToContainer() (write under lock when forking) and stopContainerWorker() (move-out under lock into a local child_process, replacing the previous unguarded std::move). This closes the specific race of concurrent read/move/write on the plain global. Residual risk: this only serializes access to the global variable itself; it does not add higher-level locking to prevent two threads from concurrently running connectToContainer/stopContainerWorker logic (e.g., both forking and only the mutex-protected assignment being safe, while the surrounding IPC channel management and process spawning logic is still not thread-safe as a whole). A complete fix would likely require a coarser lock around the whole connect/stop sequence or a documented single-threaded usage contract, which is out of scope for a minimal, targeted change in this file.
🤖 Prompt for AI agents
In osquery/worker/ipc/linux/linux_table_container_ipc.cpp around line 206, review and complete this code-review fix: stopContainerWorker moves out of the file-scope global current_running_process without any synchronization.
What the draft fix changed: Added `#include <mutex>` and a file-scope `std::mutex current_running_process_mutex` guarding all accesses to the global `current_running_process` in `connectToContainer()` (write under lock when forking) and `stopContainerWorker()` (move-out under lock into a local `child_process`, replacing the previous unguarded `std::move`). This closes the specific race of concurrent read/move/write on the plain global. Residual risk: this only serializes access to the global variable itself; it does not add higher-level locking to prevent two threads from concurrently running `connectToContainer`/`stopContainerWorker` logic (e.g., both forking and only the mutex-protected assignment being safe, while the surrounding IPC channel management and process spawning logic is still not thread-safe as a whole). A complete fix would likely require a coarser lock around the whole connect/stop sequence or a documented single-threaded usage contract, which is out of scope for a minimal, targeted change in this file.
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
| @@ -136,8 +136,20 @@ QueryData genDnsCache(QueryContext& context) { | |||
| PDNSCACHEENTRY pEntry = (PDNSCACHEENTRY)malloc(sizeof(DNSCACHEENTRY)); | |||
There was a problem hiding this comment.
🦩 🔴 dns_cache.cpp does not check for hLib==NULL or DnsGetCacheDataTable==nullptr before calling through the function pointer
In genDnsCache (osquery/tables/system/windows/dns_cache.cpp), added a null check on hLib immediately after LoadLibraryExW returns; on failure it logs a warning, frees pEntry, and returns empty results without calling GetProcAddress. Added a second check on DnsGetCacheDataTable after GetProcAddress; if null, it logs a warning, frees pEntry, frees the library (if loaded), and returns before calling through the function pointer. This prevents the null function pointer dereference.
🤖 Prompt for AI agents
In osquery/tables/system/windows/dns_cache.cpp around line 136, review and complete this code-review fix: dns_cache.cpp does not check for hLib==NULL or DnsGetCacheDataTable==nullptr before calling through the function pointer.
What the draft fix changed: In genDnsCache (osquery/tables/system/windows/dns_cache.cpp), added a null check on `hLib` immediately after LoadLibraryExW returns; on failure it logs a warning, frees `pEntry`, and returns empty results without calling GetProcAddress. Added a second check on `DnsGetCacheDataTable` after GetProcAddress; if null, it logs a warning, frees `pEntry`, frees the library (if loaded), and returns before calling through the function pointer. This prevents the null function pointer dereference.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| PDNSCACHEENTRY pEntry = (PDNSCACHEENTRY)malloc(sizeof(DNSCACHEENTRY)); | ||
| HINSTANCE hLib = | ||
| LoadLibraryExW(L"DNSAPI.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32); | ||
| if (hLib == NULL) { | ||
| LOG(WARNING) << "Failed to load DNSAPI.dll"; | ||
| free(pEntry); | ||
| return results; | ||
| } | ||
|
|
||
| DNS_GET_CACHE_DATA_TABLE DnsGetCacheDataTable = | ||
| (DNS_GET_CACHE_DATA_TABLE)GetProcAddress(hLib, "DnsGetCacheDataTable"); | ||
| if (DnsGetCacheDataTable == nullptr) { | ||
| LOG(WARNING) << "Failed to resolve DnsGetCacheDataTable"; | ||
| free(pEntry); | ||
| FreeLibrary(hLib); | ||
| return results; | ||
| } | ||
|
|
||
| int stat = DnsGetCacheDataTable(pEntry); | ||
| pEntry = pEntry->pNext; |
There was a problem hiding this comment.
🦩 🟠 dns_cache.cpp never frees the loaded DNSAPI.dll library handle
In genDnsCache, added FreeLibrary(hLib) calls: one on the new GetProcAddress-failure early-return path, and one right before the final return results; after the normal success path, ensuring the DNSAPI.dll handle is always released.
🤖 Prompt for AI agents
In osquery/tables/system/windows/dns_cache.cpp around line 133, review and complete this code-review fix: dns_cache.cpp never frees the loaded DNSAPI.dll library handle.
What the draft fix changed: In genDnsCache, added `FreeLibrary(hLib)` calls: one on the new GetProcAddress-failure early-return path, and one right before the final `return results;` after the normal success path, ensuring the DNSAPI.dll handle is always released.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| } | ||
| row["charged"] = INTEGER(bs.Capacity == bi.FullChargedCapacity); | ||
| row["current_capacity"] = INTEGER(bs.Capacity / designedVoltage); | ||
| row["voltage"] = INTEGER(bs.Voltage); |
There was a problem hiding this comment.
🦩 🟠 Battery amperage sign is unconditionally positive even when discharging, contradicting macOS battery table semantics
In genBatteryInfo (osquery/tables/system/windows/battery.cpp), added an isDischarging flag set when bs.PowerState & BATTERY_DISCHARGING is true, and negated the computed amperage value when discharging before assigning it to row["amperage"], matching macOS battery table semantics (negative while discharging, positive while charging/on AC). The change is scoped entirely within the BATTERY_STATUS handling block and does not alter any other field or control flow.
🤖 Prompt for AI agents
In osquery/tables/system/windows/battery.cpp around line 218, review and complete this code-review fix: Battery amperage sign is unconditionally positive even when discharging, contradicting macOS battery table semantics.
What the draft fix changed: In genBatteryInfo (osquery/tables/system/windows/battery.cpp), added an `isDischarging` flag set when `bs.PowerState & BATTERY_DISCHARGING` is true, and negated the computed amperage value when discharging before assigning it to `row["amperage"]`, matching macOS battery table semantics (negative while discharging, positive while charging/on AC). The change is scoped entirely within the BATTERY_STATUS handling block and does not alter any other field or control flow.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| return Status(1, "SQL plugin must include a request action"); | ||
| } | ||
|
|
||
| if (request.at("action") == "query") { |
There was a problem hiding this comment.
🦩 🟠 SQLPlugin::call uses request.at("action") repeatedly without checking key existence for sub-keys like "query"/"table", risking std::out_of_range exception
In SQLPlugin::call (osquery/sql/sql.cpp), added request.count("query") == 0 / request.count("table") == 0 checks (returning Status::failure via Status(1, ...)) before each request.at("query")/request.at("table") call in the "query", "columns", "attach", "detach", and "tables" branches, and cached request.at("action") into a local reference to avoid repeated lookups. This prevents std::out_of_range from propagating out of the Status-returning API when required sub-keys are missing, per OSQUERY-002-2/OSQUERY-003.
🤖 Prompt for AI agents
In osquery/sql/sql.cpp around line 144, review and complete this code-review fix: SQLPlugin::call uses request.at("action") repeatedly without checking key existence for sub-keys like "query"/"table", risking std::out_of_range exception.
What the draft fix changed: In SQLPlugin::call (osquery/sql/sql.cpp), added request.count("query") == 0 / request.count("table") == 0 checks (returning Status::failure via Status(1, ...)) before each request.at("query")/request.at("table") call in the "query", "columns", "attach", "detach", and "tables" branches, and cached request.at("action") into a local reference to avoid repeated lookups. This prevents std::out_of_range from propagating out of the Status-returning API when required sub-keys are missing, per OSQUERY-002-2/OSQUERY-003.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| @@ -120,10 +120,10 @@ int getPrinterSharingStatus() { | |||
| int ret = cupsAdminGetServerSettings(cups, &num_settings, &settings); | |||
There was a problem hiding this comment.
🦩 🟠 getPrinterSharingStatus leaks cups_option_t settings array when cupsAdminGetServerSettings fails
In getPrinterSharingStatus (sharing_preferences.cpp), moved the cupsFreeOptions(num_settings, settings) call out of the success-only branch of the if (ret != 0) block so it is now called unconditionally after the if/else, ensuring settings allocated by cupsAdminGetServerSettings are freed regardless of success or failure. cupsFreeOptions safely handles num_settings == 0 / settings == nullptr on the failure path, so no double-free or null-deref risk is introduced.
🤖 Prompt for AI agents
In osquery/tables/system/darwin/sharing_preferences.cpp around line 120, review and complete this code-review fix: getPrinterSharingStatus leaks cups_option_t settings array when cupsAdminGetServerSettings fails.
What the draft fix changed: In getPrinterSharingStatus (sharing_preferences.cpp), moved the cupsFreeOptions(num_settings, settings) call out of the success-only branch of the `if (ret != 0)` block so it is now called unconditionally after the if/else, ensuring settings allocated by cupsAdminGetServerSettings are freed regardless of success or failure. cupsFreeOptions safely handles num_settings == 0 / settings == nullptr on the failure path, so no double-free or null-deref risk is introduced.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
|
|
||
| ExpectedDecompressData decompressLZxpress(std::vector<UCHAR>& prefetch_data, | ||
| unsigned long size) { | ||
| if (prefetch_data.size() < 8) { | ||
| return ExpectedDecompressData::failure( | ||
| ConversionError::InvalidArgument, | ||
| "Prefetch data too small to decompress"); | ||
| } | ||
|
|
||
| RTLGETCOMPRESSIONWORKSPACESIZE RtlGetCompressionWorkSpaceSize; | ||
| RTLDECOMPRESSBUFFEREX RtlDecompressBufferEx; | ||
|
|
There was a problem hiding this comment.
🦩 🔵 decompressLZxpress reads prefetch_data[8] onward without validating prefetch_data.size() >= 8
In decompressLZxpress (osquery/utils/windows/lzxpress.cpp), added an explicit guard if (prefetch_data.size() < 8) at the top of the function that returns ExpectedDecompressData::failure with ConversionError::InvalidArgument before buffer_size is computed. This prevents the size_t underflow in prefetch_data.size() - 8 and the subsequent out-of-bounds read via &prefetch_data[8] when the input vector has fewer than 8 elements.
(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/windows/lzxpress.cpp around line 75, review and complete this code-review fix: decompressLZxpress reads prefetch_data[8] onward without validating prefetch_data.size() >= 8.
What the draft fix changed: In decompressLZxpress (osquery/utils/windows/lzxpress.cpp), added an explicit guard `if (prefetch_data.size() < 8)` at the top of the function that returns ExpectedDecompressData::failure with ConversionError::InvalidArgument before buffer_size is computed. This prevents the size_t underflow in `prefetch_data.size() - 8` and the subsequent out-of-bounds read via `&prefetch_data[8]` when the input vector has fewer than 8 elements.
_(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
| @@ -62,8 +67,8 @@ QueryData genArpCache(QueryContext& context) { | |||
| r["interface"] = fields[5]; | |||
|
|
|||
| // Note: it's also possible to detect publish entries (ATF_PUB). | |||
There was a problem hiding this comment.
🦩 🔵 hardcoded literal used for ATF_COM|ATF_PERM flag comparison instead of named constant
In genArpCache() in osquery/tables/networking/linux/arp_cache.cpp, replaced the magic-string comparison fields[2] == "0x6" with a numeric parse (strtoul base 16) of the flags field masked against newly introduced named constants kAtfCom (0x02) and kAtfPerm (0x04), matching the kernel's ATF_COM/ATF_PERM bit definitions. This makes the check robust to formatting variations (case, leading zeros, extra flag bits) since it now performs an actual bitwise AND against the combined mask rather than exact string equality. Added <cstdlib> include for strtoul.
🤖 Prompt for AI agents
In osquery/tables/networking/linux/arp_cache.cpp around line 64, review and complete this code-review fix: hardcoded literal used for ATF_COM|ATF_PERM flag comparison instead of named constant.
What the draft fix changed: In genArpCache() in osquery/tables/networking/linux/arp_cache.cpp, replaced the magic-string comparison `fields[2] == "0x6"` with a numeric parse (`strtoul` base 16) of the flags field masked against newly introduced named constants `kAtfCom` (0x02) and `kAtfPerm` (0x04), matching the kernel's ATF_COM/ATF_PERM bit definitions. This makes the check robust to formatting variations (case, leading zeros, extra flag bits) since it now performs an actual bitwise AND against the combined mask rather than exact string equality. Added `<cstdlib>` include for `strtoul`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| int result = static_cast<int>(syscall(SYS_setns, original_mnt_fd_, 0)); | ||
|
|
||
| if (result < 0) { | ||
| // We failed to restore the original mount namespace. Since this | ||
| // worker process may be reused for subsequent queries while | ||
| // keep_process_open_ is still true, force it to be treated as | ||
| // unusable so the caller will not keep it around in a stale | ||
| // namespace. | ||
| keep_process_open_ = false; | ||
|
|
||
| auto status = Status::failure( | ||
| "Failed to restore the original mount namespace, due to error: " + | ||
| std::to_string(errno)); |
There was a problem hiding this comment.
🦩 🔴 setns() switches namespace without restoring on early-return error paths, leaving worker in wrong mount namespace
In LinuxTableContainerIPC::handleJob() (worker-side, linux_table_container_ipc.cpp), when the final restore setns(original_mnt_fd_, ...) fails inside the keep_process_open_ block, the code now sets keep_process_open_ = false before returning the failure Status. This flag is also checked inside executeQueryJobs()'s loop (added a if (!keep_process_open_) break; after processing a message), so that once a job leaves the worker in a bad/unrestored namespace state, the worker process will exit its keep-open loop and terminate (via the existing std::_Exit at the end of executeQueryJobs) rather than being reused for a subsequent query while stuck in a stale mount namespace. This relies on keep_process_open_ being a plain member checked synchronously in the same thread/process (true here, since worker is single-threaded per process), so it should correctly stop reuse. Residual risk: the parent-side caller in generateInNamespace/retrieveQueryDataFromContainer does not know the worker self-terminated early beyond the failure Status already returned by handleJob over IPC, but since the child process actually exits after the loop breaks, any subsequent connectToContainer call will detect the process is gone and fork a fresh one, which is the safe behavior. A fully robust fix might also want to explicitly signal an immediate exit rather than waiting for the next message-processing iteration, but this covers the finding's core concern (stale-namespace reuse) with a minimal change.
🤖 Prompt for AI agents
In osquery/worker/ipc/linux/linux_table_container_ipc.cpp around line 345, review and complete this code-review fix: setns() switches namespace without restoring on early-return error paths, leaving worker in wrong mount namespace.
What the draft fix changed: In `LinuxTableContainerIPC::handleJob()` (worker-side, `linux_table_container_ipc.cpp`), when the final restore `setns(original_mnt_fd_, ...)` fails inside the `keep_process_open_` block, the code now sets `keep_process_open_ = false` before returning the failure Status. This flag is also checked inside `executeQueryJobs()`'s loop (added a `if (!keep_process_open_) break;` after processing a message), so that once a job leaves the worker in a bad/unrestored namespace state, the worker process will exit its keep-open loop and terminate (via the existing `std::_Exit` at the end of `executeQueryJobs`) rather than being reused for a subsequent query while stuck in a stale mount namespace. This relies on `keep_process_open_` being a plain member checked synchronously in the same thread/process (true here, since worker is single-threaded per process), so it should correctly stop reuse. Residual risk: the parent-side caller in `generateInNamespace`/`retrieveQueryDataFromContainer` does not know the worker self-terminated early beyond the failure Status already returned by `handleJob` over IPC, but since the child process actually exits after the loop breaks, any subsequent `connectToContainer` call will detect the process is gone and fork a fresh one, which is the safe behavior. A fully robust fix might also want to explicitly signal an immediate exit rather than waiting for the next message-processing iteration, but this covers the finding's core concern (stale-namespace reuse) with a minimal change.
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
| std::string original_mnt_path = | ||
| kProc + "/" + std::to_string(current_pid) + kMountNamespace; | ||
|
|
||
| if (original_mnt_fd_ > 0) { |
There was a problem hiding this comment.
🦩 🟠 original_mnt_fd_ leaked/overwritten check uses wrong sentinel value
In connectToContainer(), changed the guard if (original_mnt_fd_ > 0) to if (original_mnt_fd_ >= 0) exactly as suggested, so a previously opened fd valued 0 is no longer leaked/skipped when closing before reassigning original_mnt_fd_.
🤖 Prompt for AI agents
In osquery/worker/ipc/linux/linux_table_container_ipc.cpp around line 153, review and complete this code-review fix: original_mnt_fd_ leaked/overwritten check uses wrong sentinel value.
What the draft fix changed: In `connectToContainer()`, changed the guard `if (original_mnt_fd_ > 0)` to `if (original_mnt_fd_ >= 0)` exactly as suggested, so a previously opened fd valued 0 is no longer leaked/skipped when closing before reassigning `original_mnt_fd_`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
|
|
||
| return Status::success(); | ||
| } | ||
| void LinuxTableContainerIPC::stopContainerWorker() { |
There was a problem hiding this comment.
🦩 🟠 stopContainerWorker moves out of the file-scope global current_running_process without any synchronization
Added #include <mutex> and a file-scope std::mutex current_running_process_mutex guarding all accesses to the global current_running_process in connectToContainer() (write under lock when forking) and stopContainerWorker() (move-out under lock into a local child_process, replacing the previous unguarded std::move). This closes the specific race of concurrent read/move/write on the plain global. Residual risk: this only serializes access to the global variable itself; it does not add higher-level locking to prevent two threads from concurrently running connectToContainer/stopContainerWorker logic (e.g., both forking and only the mutex-protected assignment being safe, while the surrounding IPC channel management and process spawning logic is still not thread-safe as a whole). A complete fix would likely require a coarser lock around the whole connect/stop sequence or a documented single-threaded usage contract, which is out of scope for a minimal, targeted change in this file.
🤖 Prompt for AI agents
In osquery/worker/ipc/linux/linux_table_container_ipc.cpp around line 206, review and complete this code-review fix: stopContainerWorker moves out of the file-scope global current_running_process without any synchronization.
What the draft fix changed: Added `#include <mutex>` and a file-scope `std::mutex current_running_process_mutex` guarding all accesses to the global `current_running_process` in `connectToContainer()` (write under lock when forking) and `stopContainerWorker()` (move-out under lock into a local `child_process`, replacing the previous unguarded `std::move`). This closes the specific race of concurrent read/move/write on the plain global. Residual risk: this only serializes access to the global variable itself; it does not add higher-level locking to prevent two threads from concurrently running `connectToContainer`/`stopContainerWorker` logic (e.g., both forking and only the mutex-protected assignment being safe, while the surrounding IPC channel management and process spawning logic is still not thread-safe as a whole). A complete fix would likely require a coarser lock around the whole connect/stop sequence or a documented single-threaded usage contract, which is out of scope for a minimal, targeted change in this file.
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
| @@ -136,8 +136,20 @@ QueryData genDnsCache(QueryContext& context) { | |||
| PDNSCACHEENTRY pEntry = (PDNSCACHEENTRY)malloc(sizeof(DNSCACHEENTRY)); | |||
There was a problem hiding this comment.
🦩 🔴 dns_cache.cpp does not check for hLib==NULL or DnsGetCacheDataTable==nullptr before calling through the function pointer
In genDnsCache (osquery/tables/system/windows/dns_cache.cpp), added a null check on hLib immediately after LoadLibraryExW returns; on failure it logs a warning, frees pEntry, and returns empty results without calling GetProcAddress. Added a second check on DnsGetCacheDataTable after GetProcAddress; if null, it logs a warning, frees pEntry, frees the library (if loaded), and returns before calling through the function pointer. This prevents the null function pointer dereference.
🤖 Prompt for AI agents
In osquery/tables/system/windows/dns_cache.cpp around line 136, review and complete this code-review fix: dns_cache.cpp does not check for hLib==NULL or DnsGetCacheDataTable==nullptr before calling through the function pointer.
What the draft fix changed: In genDnsCache (osquery/tables/system/windows/dns_cache.cpp), added a null check on `hLib` immediately after LoadLibraryExW returns; on failure it logs a warning, frees `pEntry`, and returns empty results without calling GetProcAddress. Added a second check on `DnsGetCacheDataTable` after GetProcAddress; if null, it logs a warning, frees `pEntry`, frees the library (if loaded), and returns before calling through the function pointer. This prevents the null function pointer dereference.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer
| PDNSCACHEENTRY pEntry = (PDNSCACHEENTRY)malloc(sizeof(DNSCACHEENTRY)); | ||
| HINSTANCE hLib = | ||
| LoadLibraryExW(L"DNSAPI.dll", NULL, LOAD_LIBRARY_SEARCH_SYSTEM32); | ||
| if (hLib == NULL) { | ||
| LOG(WARNING) << "Failed to load DNSAPI.dll"; | ||
| free(pEntry); | ||
| return results; | ||
| } | ||
|
|
||
| DNS_GET_CACHE_DATA_TABLE DnsGetCacheDataTable = | ||
| (DNS_GET_CACHE_DATA_TABLE)GetProcAddress(hLib, "DnsGetCacheDataTable"); | ||
| if (DnsGetCacheDataTable == nullptr) { | ||
| LOG(WARNING) << "Failed to resolve DnsGetCacheDataTable"; | ||
| free(pEntry); | ||
| FreeLibrary(hLib); | ||
| return results; | ||
| } | ||
|
|
||
| int stat = DnsGetCacheDataTable(pEntry); | ||
| pEntry = pEntry->pNext; |
There was a problem hiding this comment.
🦩 🟠 dns_cache.cpp never frees the loaded DNSAPI.dll library handle
In genDnsCache, added FreeLibrary(hLib) calls: one on the new GetProcAddress-failure early-return path, and one right before the final return results; after the normal success path, ensuring the DNSAPI.dll handle is always released.
🤖 Prompt for AI agents
In osquery/tables/system/windows/dns_cache.cpp around line 133, review and complete this code-review fix: dns_cache.cpp never frees the loaded DNSAPI.dll library handle.
What the draft fix changed: In genDnsCache, added `FreeLibrary(hLib)` calls: one on the new GetProcAddress-failure early-return path, and one right before the final `return results;` after the normal success path, ensuring the DNSAPI.dll handle is always released.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| } | ||
| row["charged"] = INTEGER(bs.Capacity == bi.FullChargedCapacity); | ||
| row["current_capacity"] = INTEGER(bs.Capacity / designedVoltage); | ||
| row["voltage"] = INTEGER(bs.Voltage); |
There was a problem hiding this comment.
🦩 🟠 Battery amperage sign is unconditionally positive even when discharging, contradicting macOS battery table semantics
In genBatteryInfo (osquery/tables/system/windows/battery.cpp), added an isDischarging flag set when bs.PowerState & BATTERY_DISCHARGING is true, and negated the computed amperage value when discharging before assigning it to row["amperage"], matching macOS battery table semantics (negative while discharging, positive while charging/on AC). The change is scoped entirely within the BATTERY_STATUS handling block and does not alter any other field or control flow.
🤖 Prompt for AI agents
In osquery/tables/system/windows/battery.cpp around line 218, review and complete this code-review fix: Battery amperage sign is unconditionally positive even when discharging, contradicting macOS battery table semantics.
What the draft fix changed: In genBatteryInfo (osquery/tables/system/windows/battery.cpp), added an `isDischarging` flag set when `bs.PowerState & BATTERY_DISCHARGING` is true, and negated the computed amperage value when discharging before assigning it to `row["amperage"]`, matching macOS battery table semantics (negative while discharging, positive while charging/on AC). The change is scoped entirely within the BATTERY_STATUS handling block and does not alter any other field or control flow.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| return Status(1, "SQL plugin must include a request action"); | ||
| } | ||
|
|
||
| if (request.at("action") == "query") { |
There was a problem hiding this comment.
🦩 🟠 SQLPlugin::call uses request.at("action") repeatedly without checking key existence for sub-keys like "query"/"table", risking std::out_of_range exception
In SQLPlugin::call (osquery/sql/sql.cpp), added request.count("query") == 0 / request.count("table") == 0 checks (returning Status::failure via Status(1, ...)) before each request.at("query")/request.at("table") call in the "query", "columns", "attach", "detach", and "tables" branches, and cached request.at("action") into a local reference to avoid repeated lookups. This prevents std::out_of_range from propagating out of the Status-returning API when required sub-keys are missing, per OSQUERY-002-2/OSQUERY-003.
🤖 Prompt for AI agents
In osquery/sql/sql.cpp around line 144, review and complete this code-review fix: SQLPlugin::call uses request.at("action") repeatedly without checking key existence for sub-keys like "query"/"table", risking std::out_of_range exception.
What the draft fix changed: In SQLPlugin::call (osquery/sql/sql.cpp), added request.count("query") == 0 / request.count("table") == 0 checks (returning Status::failure via Status(1, ...)) before each request.at("query")/request.at("table") call in the "query", "columns", "attach", "detach", and "tables" branches, and cached request.at("action") into a local reference to avoid repeated lookups. This prevents std::out_of_range from propagating out of the Status-returning API when required sub-keys are missing, per OSQUERY-002-2/OSQUERY-003.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer
| @@ -120,10 +120,10 @@ int getPrinterSharingStatus() { | |||
| int ret = cupsAdminGetServerSettings(cups, &num_settings, &settings); | |||
There was a problem hiding this comment.
🦩 🟠 getPrinterSharingStatus leaks cups_option_t settings array when cupsAdminGetServerSettings fails
In getPrinterSharingStatus (sharing_preferences.cpp), moved the cupsFreeOptions(num_settings, settings) call out of the success-only branch of the if (ret != 0) block so it is now called unconditionally after the if/else, ensuring settings allocated by cupsAdminGetServerSettings are freed regardless of success or failure. cupsFreeOptions safely handles num_settings == 0 / settings == nullptr on the failure path, so no double-free or null-deref risk is introduced.
🤖 Prompt for AI agents
In osquery/tables/system/darwin/sharing_preferences.cpp around line 120, review and complete this code-review fix: getPrinterSharingStatus leaks cups_option_t settings array when cupsAdminGetServerSettings fails.
What the draft fix changed: In getPrinterSharingStatus (sharing_preferences.cpp), moved the cupsFreeOptions(num_settings, settings) call out of the success-only branch of the `if (ret != 0)` block so it is now called unconditionally after the if/else, ensuring settings allocated by cupsAdminGetServerSettings are freed regardless of success or failure. cupsFreeOptions safely handles num_settings == 0 / settings == nullptr on the failure path, so no double-free or null-deref risk is introduced.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
|
|
||
| ExpectedDecompressData decompressLZxpress(std::vector<UCHAR>& prefetch_data, | ||
| unsigned long size) { | ||
| if (prefetch_data.size() < 8) { | ||
| return ExpectedDecompressData::failure( | ||
| ConversionError::InvalidArgument, | ||
| "Prefetch data too small to decompress"); | ||
| } | ||
|
|
||
| RTLGETCOMPRESSIONWORKSPACESIZE RtlGetCompressionWorkSpaceSize; | ||
| RTLDECOMPRESSBUFFEREX RtlDecompressBufferEx; | ||
|
|
There was a problem hiding this comment.
🦩 🔵 decompressLZxpress reads prefetch_data[8] onward without validating prefetch_data.size() >= 8
In decompressLZxpress (osquery/utils/windows/lzxpress.cpp), added an explicit guard if (prefetch_data.size() < 8) at the top of the function that returns ExpectedDecompressData::failure with ConversionError::InvalidArgument before buffer_size is computed. This prevents the size_t underflow in prefetch_data.size() - 8 and the subsequent out-of-bounds read via &prefetch_data[8] when the input vector has fewer than 8 elements.
(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/windows/lzxpress.cpp around line 75, review and complete this code-review fix: decompressLZxpress reads prefetch_data[8] onward without validating prefetch_data.size() >= 8.
What the draft fix changed: In decompressLZxpress (osquery/utils/windows/lzxpress.cpp), added an explicit guard `if (prefetch_data.size() < 8)` at the top of the function that returns ExpectedDecompressData::failure with ConversionError::InvalidArgument before buffer_size is computed. This prevents the size_t underflow in `prefetch_data.size() - 8` and the subsequent out-of-bounds read via `&prefetch_data[8]` when the input vector has fewer than 8 elements.
_(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
| @@ -62,8 +67,8 @@ QueryData genArpCache(QueryContext& context) { | |||
| r["interface"] = fields[5]; | |||
|
|
|||
| // Note: it's also possible to detect publish entries (ATF_PUB). | |||
There was a problem hiding this comment.
🦩 🔵 hardcoded literal used for ATF_COM|ATF_PERM flag comparison instead of named constant
In genArpCache() in osquery/tables/networking/linux/arp_cache.cpp, replaced the magic-string comparison fields[2] == "0x6" with a numeric parse (strtoul base 16) of the flags field masked against newly introduced named constants kAtfCom (0x02) and kAtfPerm (0x04), matching the kernel's ATF_COM/ATF_PERM bit definitions. This makes the check robust to formatting variations (case, leading zeros, extra flag bits) since it now performs an actual bitwise AND against the combined mask rather than exact string equality. Added <cstdlib> include for strtoul.
🤖 Prompt for AI agents
In osquery/tables/networking/linux/arp_cache.cpp around line 64, review and complete this code-review fix: hardcoded literal used for ATF_COM|ATF_PERM flag comparison instead of named constant.
What the draft fix changed: In genArpCache() in osquery/tables/networking/linux/arp_cache.cpp, replaced the magic-string comparison `fields[2] == "0x6"` with a numeric parse (`strtoul` base 16) of the flags field masked against newly introduced named constants `kAtfCom` (0x02) and `kAtfPerm` (0x04), matching the kernel's ATF_COM/ATF_PERM bit definitions. This makes the check robust to formatting variations (case, leading zeros, extra flag bits) since it now performs an actual bitwise AND against the combined mask rather than exact string equality. Added `<cstdlib>` include for `strtoul`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 34 review findings across 27 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
osquery/worker/ipc/linux/linux_table_container_ipc.cpp:345osquery/worker/ipc/linux/linux_table_container_ipc.cpp:153osquery/worker/ipc/linux/linux_table_container_ipc.cpp:206osquery/tables/system/windows/dns_cache.cpp:136osquery/tables/system/windows/dns_cache.cpp:133osquery/events/darwin/scnetwork.cpp:46osquery/sql/sqlite_filesystem.cpp:181osquery/filesystem/linux/proc.cpp:34osquery/core/windows/wmi.cpp:246osquery/core/windows/wmi.cpp:229osquery/tables/system/linux/md_tables.cpp:120osquery/tables/system/linux/md_tables.cpp:163osquery/tables/system/windows/logical_drives.cpp:28osquery/tables/system/windows/logical_drives.cpp:50osquery/tables/events/linux/hardware_events.cpp:77osquery/tables/events/linux/hardware_events.cpp:57osquery/utils/json/json.cpp:146osquery/events/windows/etw/etw_provider_config.cpp:15osquery/tables/events/windows/etw_process_events.cpp:77tools/codegen/genapi.py:148osquery/tables/events/tests/windows/etw_process_events_tests.cpp:48osquery/tables/system/darwin/usb_devices.cpp:30osquery/tables/system/linux/memory_info.cpp:45osquery/tables/system/linux/processes.cpp:179osquery/events/linux/inotify.cpp:58osquery/remote/requests.cpp:24osquery/tables/system/darwin/processes.cpp:366osquery/utils/system/posix/system.cpp:24osquery/remote/uri.cpp:41osquery/tables/system/windows/battery.cpp:218osquery/sql/sql.cpp:144osquery/tables/system/darwin/sharing_preferences.cpp:120osquery/utils/windows/lzxpress.cpp:75osquery/tables/networking/linux/arp_cache.cpp:64What 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:
39233833-f1d6-416e-93a7-71af15e3e968Merging 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-86akhf8u2 Osquery review findings sweep (15 PRs)