From fc3f5a5c8213746c2459bbfeca3d67d4da342908 Mon Sep 17 00:00:00 2001 From: NopeCharon <115794254+NopeCharon@users.noreply.github.com> Date: Sun, 25 Oct 2026 15:20:58 +0530 Subject: [PATCH] Fix parsing of /proc//stat when the command name contains whitespace The command name (comm) field of /proc//stat is surrounded by parentheses and can contain any character, including runs of spaces, newlines and parentheses. ProcParser::ParseProcPIDStat() and GetPIDStartTimeTicks() handled this by splitting the first line of the file on spaces with absl::SkipWhitespace() and then shifting every field index by the number of spaces in the name. That breaks in several ways: - Consecutive spaces in the name are collapsed by SkipWhitespace() but are each counted in the offset, so later fields are read from the wrong column (or out of bounds of the expected field count). - Only the first line is read, so a newline in the name truncates the record and the field-count check fails. - The field-count check does not account for spaces in the name. GetPIDStartTimeTicks() is used to build UPIDs, so a process with such a name could get a wrong start time or be skipped altogether. Replace the offset arithmetic with a SplitProcPIDStat() helper that reads the whole file, takes the command name as everything between the first '(' and the last ')' (no later field can contain a ')'), and splits only the remainder on whitespace. The returned fields line up with the existing kProcStat*Field constants, so callers index them directly. In the ASAN-only code path of GetPIDStartTimeTicks(), read the file with fread() instead of fgets() so that names containing newlines are handled there too, trim the buffer to the bytes actually read, and close the FILE handle, which was previously leaked. Add parameterized tests covering names with consecutive, leading and trailing spaces, newlines, unbalanced parentheses and an empty name for both ParseProcPIDStat() and GetPIDStartTimeTicks(). Fixes #678 Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com> --- src/common/system/proc_parser.cc | 143 +++++++++++++------------- src/common/system/proc_parser_test.cc | 53 ++++++++++ 2 files changed, 123 insertions(+), 73 deletions(-) diff --git a/src/common/system/proc_parser.cc b/src/common/system/proc_parser.cc index a183295300e..32bc843981b 100644 --- a/src/common/system/proc_parser.cc +++ b/src/common/system/proc_parser.cc @@ -83,6 +83,7 @@ constexpr int kProcNetDevTxDropField = 12; constexpr int kProcStatNumFields = 52; constexpr int kProcStatPIDField = 0; +constexpr int kProcStatCommandField = 1; constexpr int kProcStatMinorFaultsField = 9; constexpr int kProcStatMajorFaultsField = 11; @@ -96,6 +97,45 @@ constexpr int kProcStatStartTimeField = 21; constexpr int kProcStatVSizeField = 22; constexpr int kProcStatRSSField = 23; +namespace { + +/** + * Splits the contents of a /proc//stat file into its fields, so that the kProcStat*Field + * constants above can be used as indices into the result. + * + * The command field is the executable name surrounded by parentheses. The name can contain any + * character, including spaces, newlines and parentheses, so it can't be split on whitespace like + * the other fields. Instead, it extends from the first '(' to the last ')' in the file, since none + * of the subsequent fields can contain a ')'. The command field is returned without parentheses. + */ +StatusOr> SplitProcPIDStat(std::string_view content, + const std::filesystem::path& fpath) { + const size_t open_paren_idx = content.find('('); + const size_t close_paren_idx = content.rfind(')'); + if (open_paren_idx == std::string_view::npos || close_paren_idx == std::string_view::npos || + close_paren_idx < open_paren_idx) { + return error::Internal("Invalid command name in file: $0.", fpath.string()); + } + + std::vector fields; + fields.push_back(absl::StripAsciiWhitespace(content.substr(0, open_paren_idx))); + fields.push_back(content.substr(open_paren_idx + 1, close_paren_idx - open_paren_idx - 1)); + for (std::string_view field : absl::StrSplit(content.substr(close_paren_idx + 1), + absl::ByAnyChar(" \n"), absl::SkipEmpty())) { + fields.push_back(field); + } + + // We check less than in case more fields are added later. + if (fields.size() < kProcStatNumFields) { + return error::Internal("Unexpected number of fields: $0, in file: $1.", fields.size(), + fpath.string()); + } + + return fields; +} + +} // namespace + Status ProcParser::ParseNetworkStatAccumulateIFaceData( const std::vector& dev_stat_record, NetworkStats* out) { DCHECK(out != nullptr); @@ -208,54 +248,29 @@ Status ProcParser::ParseProcPIDStat(int32_t pid, int64_t page_size_bytes, */ DCHECK(out != nullptr); const auto fpath = ProcPidPath(pid, "stat"); - std::ifstream ifs; - ifs.open(fpath); - if (!ifs) { - return error::Internal("Failed to open file: $0.", fpath.string()); - } - - std::string line; - bool ok = true; - if (std::getline(ifs, line)) { - std::vector split = absl::StrSplit(line, " ", absl::SkipWhitespace()); - // We check less than in case more fields are added later. - if (split.size() < kProcStatNumFields) { - return error::Unknown("Incorrect number of fields in stat file: $0.", fpath.string()); - } + PX_ASSIGN_OR_RETURN(std::string content, ReadFileToString(fpath)); + PX_ASSIGN_OR_RETURN(std::vector fields, SplitProcPIDStat(content, fpath)); - // The name is surrounded by (). We remove it first. - size_t open_paren_idx = line.find_first_of('('); - size_t close_paren_idx = line.find_last_of(')'); - if (open_paren_idx == std::string::npos || close_paren_idx == std::string::npos) { - return error::Internal("Invalid command name in file $0.", fpath.string()); - } - out->process_name = line.substr(open_paren_idx + 1, close_paren_idx - open_paren_idx - 1); + out->process_name = std::string(fields[kProcStatCommandField]); - // When split_size > kProcStatNumFields, there are spaces in the command we need to handle. - // command_offset adjusts index for the number of spaces in the command field. - int command_offset = std::count(out->process_name.begin(), out->process_name.end(), ' '); - - ok &= absl::SimpleAtoi(split[kProcStatPIDField], &out->pid); + bool ok = true; + ok &= absl::SimpleAtoi(fields[kProcStatPIDField], &out->pid); - ok &= absl::SimpleAtoi(split[kProcStatMinorFaultsField + command_offset], &out->minor_faults); - ok &= absl::SimpleAtoi(split[kProcStatMajorFaultsField + command_offset], &out->major_faults); + ok &= absl::SimpleAtoi(fields[kProcStatMinorFaultsField], &out->minor_faults); + ok &= absl::SimpleAtoi(fields[kProcStatMajorFaultsField], &out->major_faults); - ok &= absl::SimpleAtoi(split[kProcStatUTimeField + command_offset], &out->utime_ns); - ok &= absl::SimpleAtoi(split[kProcStatKTimeField + command_offset], &out->ktime_ns); - // The kernel tracks utime and ktime in kernel ticks. - out->utime_ns *= kernel_tick_time_ns; - out->ktime_ns *= kernel_tick_time_ns; + ok &= absl::SimpleAtoi(fields[kProcStatUTimeField], &out->utime_ns); + ok &= absl::SimpleAtoi(fields[kProcStatKTimeField], &out->ktime_ns); + // The kernel tracks utime and ktime in kernel ticks. + out->utime_ns *= kernel_tick_time_ns; + out->ktime_ns *= kernel_tick_time_ns; - ok &= absl::SimpleAtoi(split[kProcStatNumThreadsField + command_offset], &out->num_threads); - ok &= absl::SimpleAtoi(split[kProcStatVSizeField + command_offset], &out->vsize_bytes); - ok &= absl::SimpleAtoi(std::string(split[kProcStatRSSField + command_offset]), &out->rss_bytes); + ok &= absl::SimpleAtoi(fields[kProcStatNumThreadsField], &out->num_threads); + ok &= absl::SimpleAtoi(fields[kProcStatVSizeField], &out->vsize_bytes); + ok &= absl::SimpleAtoi(std::string(fields[kProcStatRSSField]), &out->rss_bytes); - // RSS is in pages. - out->rss_bytes *= page_size_bytes; - - } else { - return error::Internal("Failed to read proc stat file: $0.", fpath.string()); - } + // RSS is in pages. + out->rss_bytes *= page_size_bytes; if (!ok) { // This should never happen since it requires the file to be ill-formed @@ -714,7 +729,7 @@ Status ProcParser::ReadNSPid(pid_t pid, std::vector* ns_pids) const StatusOr GetPIDStartTimeTicks(const std::filesystem::path& proc_pid_path) { const std::filesystem::path proc_pid_stat_path = proc_pid_path / "stat"; - std::string line; + std::string content; // It's usually a big no-no to ifdef based on compilation mode, // but this is a workaround for an ASAN bug that causes crashes and flakiness @@ -732,43 +747,25 @@ StatusOr GetPIDStartTimeTicks(const std::filesystem::path& proc_pid_pat // A few quick runs show that /proc//stat is <200 characters on my machine, // but a lot of those numbers are 0. If they were non-zero, the string could be much // longer. Hopefully a 4KB block keeps us safe. This is for ASAN only anyways. + // The whole file is read, rather than a single line, because the command name may + // contain newlines. constexpr int kMaxSupportedProcPIDStatLength = 4096; - line.resize(kMaxSupportedProcPIDStatLength); - if (std::fgets(line.data(), line.size(), fp) == nullptr) { - return error::Internal("Could not get line from file: $0.", proc_pid_stat_path.string()); + content.resize(kMaxSupportedProcPIDStatLength); + const size_t bytes_read = std::fread(content.data(), 1, content.size(), fp); + std::fclose(fp); + if (bytes_read == 0) { + return error::Internal("Could not read file: $0.", proc_pid_stat_path.string()); } + content.resize(bytes_read); #else - std::ifstream ifs; - ifs.open(proc_pid_stat_path); - if (!ifs) { - return error::Internal("Could not open file: $0.", proc_pid_stat_path.string()); - } - - if (!std::getline(ifs, line)) { - return error::Internal("Could not get line from file: $0.", proc_pid_stat_path.string()); - } + PX_ASSIGN_OR_RETURN(content, ReadFileToString(proc_pid_stat_path)); #endif - size_t open_paren_idx = line.find_first_of('('); - size_t close_paren_idx = line.find_last_of(')'); - if (open_paren_idx == std::string::npos || close_paren_idx == std::string::npos) { - return error::Internal("Invalid command name in file: $0.", proc_pid_stat_path.string()); - } - - // When split_size > kProcStatNumFields, there are spaces in the command we need to handle. - // command_offset adjusts index for the number of spaces in the command field. - int command_offset = - std::count(line.begin() + open_paren_idx, line.begin() + close_paren_idx, ' '); - - std::vector split = absl::StrSplit(line, " ", absl::SkipWhitespace()); - // We check less than in case more fields are added later. - if (split.size() < kProcStatNumFields) { - return error::Internal("Unexpected number of columns: $0, in file: $1.", split.size(), - proc_pid_stat_path.string()); - } + PX_ASSIGN_OR_RETURN(std::vector fields, + SplitProcPIDStat(content, proc_pid_stat_path)); int64_t start_time_ticks; - if (!absl::SimpleAtoi(split[kProcStatStartTimeField + command_offset], &start_time_ticks)) { + if (!absl::SimpleAtoi(fields[kProcStatStartTimeField], &start_time_ticks)) { return error::Internal("Time value does not parse in file: $0.", proc_pid_stat_path.string()); } diff --git a/src/common/system/proc_parser_test.cc b/src/common/system/proc_parser_test.cc index 7d2baaa5315..e3b2838796a 100644 --- a/src/common/system/proc_parser_test.cc +++ b/src/common/system/proc_parser_test.cc @@ -26,6 +26,8 @@ #include #include +#include + #include "src/common/fs/fs_wrapper.h" #include "src/common/testing/test_environment.h" #include "src/common/testing/testing.h" @@ -277,6 +279,57 @@ TEST_F(ProcParserTest, read_pid_start_time) { ASSERT_OK_AND_EQ(parser_->GetPIDStartTimeTicks(123), 14329); } +// The command name in /proc//stat may contain any character, including runs of spaces, +// newlines and parentheses. None of these should affect the parsing of the subsequent fields. +class ProcParserCommandNameTest : public ProcParserTest, + public ::testing::WithParamInterface { + protected: + void SetUp() override { + ProcParserTest::SetUp(); + const std::filesystem::path pid_path = temp_dir_.path() / "4602"; + ASSERT_TRUE(std::filesystem::create_directory(pid_path)); + ASSERT_OK(WriteFileFromString( + pid_path / "stat", + absl::StrCat("4602 (", GetParam(), + ") S 3260 4602 3260 34818 4602 1077936128 1799 174589 55 68 8 23 106 72 20 0 " + "13 0 14329 114384896 2577 18446744073709551615 4194304 7917379 " + "140730842479232 0 0 0 1006254592 0 2143420159 0 0 0 17 3 0 0 3 0 0 " + "12193792 12432192 34951168 140730842488151 140730842488200 " + "140730842488200 140730842492896 0\n"))); + } + + testing::TempDir temp_dir_; +}; + +TEST_P(ProcParserCommandNameTest, ParsePidStat) { + PX_SET_FOR_SCOPE(FLAGS_proc_path, temp_dir_.path().string()); + ProcParser::ProcessStats stats; + ASSERT_OK(parser_->ParseProcPIDStat(4602, bytes_per_page_, kernel_tick_time_ns_, &stats)); + + EXPECT_EQ(GetParam(), stats.process_name); + EXPECT_EQ(4602, stats.pid); + + EXPECT_EQ(800, stats.utime_ns); + EXPECT_EQ(2300, stats.ktime_ns); + EXPECT_EQ(13, stats.num_threads); + + EXPECT_EQ(55, stats.major_faults); + EXPECT_EQ(1799, stats.minor_faults); + + EXPECT_EQ(114384896, stats.vsize_bytes); + EXPECT_EQ(2577 * bytes_per_page_, stats.rss_bytes); +} + +TEST_P(ProcParserCommandNameTest, ReadPIDStartTime) { + PX_SET_FOR_SCOPE(FLAGS_proc_path, temp_dir_.path().string()); + ASSERT_OK_AND_EQ(parser_->GetPIDStartTimeTicks(4602), 14329); +} + +INSTANTIATE_TEST_SUITE_P(CommandNames, ProcParserCommandNameTest, + ::testing::Values("npm", "", "npm start", "npm start", " npm ", + "npm\nstart", "npm\n", "npm (start)", "npm) (start", + ") 1 2 3 (")); + TEST_F(ProcParserTest, read_pid_cmdline) { PX_SET_FOR_SCOPE(FLAGS_proc_path, GetPathToTestDataFile("testdata/proc")); EXPECT_THAT("/usr/lib/slack/slack --force-device-scale-factor=1.5 --high-dpi-support=1",