Copilot commented on code in PR #3607:
URL: https://github.com/apache/brpc/pull/3607#discussion_r4271848428
##########
src/bvar/variable.cpp:
##########
@@ -689,28 +689,16 @@ int Variable::dump_exposed(Dumper* dumper, const
DumpOptions* poptions) {
// ============= export to files ==============
std::string read_command_name() {
- std::ifstream fin("/proc/self/stat");
- if (!fin.is_open()) {
+ // Use /proc/self/cmdline instead of /proc/self/stat to avoid the 15-byte
+ // TASK_COMM_LEN kernel limit on the comm field. /proc/self/cmdline
+ // contains the full argv[0] which preserves the complete command name.
+ char cmdline[1024];
+ const ssize_t nr = butil::ReadCommandLine(cmdline, sizeof(cmdline), false);
+ if (nr <= 0) {
return std::string();
}
- int pid = 0;
- std::string command_name;
- fin >> pid >> command_name;
- if (!fin.good()) {
- return std::string();
- }
- // Although the man page says the command name is in parenthesis, for
- // safety we normalize the name.
std::string s;
- if (command_name.size() >= 2UL && command_name[0] == '(' &&
- butil::back_char(command_name) == ')') {
- // remove parenthesis.
- to_underscored_name(&s,
- butil::StringPiece(command_name.data() + 1,
- command_name.size() - 2UL));
- } else {
- to_underscored_name(&s, command_name);
- }
+ to_underscored_name(&s, butil::StringPiece(cmdline, nr));
Review Comment:
`/proc/self/cmdline` contains NUL-separated strings (`argv[0]`, `argv[1]`,
...). Passing `StringPiece(cmdline, nr)` will include embedded `\\0` separators
(and potentially arguments), causing `to_underscored_name()` to incorporate
them as underscores and effectively mix args into the command name. Consider
extracting only `argv[0]` (up to the first `\\0`) before calling
`to_underscored_name()`.
##########
src/bvar/variable.cpp:
##########
@@ -689,28 +689,16 @@ int Variable::dump_exposed(Dumper* dumper, const
DumpOptions* poptions) {
// ============= export to files ==============
std::string read_command_name() {
- std::ifstream fin("/proc/self/stat");
- if (!fin.is_open()) {
+ // Use /proc/self/cmdline instead of /proc/self/stat to avoid the 15-byte
+ // TASK_COMM_LEN kernel limit on the comm field. /proc/self/cmdline
+ // contains the full argv[0] which preserves the complete command name.
+ char cmdline[1024];
+ const ssize_t nr = butil::ReadCommandLine(cmdline, sizeof(cmdline), false);
Review Comment:
The fixed 1024-byte buffer can still truncate long `argv[0]` paths (or
command lines depending on `ReadCommandLine` behavior), reintroducing silent
truncation—just at a different limit. Consider using an approach that detects
truncation and retries with a larger buffer (or uses a helper that returns a
`std::string`/dynamically-sized result if available) so the function reliably
preserves long names.
##########
src/bvar/variable.cpp:
##########
@@ -689,28 +689,16 @@ int Variable::dump_exposed(Dumper* dumper, const
DumpOptions* poptions) {
// ============= export to files ==============
std::string read_command_name() {
- std::ifstream fin("/proc/self/stat");
- if (!fin.is_open()) {
+ // Use /proc/self/cmdline instead of /proc/self/stat to avoid the 15-byte
+ // TASK_COMM_LEN kernel limit on the comm field. /proc/self/cmdline
+ // contains the full argv[0] which preserves the complete command name.
+ char cmdline[1024];
+ const ssize_t nr = butil::ReadCommandLine(cmdline, sizeof(cmdline), false);
+ if (nr <= 0) {
return std::string();
}
- int pid = 0;
- std::string command_name;
- fin >> pid >> command_name;
- if (!fin.good()) {
- return std::string();
- }
- // Although the man page says the command name is in parenthesis, for
- // safety we normalize the name.
std::string s;
- if (command_name.size() >= 2UL && command_name[0] == '(' &&
- butil::back_char(command_name) == ')') {
- // remove parenthesis.
- to_underscored_name(&s,
- butil::StringPiece(command_name.data() + 1,
- command_name.size() - 2UL));
- } else {
- to_underscored_name(&s, command_name);
- }
+ to_underscored_name(&s, butil::StringPiece(cmdline, nr));
Review Comment:
Using `argv[0]` from `/proc/self/cmdline` may include the full executable
path (e.g., `/opt/company/bin/myservice`), which can leak filesystem layout via
exported bvars/logs compared to the previous `/proc/self/stat` comm behavior
(typically a short name). If the intent is a stable 'program name', consider
normalizing to the basename of `argv[0]` before underscoring.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
