From 51779ce639564cf1cdd68d12565534bfbc1553a5 Mon Sep 17 00:00:00 2001 From: Joshua Peraza Date: Tue, 2 May 2017 10:07:21 -0700 Subject: [PATCH] linux: Make MemoryMap retry when duplicates are detected When the /proc/pid/maps file is not read atomically and the target process is actively mapping memory, entries can be read multiple times or missed entirely. This change makes MemoryMap read the whole contents of the maps file before attempting to parse it as well as check for duplication/overlap errors, retrying on failure. This change also adds ptrace attachements to unit tests to reflect actual intended usage. Bug: crashpad:30 Change-Id: Ie8549548e25c47baa418ee7439d82743f84ff41e Reviewed-on: https://chromium-review.googlesource.com/491950 Reviewed-by: Joshua Peraza Reviewed-by: Mark Mentovai Commit-Queue: Joshua Peraza --- util/linux/memory_map.cc | 267 ++++++++++++++++++++-------------- util/linux/memory_map_test.cc | 7 + 2 files changed, 166 insertions(+), 108 deletions(-) diff --git a/util/linux/memory_map.cc b/util/linux/memory_map.cc index cbf0c4e4..d1ef6c09 100644 --- a/util/linux/memory_map.cc +++ b/util/linux/memory_map.cc @@ -22,7 +22,8 @@ #include "base/logging.h" #include "build/build_config.h" #include "util/file/delimited_file_reader.h" -#include "util/file/file_reader.h" +#include "util/file/file_io.h" +#include "util/file/string_file.h" #include "util/stdlib/string_number_conversion.h" namespace crashpad { @@ -56,6 +57,134 @@ bool HexStringToNumber(const std::string& string, Type* number) { return LocalStringToNumber("0x" + string, number); } +// The result from parsing a line from the maps file. +enum class ParseResult { + // A line was successfully parsed. + kSuccess = 0, + + // The end of the file was successfully reached. + kEndOfFile, + + // There was an error in the file, likely because it was read non-atmoically. + // We should try to read it again. + kRetry, + + // An error with a message logged. + kError +}; + +// Reads a line from a maps file being read by maps_file_reader and extends +// mappings with a new MemoryMap::Mapping describing the line. +ParseResult ParseMapsLine(DelimitedFileReader* maps_file_reader, + std::vector* mappings) { + std::string field; + LinuxVMAddress start_address; + switch (maps_file_reader->GetDelim('-', &field)) { + case DelimitedFileReader::Result::kError: + return ParseResult::kError; + case DelimitedFileReader::Result::kEndOfFile: + return ParseResult::kEndOfFile; + case DelimitedFileReader::Result::kSuccess: + field.pop_back(); + if (!HexStringToNumber(field, &start_address)) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + if (!mappings->empty() && start_address < mappings->back().range.End()) { + return ParseResult::kRetry; + } + } + + LinuxVMAddress end_address; + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !HexStringToNumber(field, &end_address))) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + if (end_address <= start_address) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + + // TODO(jperaza): set bitness properly +#if defined(ARCH_CPU_64_BITS) + const bool is_64_bit = true; +#else + const bool is_64_bit = false; +#endif + + MemoryMap::Mapping mapping; + mapping.range.SetRange(is_64_bit, start_address, end_address - start_address); + + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), field.size() != 4)) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } +#define SET_FIELD(actual_c, outval, true_chars, false_chars) \ + do { \ + if (strchr(true_chars, actual_c)) { \ + *outval = true; \ + } else if (strchr(false_chars, actual_c)) { \ + *outval = false; \ + } else { \ + LOG(ERROR) << "format error"; \ + return ParseResult::kError; \ + } \ + } while (false) + SET_FIELD(field[0], &mapping.readable, "r", "-"); + SET_FIELD(field[1], &mapping.writable, "w", "-"); + SET_FIELD(field[2], &mapping.executable, "x", "-"); + SET_FIELD(field[3], &mapping.shareable, "sS", "p"); +#undef SET_FIELD + + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !HexStringToNumber(field, &mapping.offset))) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + + int major, minor; + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), field.size() != 5) || + !HexStringToNumber(field.substr(0, 2), &major) || + !HexStringToNumber(field.substr(3, 2), &minor)) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + mapping.device = MKDEV(major, minor); + + if (maps_file_reader->GetDelim(' ', &field) != + DelimitedFileReader::Result::kSuccess || + (field.pop_back(), !LocalStringToNumber(field, &mapping.inode))) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + + if (maps_file_reader->GetDelim('\n', &field) != + DelimitedFileReader::Result::kSuccess) { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + if (field.back() != '\n') { + LOG(ERROR) << "format error"; + return ParseResult::kError; + } + field.pop_back(); + + mappings->push_back(mapping); + + size_t path_start = field.find_first_not_of(' '); + if (path_start != std::string::npos) { + mappings->back().name = field.substr(path_start); + } + return ParseResult::kSuccess; +} + } // namespace MemoryMap::Mapping::Mapping() @@ -76,121 +205,43 @@ MemoryMap::~MemoryMap() {} bool MemoryMap::Initialize(pid_t pid) { INITIALIZATION_STATE_SET_INITIALIZING(initialized_); - char path[32]; - snprintf(path, sizeof(path), "/proc/%d/maps", pid); - FileReader maps_file; - if (!maps_file.Open(base::FilePath(path))) { - return false; - } - DelimitedFileReader maps_file_reader(&maps_file); - - DelimitedFileReader::Result result; - std::string field; - while ((result = maps_file_reader.GetDelim('-', &field)) == - DelimitedFileReader::Result::kSuccess) { - field.pop_back(); - - LinuxVMAddress start_address; - if (!HexStringToNumber(field, &start_address)) { - LOG(ERROR) << "format error"; + // If the maps file is not read atomically, entries can be read multiple times + // or missed entirely. The kernel reads entries from this file into a page + // sized buffer, so maps files larger than a page require multiple reads. + // Attempt to reduce the time between reads by reading the entire file into a + // StringFile before attempting to parse it. If ParseMapsLine detects + // duplicate, overlapping, or out-of-order entries, it will trigger restarting + // the read up to |attempts| times. + int attempts = 3; + do { + std::string contents; + char path[32]; + snprintf(path, sizeof(path), "/proc/%d/maps", pid); + if (!LoggingReadEntireFile(base::FilePath(path), &contents)) { return false; } - LinuxVMAddress end_address; - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), !HexStringToNumber(field, &end_address))) { - LOG(ERROR) << "format error"; - return false; + StringFile maps_file; + maps_file.SetString(contents); + DelimitedFileReader maps_file_reader(&maps_file); + + ParseResult result; + while ((result = ParseMapsLine(&maps_file_reader, &mappings_)) == + ParseResult::kSuccess) { } - if (end_address <= start_address) { - LOG(ERROR) << "format error"; + if (result == ParseResult::kEndOfFile) { + INITIALIZATION_STATE_SET_VALID(initialized_); + return true; + } + if (result == ParseResult::kError) { return false; } - // TODO(jperaza): set bitness properly -#if defined(ARCH_CPU_64_BITS) - const bool is_64_bit = true; -#else - const bool is_64_bit = false; -#endif + DCHECK(result == ParseResult::kRetry); + } while (--attempts > 0); - Mapping mapping; - mapping.range.SetRange( - is_64_bit, start_address, end_address - start_address); - - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), field.size() != 4)) { - LOG(ERROR) << "format error"; - return false; - } -#define SET_FIELD(actual_c, outval, true_chars, false_chars) \ - do { \ - if (strchr(true_chars, actual_c)) { \ - *outval = true; \ - } else if (strchr(false_chars, actual_c)) { \ - *outval = false; \ - } else { \ - LOG(ERROR) << "format error"; \ - return false; \ - } \ - } while (false) - SET_FIELD(field[0], &mapping.readable, "r", "-"); - SET_FIELD(field[1], &mapping.writable, "w", "-"); - SET_FIELD(field[2], &mapping.executable, "x", "-"); - SET_FIELD(field[3], &mapping.shareable, "sS", "p"); -#undef SET_FIELD - - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), !HexStringToNumber(field, &mapping.offset))) { - LOG(ERROR) << "format error"; - return false; - } - - int major, minor; - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), field.size() != 5) || - !HexStringToNumber(field.substr(0, 2), &major) || - !HexStringToNumber(field.substr(3, 2), &minor)) { - LOG(ERROR) << "format error"; - return false; - } - mapping.device = MKDEV(major, minor); - - if (maps_file_reader.GetDelim(' ', &field) != - DelimitedFileReader::Result::kSuccess || - (field.pop_back(), !LocalStringToNumber(field, &mapping.inode))) { - LOG(ERROR) << "format error"; - return false; - } - - if (maps_file_reader.GetDelim('\n', &field) != - DelimitedFileReader::Result::kSuccess) { - LOG(ERROR) << "format error"; - return false; - } - if (field.back() != '\n') { - LOG(ERROR) << "format error"; - return false; - } - field.pop_back(); - - mappings_.push_back(mapping); - - size_t path_start = field.find_first_not_of(' '); - if (path_start != std::string::npos) { - mappings_.back().name = field.substr(path_start); - } - } - if (result != DelimitedFileReader::Result::kEndOfFile) { - return false; - } - - INITIALIZATION_STATE_SET_VALID(initialized_); - return true; + LOG(ERROR) << "retry count exceeded"; + return false; } const MemoryMap::Mapping* MemoryMap::FindMapping(LinuxVMAddress address) const { diff --git a/util/linux/memory_map_test.cc b/util/linux/memory_map_test.cc index 9258f83e..23894b9b 100644 --- a/util/linux/memory_map_test.cc +++ b/util/linux/memory_map_test.cc @@ -28,6 +28,7 @@ #include "test/multiprocess.h" #include "test/scoped_temp_dir.h" #include "util/file/file_io.h" +#include "util/linux/scoped_ptrace_attach.h" #include "util/misc/clock.h" #include "util/misc/from_pointer_cast.h" #include "util/posix/scoped_mmap.h" @@ -104,6 +105,9 @@ class MapChildTest : public Multiprocess { std::string mapped_file_name(path_length, std::string::value_type()); CheckedReadFileExactly(ReadPipeHandle(), &mapped_file_name[0], path_length); + ScopedPtraceAttach attachment; + attachment.ResetAttach(ChildPID()); + MemoryMap map; ASSERT_TRUE(map.Initialize(ChildPID())); @@ -279,6 +283,9 @@ class MapRunningChildTest : public Multiprocess { // Let the child get back to its work SleepNanoseconds(1000); + ScopedPtraceAttach attachment; + attachment.ResetAttach(ChildPID()); + MemoryMap map; ASSERT_TRUE(map.Initialize(ChildPID()));