While adopting the algorithm for libkdumpfile, several corner cases
were found by a test case:
1. If the last part of a page is not present in the ELF file, it
should be replaced with zeroes. However, the check is incorrect.
2. If the beginning of a page is not present, following data is
read from an incorrect file offset.
3. If the page is split among several segments, the current
position in the buffer is not always taken into account.
Case 1 is a simple typo/braino (writing bufptr instead of endp).
To fix cases 2 and 3, it is best to update the paddr variable so that
it always corresponds to the current read position in the buffer.
I have tested the new code with a specially crafted ELF dump where one
page had the following (artificial) layout:
# PAGE RANGE STORED IN DATA
--------------------------------------------------
1 0x000-0x007 nowhere fake zero
2 0x008-0x067 LOAD #1 read from file
3 0x068-0x06f nowhere fake zero
4 0x070-0x13f LOAD #2 read from file
5 0x140-0x147 LOAD #2 zero (memsz > filesz)
6 0x148-0xff7 LOAD #3 read from file
7 0xff8-0xfff nowhere fake zero
Case 1 tests the conditional right after get_pt_load_extents().
Case 2 tests file read after missing data.
Case 3 tests the conditional from case 1 with non-zero buffer position.
Case 5 tests the last conditional in the loop (p < endp).
Case 6 tests exact match in get_pt_load_extents().
Case 7 tests the final conditional after the loop.
Signed-off-by: Petr Tesarik <ptesarik at suse.com>
---
makedumpfile.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -656,14 +656,15 @@ readpage_elf(unsigned long long paddr, v
p = bufptr;
endp = p + info->page_size;
while (p < endp) {
- idx = closest_pt_load(paddr + (p - bufptr), endp - p);
+ idx = closest_pt_load(paddr, endp - p);
if (idx < 0)
break;
get_pt_load_extents(idx, &phys_start, &phys_end, &offset, &size);
- if (phys_start > paddr + (p - bufptr)) {
+ if (phys_start > paddr) {
memset(p, 0, phys_start - paddr);
p += phys_start - paddr;
+ paddr = phys_start;
}
offset += paddr - phys_start;
@@ -677,6 +678,7 @@ readpage_elf(unsigned long long paddr, v
return FALSE;
}
p += size;
+ paddr += size;
}
if (p < endp) {
size = phys_end - paddr;
@@ -684,6 +686,7 @@ readpage_elf(unsigned long long paddr, v
size = endp - p;
memset(p, 0, size);
p += size;
+ paddr += size;
}
}
@@ -691,7 +694,7 @@ readpage_elf(unsigned long long paddr, v
ERRMSG("Attempt to read non-existent page at 0x%llx.\n",
paddr);
return FALSE;
- } else if (p < bufptr)
+ } else if (p < endp)
memset(p, 0, endp - p);
return TRUE;
@@ -708,14 +711,15 @@ readpage_elf_parallel(int fd_memory, uns
p = bufptr;
endp = p + info->page_size;
while (p < endp) {
- idx = closest_pt_load(paddr + (p - bufptr), endp - p);
+ idx = closest_pt_load(paddr, endp - p);
if (idx < 0)
break;
get_pt_load_extents(idx, &phys_start, &phys_end, &offset, &size);
- if (phys_start > paddr + (p - bufptr)) {
+ if (phys_start > paddr) {
memset(p, 0, phys_start - paddr);
p += phys_start - paddr;
+ paddr = phys_start;
}
offset += paddr - phys_start;
@@ -730,6 +734,7 @@ readpage_elf_parallel(int fd_memory, uns
return FALSE;
}
p += size;
+ paddr += size;
}
if (p < endp) {
size = phys_end - paddr;
@@ -737,6 +742,7 @@ readpage_elf_parallel(int fd_memory, uns
size = endp - p;
memset(p, 0, size);
p += size;
+ paddr += size;
}
}
@@ -744,7 +750,7 @@ readpage_elf_parallel(int fd_memory, uns
ERRMSG("Attempt to read non-existent page at 0x%llx.\n",
paddr);
return FALSE;
- } else if (p < bufptr)
+ } else if (p < endp)
memset(p, 0, endp - p);
return TRUE;
