libdrgn: fix handling of p_filesz < p_memsz in core dumps

I implemented the case of a segment in a core file with p_filesz <
p_memsz by treating the difference as zero bytes. This is correct for
ET_EXEC and ET_DYN, but for ET_CORE, it actually means that the memory
existed in the program but was not saved. For userspace core dumps, this
typically happens for read-only file mappings. For kernel core dumps,
makedumpfile does this to indicate memory that was excluded.

Instead, let's return a DRGN_FAULT_ERROR if an attempt is made to read
from these bytes. In the future, we need to read from the
executable/library files when we can.

Signed-off-by: Omar Sandoval <[email protected]>

Author: osandov

libdrgn/debug_info.c

@@ -1183,6 +1183,12 @@ struct drgn_error *drgn_debug_info_load(struct drgn_debug_info *dbinfo,
 	if (err)
 		goto err;
 
+	/*
+	 * TODO: for core dumps, we need to add memory reader segments for
+	 * read-only segments of the loaded binaries since those aren't saved in
+	 * the core dump.
+	 */
+
 	/*
 	 * If this fails, it's too late to roll back. This can only fail with
 	 * enomem, so it's not a big deal.

libdrgn/memory_reader.c

@@ -249,21 +249,19 @@ struct drgn_error *drgn_read_memory_file(void *buf, uint64_t address,
 					 void *arg, bool physical)
 {
 	struct drgn_memory_file_segment *file_segment = arg;
-	char *p = buf;
-	uint64_t file_offset = file_segment->file_offset + offset;
-	size_t file_count;
 
-	if (offset < file_segment->file_size) {
-		file_count = min((uint64_t)count,
-				 file_segment->file_size - offset);
-		count -= file_count;
-	} else {
-		file_count = 0;
+	if (offset > file_segment->file_size ||
+	    count > file_segment->file_size - offset) {
+		if (offset <= file_segment->file_size)
+			address += file_segment->file_size - offset;
+		return drgn_error_create_fault("memory not saved in core dump",
+					       address);
 	}
-	while (file_count) {
-		ssize_t ret;
 
-		ret = pread(file_segment->fd, p, file_count, file_offset);
+	uint64_t file_offset = file_segment->file_offset + offset;
+	char *p = buf;
+	while (count) {
+		ssize_t ret = pread(file_segment->fd, p, count, file_offset);
 		if (ret == -1) {
 			if (errno == EINTR) {
 				continue;
@@ -278,9 +276,9 @@ struct drgn_error *drgn_read_memory_file(void *buf, uint64_t address,
 						       address);
 		}
 		p += ret;
-		file_count -= ret;
+		address += ret;
+		count -= ret;
 		file_offset += ret;
 	}
-	memset(p, 0, count);
 	return NULL;
 }

libdrgn/memory_reader.h

@@ -123,8 +123,9 @@ struct drgn_memory_file_segment {
 	uint64_t file_offset;
 	/**
 	 * Size of the segment in the file. This may be less than the size of
-	 * the segment in memory, in which case the remaining bytes are treated
-	 * as if they contained zeroes.
+	 * the segment in memory, which means that the remaining bytes were in
+	 * the program's memory but were not saved in the core dump. Attempting
+	 * to read these bytes is treated as a fault.
 	 */
 	uint64_t file_size;
 	/** File descriptor. */

libdrgn/program.c

@@ -350,8 +350,10 @@ drgn_program_set_core_dump(struct drgn_program *prog, const char *path)
 		goto out_platform;
 	}
 
-	if ((is_proc_kcore || vmcoreinfo_note) &&
-	    prog->platform.arch->linux_kernel_pgtable_iterator_next) {
+	bool pgtable_reader =
+		(is_proc_kcore || vmcoreinfo_note) &&
+		prog->platform.arch->linux_kernel_pgtable_iterator_next;
+	if (pgtable_reader) {
 		/*
 		 * Try to read any memory that isn't in the core dump via the
 		 * page table.
@@ -381,6 +383,13 @@ drgn_program_set_core_dump(struct drgn_program *prog, const char *path)
 		prog->file_segments[j].fd = prog->core_fd;
 		prog->file_segments[j].eio_is_fault = false;
 		err = drgn_program_add_memory_segment(prog, phdr->p_vaddr,
+						      /*
+						       * Don't override the page
+						       * table reader for
+						       * unsaved regions.
+						       */
+						      pgtable_reader ?
+						      phdr->p_filesz :
 						      phdr->p_memsz,
 						      drgn_read_memory_file,
 						      &prog->file_segments[j],
@@ -391,6 +400,8 @@ drgn_program_set_core_dump(struct drgn_program *prog, const char *path)
 		    phdr->p_paddr != (is_64_bit ? UINT64_MAX : UINT32_MAX)) {
 			err = drgn_program_add_memory_segment(prog,
 							      phdr->p_paddr,
+							      pgtable_reader ?
+							      phdr->p_filesz :
 							      phdr->p_memsz,
 							      drgn_read_memory_file,
 							      &prog->file_segments[j],
@@ -436,6 +447,8 @@ drgn_program_set_core_dump(struct drgn_program *prog, const char *path)
 				phys_addr = phdr->p_vaddr - direct_mapping;
 				err = drgn_program_add_memory_segment(prog,
 								      phys_addr,
+								      pgtable_reader ?
+								      phdr->p_filesz :
 								      phdr->p_memsz,
 								      drgn_read_memory_file,
 								      &prog->file_segments[j],

tests/test_program.py

@@ -806,7 +806,7 @@ def test_physical(self):
         self.assertEqual(prog.read(0xFFFF0000, len(data)), data)
         self.assertEqual(prog.read(0xA0, len(data), physical=True), data)
 
-    def test_zero_fill(self):
+    def test_unsaved(self):
         data = b"hello, world"
         prog = Program()
         with tempfile.NamedTemporaryFile() as f:
@@ -825,4 +825,6 @@ def test_zero_fill(self):
             )
             f.flush()
             prog.set_core_dump(f.name)
-        self.assertEqual(prog.read(0xFFFF0000, len(data) + 4), data + bytes(4))
+        with self.assertRaisesRegex(FaultError, "memory not saved in core dump") as cm:
+            prog.read(0xFFFF0000, len(data) + 4)
+        self.assertEqual(cm.exception.address, 0xFFFF000C)