llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: satyanarayana reddy janga (satyajanga) <details> <summary>Changes</summary> **Issue** An internal failing test found a latent bug in lldb's save-core (minidump writer). When it saved a memory range that had an unreadable page in it, it: - stopped at that page and threw away the readable memory after it, and - still recorded the full size for that range, which shifted and corrupted every following range in the dump. Result: coredumps were missing/garbled memory — e.g. folly's coroutine cookie read back as 0, so co_print_suspended found "no suspended frames." **Fix** Rewrote ReadWriteMemoryInChunks in MinidumpFileBuilder.cpp to: save the readable bytes, zero-fill the unreadable page, keep going, and record the exact number of bytes actually written. That keeps the dump aligned and preserves memory after a hole. One file, plus a #include <cstring>. **Test** Added an lldb API test that builds a memory region with a readable page followed by an unreadable tail, saves a core of it, and checks the readable data comes back intact and the dump isn't corrupted. Verified it fails without the fix and passes with it. --- Full diff: https://github.com/llvm/llvm-project/pull/212641.diff 4 Files Affected: - (modified) lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp (+45-60) - (added) lldb/test/API/functionalities/process_save_core_minidump_partial_read/Makefile (+3) - (added) lldb/test/API/functionalities/process_save_core_minidump_partial_read/TestProcessSaveCoreMinidumpPartialRead.py (+106) - (added) lldb/test/API/functionalities/process_save_core_minidump_partial_read/main.cpp (+33) ``````````diff diff --git a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp index 876eea28a8923..ef1a3713175fc 100644 --- a/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp +++ b/lldb/source/Plugins/ObjectFile/Minidump/MinidumpFileBuilder.cpp @@ -8,6 +8,8 @@ #include "MinidumpFileBuilder.h" +#include <cstring> + #include "Plugins/Process/minidump/RegisterContextMinidump_ARM64.h" #include "Plugins/Process/minidump/RegisterContextMinidump_x86_64.h" @@ -968,72 +970,55 @@ Status MinidumpFileBuilder::ReadWriteMemoryInChunks( const lldb::addr_t addr = range.range.start(); const lldb::addr_t size = range.range.size(); Log *log = GetLog(LLDBLog::Object); - uint64_t total_bytes_read = 0; + void *buf = data_buffer.GetBytes(); + const lldb::addr_t chunk_size = data_buffer.GetByteSize(); + // The smallest page size on any supported target. Used to step over an + // unreadable page without zero-filling memory that may be readable. + const lldb::addr_t page_size = 4096; + + // We must write exactly `size` bytes for this range. The Memory64List stores + // every range's contents in one contiguous blob indexed by the cumulative + // DataSize of the preceding ranges, so writing fewer bytes than DataSize + // corrupts every following range. On a failed read we zero-fill the + // unreadable page and keep going rather than truncating the range. + bytes_read = 0; Status addDataError; - Process::ReadMemoryChunkCallback callback = - [&](Status &error, lldb::addr_t current_addr, const void *buf, - uint64_t bytes_read) -> lldb_private::IterationAction { - if (error.Fail() || bytes_read == 0) { - LLDB_LOGF(log, - "Failed to read memory region at: 0x%" PRIx64 - ". Bytes read: 0x%" PRIx64 ", error: %s", - current_addr, bytes_read, error.AsCString()); - - // If we failed in a memory read, we would normally want to skip - // this entire region. If we had already written to the minidump - // file, we can't easily rewind that state. - // - // So if we do encounter an error while reading, we return - // immediately, any prior bytes read will still be included but - // any bytes partially read before the error are ignored. - return lldb_private::IterationAction::Stop; - } - - if (current_addr != addr + total_bytes_read) { - LLDB_LOGF(log, - "Current addr is at unexpected address, 0x%" PRIx64 - ", expected at 0x%" PRIx64, - current_addr, addr + total_bytes_read); - - // Something went wrong and the address is not where it should be - // we'll error out of this Minidump generation. - addDataError = Status::FromErrorStringWithFormat( - "Unexpected address encounterd when reading memory in chunks " - "0x%" PRIx64 " expected 0x%" PRIx64, - current_addr, addr + total_bytes_read); - return lldb_private::IterationAction::Stop; + while (bytes_read < size) { + const lldb::addr_t current_addr = addr + bytes_read; + const lldb::addr_t bytes_remaining = size - bytes_read; + const lldb::addr_t bytes_to_read = std::min(bytes_remaining, chunk_size); + Status error; + const lldb::addr_t bytes_read_for_chunk = + m_process_sp->ReadMemoryFromInferior(current_addr, buf, bytes_to_read, + error); + + if (bytes_read_for_chunk > 0) { + addDataError = AddData(buf, bytes_read_for_chunk); + if (addDataError.Fail()) + return addDataError; + bytes_read += bytes_read_for_chunk; } - // Write to the minidump file with the chunk potentially flushing to - // disk. - // This error will be captured by the outer scope and is considered fatal. - // If we get an error writing to disk we can't easily guarauntee that we - // won't corrupt the minidump. - addDataError = AddData(buf, bytes_read); - if (addDataError.Fail()) - return lldb_private::IterationAction::Stop; - - total_bytes_read += bytes_read; - // If we have a partial read, report it, but only if the partial read - // didn't finish reading the entire region. - if (bytes_read != data_buffer.GetByteSize() && total_bytes_read != size) { + // The read stopped short at an unreadable page. Zero-fill up to the next + // page boundary and continue past it, so readable memory that follows is + // still captured. + if (bytes_read_for_chunk < bytes_to_read) { + const lldb::addr_t hole = addr + bytes_read; + lldb::addr_t fill = ((hole + page_size) & ~(page_size - 1)) - hole; + fill = std::min(fill, size - bytes_read); + fill = std::min(fill, chunk_size); LLDB_LOGF(log, - "Memory region at: 0x%" PRIx64 " partial read 0x%" PRIx64 - " bytes out of 0x%" PRIx64 " bytes.", - current_addr, bytes_read, - data_buffer.GetByteSize() - bytes_read); - - // If we've read some bytes, we stop trying to read more and return - // this best effort attempt - return lldb_private::IterationAction::Stop; + "Failed to read memory region at: 0x%" PRIx64 + ". Zero-filling 0x%" PRIx64 " bytes, error: %s", + hole, fill, error.AsCString()); + ::memset(buf, 0, fill); + addDataError = AddData(buf, fill); + if (addDataError.Fail()) + return addDataError; + bytes_read += fill; } + } - // No problems, keep going! - return lldb_private::IterationAction::Continue; - }; - - bytes_read = m_process_sp->ReadMemoryInChunks( - addr, data_buffer.GetBytes(), data_buffer.GetByteSize(), size, callback); return addDataError; } diff --git a/lldb/test/API/functionalities/process_save_core_minidump_partial_read/Makefile b/lldb/test/API/functionalities/process_save_core_minidump_partial_read/Makefile new file mode 100644 index 0000000000000..99998b20bcb05 --- /dev/null +++ b/lldb/test/API/functionalities/process_save_core_minidump_partial_read/Makefile @@ -0,0 +1,3 @@ +CXX_SOURCES := main.cpp + +include Makefile.rules diff --git a/lldb/test/API/functionalities/process_save_core_minidump_partial_read/TestProcessSaveCoreMinidumpPartialRead.py b/lldb/test/API/functionalities/process_save_core_minidump_partial_read/TestProcessSaveCoreMinidumpPartialRead.py new file mode 100644 index 0000000000000..6ab140e5e4e8c --- /dev/null +++ b/lldb/test/API/functionalities/process_save_core_minidump_partial_read/TestProcessSaveCoreMinidumpPartialRead.py @@ -0,0 +1,106 @@ +""" +Regression test for saving a minidump when a saved memory range contains an +unreadable page. + +The minidump Memory64List stores every range's bytes in one contiguous blob +indexed by the cumulative DataSize of the preceding ranges. If the writer counts +bytes it failed to read (and therefore never wrote) into a range's DataSize, the +blob desyncs and later ranges -- and the data read back from them -- are +corrupted. See MinidumpFileBuilder::ReadWriteMemoryInChunks. +""" + +import os +import struct + +import lldb +from lldbsuite.test.decorators import * +from lldbsuite.test.lldbtest import * +from lldbsuite.test import lldbutil + + +class ProcessSaveCoreMinidumpPartialReadTestCase(TestBase): + def _assert_memory64_within_file(self, path): + """The Memory64List data must not claim more bytes than the file holds.""" + with open(path, "rb") as f: + data = f.read() + self.assertEqual(data[:4], b"MDMP") + num_streams, dir_rva = struct.unpack_from("<II", data, 8) + found = False + for i in range(num_streams): + stream_type, _, rva = struct.unpack_from("<III", data, dir_rva + i * 12) + if stream_type == 9: # Memory64ListStream + found = True + num_ranges, base_rva = struct.unpack_from("<QQ", data, rva) + total = sum( + struct.unpack_from("<QQ", data, rva + 16 + j * 16)[1] + for j in range(num_ranges) + ) + self.assertLessEqual( + base_rva + total, + len(data), + "Memory64List DataSize overflows the minidump file", + ) + self.assertTrue(found, "minidump has no Memory64List stream") + + @skipUnlessArch("x86_64") + @skipUnlessPlatform(["linux"]) + def test_save_core_range_with_unreadable_tail(self): + """A saved range whose tail is unreadable must keep its readable bytes + intact and must not corrupt the minidump's memory blob.""" + self.build() + exe = self.getBuildArtifact("a.out") + target = self.dbg.CreateTarget(exe) + lldbutil.run_break_set_by_source_regexp(self, "Set a breakpoint here") + process = target.LaunchSimple(None, None, self.get_process_working_directory()) + self.assertState(process.GetState(), lldb.eStateStopped) + + frame = process.GetSelectedThread().GetFrameAtIndex(0) + region_addr = frame.FindVariable("region").GetValueAsUnsigned() + page = frame.FindVariable("page").GetValueAsUnsigned() + self.assertNotEqual(region_addr, 0) + self.assertNotEqual(page, 0) + + # Sanity: the live process reads the backed first page (the sentinel), + # while reading into the unbacked tail fails. + live_error = lldb.SBError() + live_page = process.ReadMemory(region_addr, page, live_error) + self.assertSuccess(live_error) + self.assertEqual(live_page, b"\xab" * page) + tail_error = lldb.SBError() + process.ReadMemory(region_addr + page, page, tail_error) + self.assertTrue(tail_error.Fail()) + + core_path = self.getBuildArtifact("partial_read.dmp") + options = lldb.SBSaveCoreOptions() + options.SetOutputFile(lldb.SBFileSpec(core_path)) + options.SetPluginName("minidump") + options.SetStyle(lldb.eSaveCoreCustomOnly) + # Force the whole four-page mapping (readable first page + unreadable + # tail) to be saved as a single range. + rw = 0b110 # ePermissionsReadable | ePermissionsWritable + options.AddMemoryRegionToSave( + lldb.SBMemoryRegionInfo("", region_addr, region_addr + 4 * page, rw, True) + ) + self.assertSuccess(process.SaveCore(options)) + + try: + core_target = self.dbg.CreateTarget(None) + core_process = core_target.LoadCore(core_path) + self.assertTrue(core_process.IsValid()) + + # The readable page that precedes the unreadable tail must survive + # byte-for-byte. Before the fix this read back as garbage/zero + # because the range's data was dropped and the blob desynced. + core_error = lldb.SBError() + core_page = core_process.ReadMemory(region_addr, page, core_error) + self.assertSuccess(core_error) + self.assertEqual( + core_page, + b"\xab" * page, + "readable page before the unreadable tail was lost or misaligned", + ) + + self._assert_memory64_within_file(core_path) + finally: + if os.path.isfile(core_path): + os.unlink(core_path) diff --git a/lldb/test/API/functionalities/process_save_core_minidump_partial_read/main.cpp b/lldb/test/API/functionalities/process_save_core_minidump_partial_read/main.cpp new file mode 100644 index 0000000000000..8e229190a1678 --- /dev/null +++ b/lldb/test/API/functionalities/process_save_core_minidump_partial_read/main.cpp @@ -0,0 +1,33 @@ +#include <cstdint> +#include <cstdio> +#include <cstdlib> +#include <cstring> +#include <fcntl.h> +#include <sys/mman.h> +#include <unistd.h> + +// Create a single read/write memory region whose tail is unreadable: a +// file-backed mapping that is larger than the file. The first page is backed by +// the file (readable) and holds a sentinel; the pages that follow are past EOF +// and fail to read via process_vm_readv. This is the shape that used to make +// the minidump save-core writer truncate a range and desync the Memory64 data +// blob (see MinidumpFileBuilder::ReadWriteMemoryInChunks). +int main() { + const size_t page = sysconf(_SC_PAGESIZE); + char path[] = "/tmp/lldb_savecore_holeXXXXXX"; + int fd = mkstemp(path); + if (fd < 0) + return 1; + unlink(path); + if (ftruncate(fd, page) != 0) // the file is exactly one page + return 1; + const size_t map_size = 4 * page; // map four pages; pages 1..3 are past EOF + uint8_t *region = static_cast<uint8_t *>( + mmap(nullptr, map_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0)); + if (region == MAP_FAILED) + return 1; + memset(region, 0xAB, page); // sentinel in the backed (readable) first page + printf("region = %p, page = %zu\n", (void *)region, page); + fflush(stdout); + return 0; // Set a breakpoint here +} `````````` </details> https://github.com/llvm/llvm-project/pull/212641 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
