[PATCH 2/3] perf symbol-elf: Fix zero-size symbol end calculation across .symtab and .dynsym

[Ian Rogers <irogers@google.com>]

When loading symbols from an ELF binary in dso__load_sym(),
dso__load_sym_internal() can be invoked multiple times: first for
.symtab (or .gnu_debugdata's .symtab) and then for .dynsym.

Previously, dso__load_sym_internal() ran symbols__fixup_end() and
symbols__fixup_duplicate() at the end of each individual symbol table
load. Consequently, a zero-size symbol in .symtab had its end address
extended to the start of the next .symtab symbol before .dynsym was
loaded. If an exported function existed only in .dynsym (for example,
when stripped from .symtab or when using .gnu_debugdata minidebuginfo)
at an address between the two .symtab symbols, the zero-size .symtab
symbol overlapped the .dynsym function and samples inside the .dynsym
function were misattributed to the preceding zero-size symbol.

Move symbols__fixup_end(), symbols__fixup_duplicate(), and
maps__fixup_end() from dso__load_sym_internal() to dso__load_sym() so
they run once after all symbol tables have been loaded into the DSO, and
add a unit test in 'perf test symbols' verifying that a zero-size
.symtab symbol ends at a following .dynsym-only function rather than
overlapping it.

Fixes: 87704345cc60 ("perf symbol-elf: Decode dynsym even if symtab exists")
Fixes: b10f74308e13 ("perf symbol: Support .gnu_debugdata for symbols")
Suggested-by: Alireza Haghdoost <haghdoost@uber.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/symbols.c   | 169 +++++++++++++++++++++++++++++++++++
 tools/perf/util/symbol-elf.c |  36 ++++----
 2 files changed, 190 insertions(+), 15 deletions(-)

diff --git a/tools/perf/tests/symbols.c b/tools/perf/tests/symbols.c
index b644d95c6be9..d3180db45ee6 100644
--- a/tools/perf/tests/symbols.c
+++ b/tools/perf/tests/symbols.c
@@ -2,8 +2,11 @@
 #include <linux/compiler.h>
 #include <linux/string.h>
 #include <sys/mman.h>
+#include <elf.h>
 #include <inttypes.h>
 #include <limits.h>
+#include <stdlib.h>
+#include <unistd.h>
 #include "debug.h"
 #include "dso.h"
 #include "env.h"
@@ -308,9 +311,175 @@ static int test__kallsyms_fixup_end(struct test_suite *test __maybe_unused,
 	return ret;
 }
 
+#ifdef HAVE_LIBELF_SUPPORT
+static int write_test_elf(int fd)
+{
+	static const char shstrtab[] = "\0.shstrtab\0.text\0.strtab\0.symtab\0.dynsym";
+	static const char strtab[] = "\0zfun\0efun\0after_fun";
+	struct {
+		Elf64_Ehdr ehdr;
+		Elf64_Shdr shdrs[6];
+		char shstr[sizeof(shstrtab)];
+		char str[sizeof(strtab)];
+		Elf64_Sym symtab[3];
+		Elf64_Sym dynsym[2];
+		u8 text[0x60];
+	} img;
+
+	memset(&img, 0, sizeof(img));
+	memcpy(img.ehdr.e_ident, ELFMAG, SELFMAG);
+	img.ehdr.e_ident[EI_CLASS] = ELFCLASS64;
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+	img.ehdr.e_ident[EI_DATA] = ELFDATA2MSB;
+#else
+	img.ehdr.e_ident[EI_DATA] = ELFDATA2LSB;
+#endif
+	img.ehdr.e_ident[EI_VERSION] = EV_CURRENT;
+	img.ehdr.e_type = ET_DYN;
+	img.ehdr.e_machine = EM_X86_64;
+	img.ehdr.e_version = EV_CURRENT;
+	img.ehdr.e_shoff = offsetof(typeof(img), shdrs);
+	img.ehdr.e_ehsize = sizeof(Elf64_Ehdr);
+	img.ehdr.e_shentsize = sizeof(Elf64_Shdr);
+	img.ehdr.e_shnum = ARRAY_SIZE(img.shdrs);
+	img.ehdr.e_shstrndx = 1;
+
+	memcpy(img.shstr, shstrtab, sizeof(shstrtab));
+	memcpy(img.str, strtab, sizeof(strtab));
+
+	/* Section 1: .shstrtab */
+	img.shdrs[1].sh_name = 1;
+	img.shdrs[1].sh_type = SHT_STRTAB;
+	img.shdrs[1].sh_offset = offsetof(typeof(img), shstr);
+	img.shdrs[1].sh_size = sizeof(shstrtab);
+
+	/* Section 2: .text */
+	img.shdrs[2].sh_name = 11;
+	img.shdrs[2].sh_type = SHT_PROGBITS;
+	img.shdrs[2].sh_flags = SHF_ALLOC | SHF_EXECINSTR;
+	img.shdrs[2].sh_addr = 0x1000;
+	img.shdrs[2].sh_offset = offsetof(typeof(img), text);
+	img.shdrs[2].sh_size = sizeof(img.text);
+
+	/* Section 3: .strtab */
+	img.shdrs[3].sh_name = 17;
+	img.shdrs[3].sh_type = SHT_STRTAB;
+	img.shdrs[3].sh_flags = SHF_ALLOC;
+	img.shdrs[3].sh_offset = offsetof(typeof(img), str);
+	img.shdrs[3].sh_size = sizeof(strtab);
+
+	/*
+	 * Section 4: .symtab with zero-size zfun at 0x1010 and after_fun at
+	 * 0x1040, but without efun (simulating objcopy --strip-symbol=efun or
+	 * .gnu_debugdata minidebuginfo where exported symbols live only in
+	 * .dynsym).
+	 */
+	img.shdrs[4].sh_name = 25;
+	img.shdrs[4].sh_type = SHT_SYMTAB;
+	img.shdrs[4].sh_offset = offsetof(typeof(img), symtab);
+	img.shdrs[4].sh_size = sizeof(img.symtab);
+	img.shdrs[4].sh_link = 3;
+	img.shdrs[4].sh_info = 2;
+	img.shdrs[4].sh_entsize = sizeof(Elf64_Sym);
+
+	img.symtab[1].st_name = 1; /* zfun */
+	img.symtab[1].st_info = ELF64_ST_INFO(STB_LOCAL, STT_FUNC);
+	img.symtab[1].st_shndx = 2;
+	img.symtab[1].st_value = 0x1010;
+	img.symtab[1].st_size = 0;
+
+	img.symtab[2].st_name = 11; /* after_fun */
+	img.symtab[2].st_info = ELF64_ST_INFO(STB_GLOBAL, STT_FUNC);
+	img.symtab[2].st_shndx = 2;
+	img.symtab[2].st_value = 0x1040;
+	img.symtab[2].st_size = 0x10;
+
+	/* Section 5: .dynsym with efun at 0x1020..0x1030 */
+	img.shdrs[5].sh_name = 33;
+	img.shdrs[5].sh_type = SHT_DYNSYM;
+	img.shdrs[5].sh_flags = SHF_ALLOC;
+	img.shdrs[5].sh_offset = offsetof(typeof(img), dynsym);
+	img.shdrs[5].sh_size = sizeof(img.dynsym);
+	img.shdrs[5].sh_link = 3;
+	img.shdrs[5].sh_info = 1;
+	img.shdrs[5].sh_entsize = sizeof(Elf64_Sym);
+
+	img.dynsym[1].st_name = 6; /* efun */
+	img.dynsym[1].st_info = ELF64_ST_INFO(STB_GLOBAL, STT_FUNC);
+	img.dynsym[1].st_shndx = 2;
+	img.dynsym[1].st_value = 0x1020;
+	img.dynsym[1].st_size = 0x10;
+
+	return write(fd, &img, sizeof(img)) == (ssize_t)sizeof(img) ? 0 : -1;
+}
+
+static int test__elf_symtab_dynsym_fixup_end(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	char tmpfile[] = "/tmp/perf-test-symtab-dynsym-XXXXXX";
+	struct test_info ti;
+	struct map *map = NULL;
+	struct dso *dso;
+	u64 text_offs;
+	int fd, ret;
+
+	fd = mkstemp(tmpfile);
+	if (fd < 0)
+		return TEST_FAIL;
+
+	if (write_test_elf(fd)) {
+		close(fd);
+		unlink(tmpfile);
+		return TEST_FAIL;
+	}
+	close(fd);
+
+	ret = init_test_info(&ti);
+	if (ret != TEST_OK) {
+		unlink(tmpfile);
+		return ret;
+	}
+
+	ret = create_map(&ti, tmpfile, &map);
+	if (ret != TEST_OK)
+		goto out_exit;
+
+	dso = map__dso(map);
+	if (dso__load(dso, map) <= 0) {
+		ret = TEST_FAIL;
+		goto out_put;
+	}
+
+	/* Verify no overlapping or zero-length symbols exist. */
+	ret = test_dso(dso);
+	text_offs = 0x1000 - dso__text_offset(dso);
+	if (check_symbol(dso, text_offs + 0x14, "zfun", text_offs + 0x20))
+		ret = TEST_FAIL;
+	if (check_symbol(dso, text_offs + 0x24, "efun", text_offs + 0x30))
+		ret = TEST_FAIL;
+	if (check_symbol(dso, text_offs + 0x44, "after_fun", text_offs + 0x50))
+		ret = TEST_FAIL;
+
+out_put:
+	map__put(map);
+out_exit:
+	exit_test_info(&ti);
+	unlink(tmpfile);
+	return ret;
+}
+#else
+static int test__elf_symtab_dynsym_fixup_end(struct test_suite *test __maybe_unused,
+					     int subtest __maybe_unused)
+{
+	return TEST_SKIP;
+}
+#endif
+
 static struct test_case tests__symbols[] = {
 	TEST_CASE("Symbols", symbols),
 	TEST_CASE("Kallsyms symbol ends", kallsyms_fixup_end),
+	TEST_CASE("ELF zero-size symbol end across .symtab and .dynsym",
+		  elf_symtab_dynsym_fixup_end),
 	{ .name = NULL, }
 };
 
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 1e2716c6af45..77ef5b2dbf32 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1789,21 +1789,6 @@ dso__load_sym_internal(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 		nr++;
 	}
 	dso__put(curr_dso);
-
-	/*
-	 * For misannotated, zeroed, ASM function sizes.
-	 */
-	if (nr > 0) {
-		symbols__fixup_end(dso__symbols(dso), false);
-		symbols__fixup_duplicate(dso__symbols(dso));
-		if (kmap) {
-			/*
-			 * We need to fixup this here too because we create new
-			 * maps here, for things like vsyscall sections.
-			 */
-			maps__fixup_end(kmaps);
-		}
-	}
 	return nr;
 out_elf_end:
 	dso__put(curr_dso);
@@ -1864,6 +1849,27 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 		nr += err;
 	}
 
+	/*
+	 * For misannotated, zeroed, ASM function sizes. Run after all symbol
+	 * tables (.symtab, .dynsym, .gnu_debugdata) have been loaded so a
+	 * zero-size symbol in .symtab ends at the next symbol across all
+	 * tables rather than overlapping a .dynsym-only function.
+	 */
+	if (nr > 0) {
+		struct kmap *kmap = dso__kernel(dso) ? map__kmap(map) : NULL;
+		struct maps *kmaps = kmap ? map__kmaps(map) : NULL;
+
+		symbols__fixup_end(dso__symbols(dso), false);
+		symbols__fixup_duplicate(dso__symbols(dso));
+		if (kmap) {
+			/*
+			 * We need to fixup this here too because we create new
+			 * maps here, for things like vsyscall sections.
+			 */
+			maps__fixup_end(kmaps);
+		}
+	}
+
 	return nr;
 }
 
-- 
2.56.0.317.g454f8302e1-goog