Skip to content

fix(OSQUERY-004): CU-86akhf8u2 2 review findings across 2 files #102

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 2 commits into
base: master
Choose a base branch
from
Draft

fix(OSQUERY-004): CU-86akhf8u2 2 review findings across 2 files #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8467eaf
fix(OSQUERY-004): 2 review findings across 2 files
flamingo[bot] Sep 28, 2026
85633e6
fix(OSQUERY-004): 2 review findings across 2 files
flamingo[bot] Sep 28, 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
3 changes: 2 additions & 1 deletion osquery/tables/yara/yara_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ void YARACompilerCallback(int error_level,
else
ss << "YARA rule file " << file_name;
if (error_level == YARA_ERROR_LEVEL_ERROR) {

@flamingo flamingo Bot Sep 28, 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.

🦩 🟠 YARA compilation error/warning messages logged only at VLOG(1), losing visibility on real compile errors

In YARACompilerCallback (osquery/tables/yara/yara_utils.cpp), changed the error_level == YARA_ERROR_LEVEL_ERROR branch from VLOG(1) to LOG(ERROR) so compile errors are always visible in normal operation; the warning branch remains VLOG(1) as suggested, leaving that behavior unchanged.

🤖 Prompt for AI agents 
In osquery/tables/yara/yara_utils.cpp around line 102, review and complete this code-review fix: YARA compilation error/warning messages logged only at VLOG(1), losing visibility on real compile errors.
What the draft fix changed: In `YARACompilerCallback` (osquery/tables/yara/yara_utils.cpp), changed the `error_level == YARA_ERROR_LEVEL_ERROR` branch from `VLOG(1)` to `LOG(ERROR)` so compile errors are always visible in normal operation; the warning branch remains `VLOG(1)` as suggested, leaving that behavior unchanged.
Verify the change is correct and complete; do not refactor unrelated code.

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

VLOG(1) << ss.str() << "(" << line_number << "): error: " << message;
LOG(ERROR) << ss.str() << "(" << line_number << "): error: " << message;
} else {
VLOG(1) << ss.str() << "(" << line_number << "): warning: " << message;
}
Expand Down Expand Up @@ -472,3 +472,4 @@ Status YARAConfigParserPlugin::update(const std::string& source,
/// Call the simple YARA ConfigParserPlugin "yara".
REGISTER(YARAConfigParserPlugin, "config_parser", "yara");
} // namespace osquery

13 changes: 13 additions & 0 deletions osquery/utils/conversions/windows/strings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ struct utf_converter {
result.resize(str.length() * 2);
auto count = MultiByteToWideChar(
CP_UTF8, 0, str.c_str(), -1, &result[0], str.length() * 2);
if (count <= 0) {
LOG(WARNING) << "Failed to convert string to wstring, "
"MultiByteToWideChar error "
<< GetLastError();
return std::wstring();
}
result.resize(count - 1);
}

Comment on lines 29 to 40

@flamingo flamingo Bot Sep 28, 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.

🦩 🟠 Silent truncation when MultiByteToWideChar/WideCharToMultiByte fail (count==0) is not logged

In utf_converter::from_bytes and utf_converter::to_bytes (osquery/utils/conversions/windows/strings.cpp), added a check if (count <= 0) right after the MultiByteToWideChar/WideCharToMultiByte calls. On failure, a LOG(WARNING) message including GetLastError() is emitted and the function returns an empty std::wstring()/std::string() instead of calling resize(count - 1) with an underflowed unsigned value. This prevents the length_error/bad_alloc/misbehavior described in the finding and satisfies the OSQUERY-004 logging requirement. Not converted to a Status-returning API since callers (stringToWstring/wstringToString) and the class's public interface were left unchanged to keep the fix minimal; a more complete fix would propagate failure via Status through the public wrappers, which is a larger, riskier change spanning callers outside this file.

🤖 Prompt for AI agents 
In osquery/utils/conversions/windows/strings.cpp around line 26, review and complete this code-review fix: Silent truncation when MultiByteToWideChar/WideCharToMultiByte fail (count==0) is not logged.
What the draft fix changed: In `utf_converter::from_bytes` and `utf_converter::to_bytes` (osquery/utils/conversions/windows/strings.cpp), added a check `if (count <= 0)` right after the `MultiByteToWideChar`/`WideCharToMultiByte` calls. On failure, a `LOG(WARNING)` message including `GetLastError()` is emitted and the function returns an empty `std::wstring()`/`std::string()` instead of calling `resize(count - 1)` with an underflowed unsigned value. This prevents the length_error/bad_alloc/misbehavior described in the finding and satisfies the OSQUERY-004 logging requirement. Not converted to a `Status`-returning API since callers (`stringToWstring`/`wstringToString`) and the class's public interface were left unchanged to keep the fix minimal; a more complete fix would propagate failure via `Status` through the public wrappers, which is a larger, riskier change spanning callers outside this file.
Verify the change is correct and complete; do not refactor unrelated code.

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

Expand All @@ -47,6 +53,12 @@ struct utf_converter {
str.length() * 4,
NULL,
NULL);
if (count <= 0) {
LOG(WARNING) << "Failed to convert wstring to string, "
"WideCharToMultiByte error "
<< GetLastError();
return std::string();
}
result.resize(count - 1);
}

Expand Down Expand Up @@ -169,3 +181,4 @@ std::string errorDwordToString(DWORD error_code) {
}

} // namespace osquery

Loading