Repository navigation
fix(OSQUERY-004): CU-86akhf8u2 2 review findings across 2 files - #102
flamingo[bot] wants to merge 2 commits into
Conversation
| @@ -100,7 +100,7 @@ void YARACompilerCallback(int error_level, | |||
| else | |||
| ss << "YARA rule file " << file_name; | |||
| if (error_level == YARA_ERROR_LEVEL_ERROR) { | |||
There was a problem hiding this comment.
🦩 🟠 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
| 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); | ||
| } | ||
|
|
There was a problem hiding this comment.
🦩 🟠 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
Closes 2 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
osquery/tables/yara/yara_utils.cpp:102osquery/utils/conversions/windows/strings.cpp:26What 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:
46e37628-c6c9-49a4-aa29-9b30b2c8be3fMerging 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)