Skip to content

fix(OSQUERY-004-2): 8 review findings across 4 files #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 4 commits into
base: master
Choose a base branch
from
+15 −15
Draft

fix(OSQUERY-004-2): 8 review findings across 4 files #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
efed3c2
fix(OSQUERY-004-2): 8 review findings across 4 files
flamingo[bot] Aug 24, 2026
38e9b87
fix(OSQUERY-004-2): 8 review findings across 4 files
flamingo[bot] Aug 24, 2026
45955a7
fix(OSQUERY-004-2): 8 review findings across 4 files
flamingo[bot] Aug 24, 2026
f7b4a0b
fix(OSQUERY-004-2): 8 review findings across 4 files
flamingo[bot] Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions osquery/core/windows/wmi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,13 +77,13 @@ void WmiResultItem::PrintType(const std::string& name) const {
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);
Comment on lines 77 to 89

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand Down Expand Up @@ -238,7 +238,7 @@ Status WmiResultItem::GetUnsignedLong(const std::string& name,
VariantClear(&value);
return Status::failure("Invalid data type returned.");
}
ret = value.lVal;

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

ret = value.ulVal;
VariantClear(&value);
return Status::success();
}
Expand All @@ -255,7 +255,7 @@ Status WmiResultItem::GetLongLong(const std::string& name,
VariantClear(&value);
return Status::failure("Invalid data type returned.");
}
ret = value.lVal;
ret = value.llVal;
VariantClear(&value);
return Status::success();
}
Expand All @@ -272,7 +272,7 @@ Status WmiResultItem::GetUnsignedLongLong(const std::string& name,
VariantClear(&value);

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

return Status::failure("Invalid data type returned.");
}
ret = value.lVal;
ret = value.ullVal;
VariantClear(&value);
return Status::success();
}
Expand Down
4 changes: 3 additions & 1 deletion osquery/events/windows/etw/etw_user_session.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ void UserEtwSessionRunnable::resume() {
void UserEtwSessionRunnable::initUserTraceSession(
const std::string& sessionName) {
if (sessionName.empty()) {
LOG(ERROR) << "UserTraceSession does not have a name.";
return;
}

Expand All @@ -176,6 +177,7 @@ void UserEtwSessionRunnable::initUserTraceSession(
void UserEtwSessionRunnable::stopUserTraceSession(
const std::string& sessionName) {
if (sessionName.empty()) {
LOG(ERROR) << "Failed to stop user trace session, session name is empty.";
return;
}

Comment on lines 177 to 183

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

Expand All @@ -201,4 +203,4 @@ void UserEtwSessionRunnable::stopUserTraceSession(
LOG(WARNING) << "ControlTrace() failed with error code " << retCtrl;
}
}
} // namespace osquery
} // namespace osquery
2 changes: 1 addition & 1 deletion plugins/config/parsers/file_paths.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ void FilePathsConfigParserPlugin::updateFilePathsQuery(
for (const auto& row : sql.rows()) {
auto pathIt = row.find("path");
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 '"

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

<< query.GetString() << "'";
} else {
Expand Down
10 changes: 4 additions & 6 deletions plugins/logger/filesystem_logger.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,23 @@ namespace {
bool validateLoggerMode(const char* flagname, const std::string& value) {
// Account for leading 0, special bit, and normal permissions
if (value.size() > 5) {
osquery::systemLog(kLoggerModeInvalidValueError);
std::cerr << kLoggerModeInvalidValueError << std::endl;

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

LOG(ERROR) << kLoggerModeInvalidValueError;

return false;
}

const auto logger_mode_octal_exp = tryTo<std::int32_t>(value, 8);

if (logger_mode_octal_exp.isError()) {
osquery::systemLog(kLoggerModeConversionFailureError);

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

std::cerr << kLoggerModeConversionFailureError << std::endl;
LOG(ERROR) << kLoggerModeConversionFailureError;

return false;
}

const auto logger_mode_octal = logger_mode_octal_exp.get();

if (logger_mode_octal <= 0 || logger_mode_octal > 07777) {

@flamingo flamingo Bot Aug 24, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 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

osquery::systemLog(kLoggerModeInvalidValueError);
std::cerr << kLoggerModeInvalidValueError << std::endl;
LOG(ERROR) << kLoggerModeInvalidValueError;
return false;
}

Expand Down Expand Up @@ -320,3 +317,4 @@ void FilesystemLoggerPlugin::init(const std::string& name,
FLAGS_stderrthreshold = stderr_threshold;
}
} // namespace osquery