fix(OSQUERY-004-2): 8 review findings across 4 files - #43
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| VARIANT value; | ||
| HRESULT hr = result_->Get(property_name.c_str(), 0, &value, nullptr, nullptr); | ||
| if (hr != S_OK) { | ||
| std::cerr << "Failed: " << name << "\n"; | ||
| LOG(INFO) << "Failed: " << name; | ||
| } else { | ||
| std::cout << "Name=" << name << ", Type=" << value.vt << "\n"; | ||
| VLOG(1) << "Name=" << name << ", Type=" << value.vt; | ||
| if (value.vt == VT_I4) { | ||
| std::cout << " Value=" << value.lVal << "\n"; | ||
| VLOG(1) << " Value=" << value.lVal; | ||
| } else if (value.vt == VT_BSTR) { | ||
| std::wcout << " Value=" << value.bstrVal << "\n"; | ||
| VLOG(1) << " Value=" << wstringToString(value.bstrVal); | ||
| } | ||
| } | ||
| VariantClear(&value); |
There was a problem hiding this comment.
🦩 🟠 WMI PrintType() uses std::cerr/std::cout instead of osquery LOG/VLOG macros
In WmiResultItem::PrintType, replaced std::cerr with LOG(INFO) for the failure message, and std::cout/std::wcout with VLOG(1) for the diagnostic type/value output; the wide-string bstrVal is converted via existing wstringToString() helper to be compatible with the ostream-based VLOG macro instead of std::wcout.
🤖 Prompt for AI agents
In osquery/core/windows/wmi.cpp around line 66, review and complete this code-review fix: WMI PrintType() uses std::cerr/std::cout instead of osquery LOG/VLOG macros.
What the draft fix changed: In WmiResultItem::PrintType, replaced std::cerr with LOG(INFO) for the failure message, and std::cout/std::wcout with VLOG(1) for the diagnostic type/value output; the wide-string bstrVal is converted via existing wstringToString() helper to be compatible with the ostream-based VLOG macro instead of std::wcout.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| VariantClear(&value); | ||
| return Status::failure("Invalid data type returned."); | ||
| } | ||
| ret = value.lVal; |
There was a problem hiding this comment.
🦩 🟠 WmiResultItem::GetUnsignedLong and GetLongLong read the wrong VARIANT union member for wide types
In WmiResultItem::GetUnsignedLong, changed ret = value.lVal; to ret = value.ulVal; to read the correct unsigned LONG union member for VT_UI4. In WmiResultItem::GetLongLong, changed ret = value.lVal; to ret = value.llVal; to read the correct 64-bit signed LONGLONG member for VT_I8.
🤖 Prompt for AI agents
In osquery/core/windows/wmi.cpp around line 241, review and complete this code-review fix: WmiResultItem::GetUnsignedLong and GetLongLong read the wrong VARIANT union member for wide types.
What the draft fix changed: In WmiResultItem::GetUnsignedLong, changed `ret = value.lVal;` to `ret = value.ulVal;` to read the correct unsigned LONG union member for VT_UI4. In WmiResultItem::GetLongLong, changed `ret = value.lVal;` to `ret = value.llVal;` to read the correct 64-bit signed LONGLONG member for VT_I8.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| @@ -272,7 +272,7 @@ Status WmiResultItem::GetUnsignedLongLong(const std::string& name, | |||
| VariantClear(&value); | |||
There was a problem hiding this comment.
🦩 🟠 GetUnsignedLongLong reads 32-bit value.lVal instead of the 64-bit unsigned member for VT_UI8
In WmiResultItem::GetUnsignedLongLong, changed ret = value.lVal; to ret = value.ullVal; to read the correct 64-bit unsigned ULONGLONG member for VT_UI8.
🤖 Prompt for AI agents
In osquery/core/windows/wmi.cpp around line 272, review and complete this code-review fix: GetUnsignedLongLong reads 32-bit value.lVal instead of the 64-bit unsigned member for VT_UI8.
What the draft fix changed: In WmiResultItem::GetUnsignedLongLong, changed `ret = value.lVal;` to `ret = value.ullVal;` to read the correct 64-bit unsigned ULONGLONG member for VT_UI8.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| // Account for leading 0, special bit, and normal permissions | ||
| if (value.size() > 5) { | ||
| osquery::systemLog(kLoggerModeInvalidValueError); | ||
| std::cerr << kLoggerModeInvalidValueError << std::endl; |
There was a problem hiding this comment.
🦩 🟠 std::cerr used for diagnostic output instead of LOG() macros in validateLoggerMode
In validateLoggerMode() (size check branch), replaced osquery::systemLog(kLoggerModeInvalidValueError); std::cerr << kLoggerModeInvalidValueError << std::endl; with a single LOG(ERROR) << kLoggerModeInvalidValueError;, routing diagnostic output through the Glog-based LOG() macro and removing the redundant systemLog/cerr duplication as suggested.
🤖 Prompt for AI agents
In plugins/logger/filesystem_logger.cpp around line 36, review and complete this code-review fix: std::cerr used for diagnostic output instead of LOG() macros in validateLoggerMode.
What the draft fix changed: In validateLoggerMode() (size check branch), replaced `osquery::systemLog(kLoggerModeInvalidValueError); std::cerr << kLoggerModeInvalidValueError << std::endl;` with a single `LOG(ERROR) << kLoggerModeInvalidValueError;`, routing diagnostic output through the Glog-based LOG() macro and removing the redundant systemLog/cerr duplication as suggested.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| const auto logger_mode_octal_exp = tryTo<std::int32_t>(value, 8); | ||
|
|
||
| if (logger_mode_octal_exp.isError()) { | ||
| osquery::systemLog(kLoggerModeConversionFailureError); |
There was a problem hiding this comment.
🦩 🟠 std::cerr used for diagnostic output instead of LOG() macros (octal conversion failure path)
In validateLoggerMode() (octal conversion failure branch), replaced osquery::systemLog(kLoggerModeConversionFailureError); std::cerr << kLoggerModeConversionFailureError << std::endl; with LOG(ERROR) << kLoggerModeConversionFailureError;.
🤖 Prompt for AI agents
In plugins/logger/filesystem_logger.cpp around line 44, review and complete this code-review fix: std::cerr used for diagnostic output instead of LOG() macros (octal conversion failure path).
What the draft fix changed: In validateLoggerMode() (octal conversion failure branch), replaced `osquery::systemLog(kLoggerModeConversionFailureError); std::cerr << kLoggerModeConversionFailureError << std::endl;` with `LOG(ERROR) << kLoggerModeConversionFailureError;`.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
|
|
||
| const auto logger_mode_octal = logger_mode_octal_exp.get(); | ||
|
|
||
| if (logger_mode_octal <= 0 || logger_mode_octal > 07777) { |
There was a problem hiding this comment.
🦩 🟠 std::cerr used instead of LOG() macro for logger_mode range validation error
In validateLoggerMode() (range validation branch), replaced osquery::systemLog(kLoggerModeInvalidValueError); std::cerr << kLoggerModeInvalidValueError << std::endl; with LOG(ERROR) << kLoggerModeInvalidValueError;. Note: <iostream> include and #include <osquery/logger/data_logger.h> (for systemLog) are left in place since removal wasn't required by the findings and other code in the file may still rely on them; LOG() macro availability is provided via the existing <osquery/logger/logger.h> include, so no new include was needed.
🤖 Prompt for AI agents
In plugins/logger/filesystem_logger.cpp around line 52, review and complete this code-review fix: std::cerr used instead of LOG() macro for logger_mode range validation error.
What the draft fix changed: In validateLoggerMode() (range validation branch), replaced `osquery::systemLog(kLoggerModeInvalidValueError); std::cerr << kLoggerModeInvalidValueError << std::endl;` with `LOG(ERROR) << kLoggerModeInvalidValueError;`. Note: `<iostream>` include and `#include <osquery/logger/data_logger.h>` (for systemLog) are left in place since removal wasn't required by the findings and other code in the file may still rely on them; LOG() macro availability is provided via the existing `<osquery/logger/logger.h>` include, so no new include was needed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer
| if (pathIt == row.end()) { | ||
| LOG(ERROR) << "Cold not find non-empty 'path' column in the " | ||
| LOG(ERROR) << "Could not find non-empty 'path' column in the " | ||
| "results of file_paths_query '" |
There was a problem hiding this comment.
🦩 🟠 LOG(ERROR) message typo 'Cold not find' in file_paths.cpp
Fixed the typo "Cold not find" to "Could not find" in the LOG(ERROR) message inside FilePathsConfigParserPlugin::updateFilePathsQuery, at the branch handling a missing 'path' column in query results.
🤖 Prompt for AI agents
In plugins/config/parsers/file_paths.cpp around line 148, review and complete this code-review fix: LOG(ERROR) message typo 'Cold not find' in file_paths.cpp.
What the draft fix changed: Fixed the typo "Cold not find" to "Could not find" in the LOG(ERROR) message inside FilePathsConfigParserPlugin::updateFilePathsQuery, at the branch handling a missing 'path' column in query results.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 98 high — react 👍/👎 to teach the reviewer
| void UserEtwSessionRunnable::stopUserTraceSession( | ||
| const std::string& sessionName) { | ||
| if (sessionName.empty()) { | ||
| LOG(ERROR) << "Failed to stop user trace session, session name is empty."; | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 initUserTraceSession silently returns on empty session name without logging, unlike its kernel counterpart
Added LOG(ERROR) << "UserTraceSession does not have a name."; in UserEtwSessionRunnable::initUserTraceSession before the early return on empty sessionName, and added LOG(ERROR) << "Failed to stop user trace session, session name is empty."; in UserEtwSessionRunnable::stopUserTraceSession before its early return on empty sessionName, mirroring the logging behavior of the kernel counterpart functions.
🤖 Prompt for AI agents
In osquery/events/windows/etw/etw_user_session.cpp around line 175, review and complete this code-review fix: initUserTraceSession silently returns on empty session name without logging, unlike its kernel counterpart.
What the draft fix changed: Added `LOG(ERROR) << "UserTraceSession does not have a name.";` in `UserEtwSessionRunnable::initUserTraceSession` before the early return on empty `sessionName`, and added `LOG(ERROR) << "Failed to stop user trace session, session name is empty.";` in `UserEtwSessionRunnable::stopUserTraceSession` before its early return on empty `sessionName`, mirroring the logging behavior of the kernel counterpart functions.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
Closes 8 review findings across 4 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
osquery/core/windows/wmi.cpp:66osquery/core/windows/wmi.cpp:241osquery/core/windows/wmi.cpp:272plugins/logger/filesystem_logger.cpp:36plugins/logger/filesystem_logger.cpp:44plugins/logger/filesystem_logger.cpp:52plugins/config/parsers/file_paths.cpp:148osquery/events/windows/etw/etw_user_session.cpp:175What 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.