Repository navigation
fix: avoid 15-byte command_name truncation in read_command_name() - #3607
Asthenia0412 wants to merge 1 commit into
Conversation
read_command_name() in bvar/variable.cpp was reading the command name from /proc/self/stat's comm field, which is limited by the kernel's TASK_COMM_LEN (16 bytes including null terminator), causing silent truncation for any process name longer than 15 characters. This fix switches to reading from /proc/self/cmdline via the existing butil::ReadCommandLine() helper, which returns the full argv[0] and is already used by other parts of brpc (GetProgramName, pprof_service). Fixes apache#1251
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
What changed in this PR
Updates bvar::read_command_name() to avoid the Linux kernel TASK_COMM_LEN (15-char) truncation by switching from /proc/self/stat to /proc/self/cmdline, enabling longer command names to be captured for debugging/visibility.
Changes:
- Replace
/proc/self/statparsing withbutil::ReadCommandLine()(reads/proc/self/cmdline). - Add the required
butil/process_util.hinclude and remove<fstream>usage. - Simplify normalization by directly underscoring the read command-line content.
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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)); |
| char cmdline[1024]; | ||
| const ssize_t nr = butil::ReadCommandLine(cmdline, sizeof(cmdline), false); |
| } else { | ||
| to_underscored_name(&s, command_name); | ||
| } | ||
| to_underscored_name(&s, butil::StringPiece(cmdline, nr)); |
wwbmmm
left a comment
wwbmmm
left a comment
There was a problem hiding this comment.
[replied by brpc-oncall robot] Switching from /proc/self/stat to /proc/self/cmdline is the right direction to avoid the 15-char TASK_COMM_LEN truncation, but the name derived from argv[0] changes semantics and needs normalization. Note: two of the Copilot findings are false positives — (1) with_args=false already stops at the first '\0' inside butil::ReadCommandLine, so no NUL separators or arguments can leak into StringPiece; (2) the 1024-byte buffer cannot silently truncate, since ReadCommandLine returns -1 (and logs an error) when the buffer is not big enough, which this code maps to an empty name.
There was a problem hiding this comment.
[replied by brpc-oncall robot] Beyond the path leak Copilot mentioned, this is a user-visible naming/compatibility change. comm used to be a short executable name, while argv[0] is whatever was used to launch the process; deployments that start with an absolute or relative path will now produce names like _usr_local_bin_myservice (to_underscored_name turns the leading '/' into '' and '/' into ''). That propagates to bvar_dump_file/bvar_dump_prefix/mbvar_* (monitor/bvar.<app>.data, <app> prefix) and to the default rpc_dump_dir (./rpc_data/rpc_dump/<app>), so existing monitoring/dump paths and downstream consumers (e.g. noah) change after upgrade, and the leading '_' is an additional oddity. Suggest normalizing to the basename of argv[0] (strip everything up to the last '/') before calling to_underscored_name, which still fixes the 15-char truncation while keeping the previous naming behavior.
Background
brpc builtin command system silently truncates command names longer than 15 characters, because
bvar::read_command_name()reads from/proc/self/statwhosecommfield is kernel-limited toTASK_COMM_LEN= 16 bytes (15 chars + null).Problem
Hardcoded kernel-level 15-char limit on process name makes RPC debugging difficult when users define descriptive command names (e.g. long service names).
Root Cause
The old implementation parsed
/proc/self/stat:Linux kernel's
TASK_COMM_LEN = 16means thecommfield is always truncated to 15 characters.Solution
Switch to reading from
/proc/self/cmdlinevia the existingbutil::ReadCommandLine()helper, which returns the fullargv[0]. This helper is already used elsewhere in brpc (GetProgramName,pprof_service), so no new dependencies are introduced.Verification
g++ -std=c++17 -fsyntax-onlypasses with no errors./proc/self/statcomm truncatesmyhelloworldprocess_test_longtomyhelloworldpro(15 chars)/proc/self/cmdlinereturns fullargv[0]without truncationFixes #1251