Repository navigation
fix(OSQUERY-004-2): CU-17tkuw5wp49 4 review findings across 4 files - #117
flamingo[bot] wants to merge 4 commits into
Conversation
| ::execve(arguments[0], argv, ::environ); | ||
|
|
||
| // Code should never reach this point | ||
| std::cerr << "Could not start extension process: " << exec_path |
There was a problem hiding this comment.
🦩 🔵 std::cerr used for diagnostics in production process.cpp code
Replaced both std::cerr diagnostic statements in osquery/process/posix/process.cpp with LOG(ERROR) macros: in launchExtension (previously logging "Could not start extension process...") and in launchTestPythonScript (previously logging "Could not start process at..."). Both now route through the standard osquery logging pipeline as required by OSQUERY-004/OSQUERY-004-2. The now-unused <iostream> include was left in place since it is not part of the specific finding and removing it risks breaking other unseen usages; this is a conservative choice that could be flagged separately as an unused include.
🤖 Prompt for AI agents
In osquery/process/posix/process.cpp around line 189, review and complete this code-review fix: std::cerr used for diagnostics in production process.cpp code.
What the draft fix changed: Replaced both std::cerr diagnostic statements in osquery/process/posix/process.cpp with LOG(ERROR) macros: in launchExtension (previously logging "Could not start extension process...") and in launchTestPythonScript (previously logging "Could not start process at..."). Both now route through the standard osquery logging pipeline as required by OSQUERY-004/OSQUERY-004-2. The now-unused `<iostream>` include was left in place since it is not part of the specific finding and removing it risks breaking other unseen usages; this is a conservative choice that could be flagged separately as an unused include.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| @@ -231,7 +231,7 @@ static auto getClient() { | |||
| } | |||
|
|
|||
| void printRawStderr(const std::string& s) { | |||
There was a problem hiding this comment.
🦩 🔵 printRawStderr bypasses LOG/VLOG macros in TLS transport
Changed printRawStderr() (osquery/remote/transports/tls.cpp) to use VLOG(1) << s; instead of fprintf(stderr, "%s\n", s.c_str());, routing diagnostic output through osquery's logging macros as mandated by OSQUERY-004/OSQUERY-004-2. Updated the adjacent comments at the three call sites in sendRequest() to reflect that the body is now logged via VLOG(1) gated behind FLAGS_verbose && FLAGS_tls_dump rather than written raw to stderr. Risk: this does change runtime behavior (output now goes through glog's VLOG sink instead of unconditionally to stderr), which is exactly what the finding requested, but a reviewer should confirm this behavior change (log destination/formatting differences, e.g. VLOG prepending glog's line prefix) is acceptable for this diagnostic dump path.
🤖 Prompt for AI agents
In osquery/remote/transports/tls.cpp around line 233, review and complete this code-review fix: printRawStderr bypasses LOG/VLOG macros in TLS transport.
What the draft fix changed: Changed `printRawStderr()` (osquery/remote/transports/tls.cpp) to use `VLOG(1) << s;` instead of `fprintf(stderr, "%s\n", s.c_str());`, routing diagnostic output through osquery's logging macros as mandated by OSQUERY-004/OSQUERY-004-2. Updated the adjacent comments at the three call sites in `sendRequest()` to reflect that the body is now logged via VLOG(1) gated behind `FLAGS_verbose && FLAGS_tls_dump` rather than written raw to stderr. Risk: this does change runtime behavior (output now goes through glog's VLOG sink instead of unconditionally to stderr), which is exactly what the finding requested, but a reviewer should confirm this behavior change (log destination/formatting differences, e.g. VLOG prepending glog's line prefix) is acceptable for this diagnostic dump path.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| @@ -258,7 +258,7 @@ void signalHandler(int num) { | |||
| bool validateAlarmTimeout(const char* flagname, std::uint64_t value) { | |||
There was a problem hiding this comment.
🦩 🔵 validateAlarmTimeout logs via std::cerr instead of LOG(ERROR)
In validateAlarmTimeout (osquery/core/init.cpp, anonymous namespace), replaced the direct std::cerr << "Alarm timeout cannot be lower than 10 seconds" << std::endl; call with LOG(ERROR) << "Alarm timeout cannot be lower than 10 seconds";, matching the suggested fix exactly and keeping the existing osquery::systemLog(...) call intact. No other lines in the file were touched.
🤖 Prompt for AI agents
In osquery/core/init.cpp around line 258, review and complete this code-review fix: validateAlarmTimeout logs via std::cerr instead of LOG(ERROR).
What the draft fix changed: In validateAlarmTimeout (osquery/core/init.cpp, anonymous namespace), replaced the direct `std::cerr << "Alarm timeout cannot be lower than 10 seconds" << std::endl;` call with `LOG(ERROR) << "Alarm timeout cannot be lower than 10 seconds";`, matching the suggested fix exactly and keeping the existing `osquery::systemLog(...)` call intact. No other lines in the file were touched.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
| std::string error_message = | ||
| "Only values higher than 0 are supported for " + std::string(flagname); | ||
| osquery::systemLog(error_message); | ||
| std::cerr << error_message << std::endl; |
There was a problem hiding this comment.
🦩 🔵 std::cerr usage inside library code (not CLI tooling) in aws_util.cpp
Replaced std::cerr << error_message << std::endl; with LOG(ERROR) << error_message; inside the anonymous-namespace function validateIMDSV2RequestAttempts in aws_util.cpp, matching the suggested fix and the logging convention (LOG(ERROR)) used pervasively elsewhere in this file. No other lines were changed.
🤖 Prompt for AI agents
In osquery/utils/aws/aws_util.cpp around line 168, review and complete this code-review fix: std::cerr usage inside library code (not CLI tooling) in aws_util.cpp.
What the draft fix changed: Replaced `std::cerr << error_message << std::endl;` with `LOG(ERROR) << error_message;` inside the anonymous-namespace function `validateIMDSV2RequestAttempts` in aws_util.cpp, matching the suggested fix and the logging convention (`LOG(ERROR)`) used pervasively elsewhere in this file. No other lines were changed.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer
Closes 4 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/process/posix/process.cpp:189osquery/remote/transports/tls.cpp:233osquery/core/init.cpp:258osquery/utils/aws/aws_util.cpp:168What 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:
cb1fa104-eac6-46a7-aba8-90d794c59386Merging 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-17tkuw5wp49 Osquery review findings sweep (8 PRs)