* [PATCH elfutils 0/2] fix two /proc/pid/maps inode parsing issues @ 2019-01-25 21:20 Yonghong Song 2019-01-25 21:20 ` [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh Yonghong Song 2019-01-25 21:20 ` [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly Yonghong Song 0 siblings, 2 replies; 8+ messages in thread From: Yonghong Song @ 2019-01-25 21:20 UTC (permalink / raw) To: elfutils-devel, ast; +Cc: yhs The inode number in /proc/pid/maps has type "unsigned long". The current parsing in libdwfl/linux-proc-maps.c and tests/backtrace-data.c is not correct and it triggered the following four test failures in one x64 box. FAIL: dwfl-bug-fd-leak FAIL: run-backtrace-data.sh FAIL: run-backtrace-dwarf.sh FAIL: vdsosyms The offending map entry 7f269223d000-7f269226b000 r-xp 00000000 00:50 10224326387095067468 /home/... has an inode number beyond the valid range of type "long". This patch set fixed the problem in libdwfl and tests respectively. Yonghong Song (2): [libdwfl] parse inode in /proc/pid/maps correctly [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh libdwfl/linux-proc-maps.c | 2 +- tests/backtrace-data.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh 2019-01-25 21:20 [PATCH elfutils 0/2] fix two /proc/pid/maps inode parsing issues Yonghong Song @ 2019-01-25 21:20 ` Yonghong Song 2019-01-29 20:50 ` Mark Wielaard 2019-01-25 21:20 ` [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly Yonghong Song 1 sibling, 1 reply; 8+ messages in thread From: Yonghong Song @ 2019-01-25 21:20 UTC (permalink / raw) To: elfutils-devel, ast; +Cc: yhs The backtrace-data.c parsed the inode in /proc/pid/maps with format "%*x". This caused failure if inode is big. For example, 7f269223d000-7f269226b000 r-xp 00000000 00:50 10224326387095067468 /home/... The correct format should be "%*lu" to reflect inode "unsigned long" type. But that caused the following compilation error. acktrace-data.c: In function ‘maps_lookup’: backtrace-data.c:109:22: error: use of assignment suppression and length modifier together in gnu_scanf format [-Werror=format=] i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*lu", &start, &end, &offset); Fix the test with inode format string "%*s" then. Signed-off-by: Yonghong Song <yhs@fb.com> --- tests/backtrace-data.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c index 3a91c664..85ae9729 100644 --- a/tests/backtrace-data.c +++ b/tests/backtrace-data.c @@ -106,7 +106,7 @@ maps_lookup (pid_t pid, Dwarf_Addr addr, GElf_Addr *basep) { // 37e3c22000-37e3c23000 rw-p 00022000 00:11 49532 /lib64/ld-2.14.90.so */ unsigned long start, end, offset; - i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*x", &start, &end, &offset); + i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*s", &start, &end, &offset); assert (errno == 0); if (i != 3) break; -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh 2019-01-25 21:20 ` [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh Yonghong Song @ 2019-01-29 20:50 ` Mark Wielaard 2019-01-29 21:23 ` Yonghong Song 0 siblings, 1 reply; 8+ messages in thread From: Mark Wielaard @ 2019-01-29 20:50 UTC (permalink / raw) To: Yonghong Song; +Cc: elfutils-devel, ast On Fri, Jan 25, 2019 at 01:20:09PM -0800, Yonghong Song wrote: > The backtrace-data.c parsed the inode in /proc/pid/maps with > format "%*x". > This caused failure if inode is big. For example, > 7f269223d000-7f269226b000 r-xp 00000000 00:50 10224326387095067468 /home/... I have a bit of trouble replicating this (with a simple sscanf). How exactly does it fail? > The correct format should be "%*lu" to reflect inode "unsigned long" type. > But that caused the following compilation error. > acktrace-data.c: In function âmaps_lookupâ: > backtrace-data.c:109:22: error: use of assignment suppression and length modifier > together in gnu_scanf format [-Werror=format=] > i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*lu", &start, &end, &offset); Not that it matters much, since we are really ignoring the rest of the line and this is just a test. But I do wonder why %*u doesn't work. The warning says you cannot combine a length specifier with a ignored format specifier. Which kind of makes sense given that the length is for the variable to assign the value for, not the format. So it seems $*u should do the trick. But since I haven't been able to make the original fail, I might be wrong. Cheers, Mark ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh 2019-01-29 20:50 ` Mark Wielaard @ 2019-01-29 21:23 ` Yonghong Song 2019-01-31 21:14 ` Mark Wielaard 0 siblings, 1 reply; 8+ messages in thread From: Yonghong Song @ 2019-01-29 21:23 UTC (permalink / raw) To: Mark Wielaard; +Cc: elfutils-devel, Alexei Starovoitov On 1/29/19 12:50 PM, Mark Wielaard wrote: > On Fri, Jan 25, 2019 at 01:20:09PM -0800, Yonghong Song wrote: >> The backtrace-data.c parsed the inode in /proc/pid/maps with >> format "%*x". >> This caused failure if inode is big. For example, >> 7f269223d000-7f269226b000 r-xp 00000000 00:50 10224326387095067468 /home/... > > I have a bit of trouble replicating this (with a simple sscanf). > How exactly does it fail? The error message looks like: -bash-4.4$ cat run-backtrace-data.sh.log backtrace-data: /home/engshare/elfutils/0.174/src/elfutils-0.174/tests/backtrace-data.c:110: maps_lookup: Assertion `errno == 0' failed. /home/engshare/elfutils/0.174/src/elfutils-0.174/tests/test-subr.sh: line 84: 3123578 Aborted (core dumped) LD_LIBRARY_PATH="${built_library_path}${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH" $VALGRIND_CMD "$@" data: no main -bash-4.4$ The reason is errno is ERANGE. > >> The correct format should be "%*lu" to reflect inode "unsigned long" type. >> But that caused the following compilation error. >> acktrace-data.c: In function ‘maps_lookup’: >> backtrace-data.c:109:22: error: use of assignment suppression and length modifier >> together in gnu_scanf format [-Werror=format=] >> i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*lu", &start, &end, &offset); > > Not that it matters much, since we are really ignoring the rest of the > line and this is just a test. But I do wonder why %*u doesn't work. > The warning says you cannot combine a length specifier with > a ignored format specifier. Which kind of makes sense given that the > length is for the variable to assign the value for, not the format. > So it seems $*u should do the trick. But since I haven't been able > to make the original fail, I might be wrong. "%u" works as well. Let me submit another patch for this. Thanks! > > Cheers, > > Mark > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh 2019-01-29 21:23 ` Yonghong Song @ 2019-01-31 21:14 ` Mark Wielaard 2019-01-31 22:26 ` Mark Wielaard 0 siblings, 1 reply; 8+ messages in thread From: Mark Wielaard @ 2019-01-31 21:14 UTC (permalink / raw) To: Yonghong Song; +Cc: elfutils-devel, Alexei Starovoitov On Tue, Jan 29, 2019 at 09:23:39PM +0000, Yonghong Song wrote: > On 1/29/19 12:50 PM, Mark Wielaard wrote: > > On Fri, Jan 25, 2019 at 01:20:09PM -0800, Yonghong Song wrote: > >> The backtrace-data.c parsed the inode in /proc/pid/maps with > >> format "%*x". > >> This caused failure if inode is big. For example, > >> 7f269223d000-7f269226b000 r-xp 00000000 00:50 10224326387095067468 /home/... > > > > I have a bit of trouble replicating this (with a simple sscanf). > > How exactly does it fail? > > The error message looks like: > > -bash-4.4$ cat run-backtrace-data.sh.log > backtrace-data: > /home/engshare/elfutils/0.174/src/elfutils-0.174/tests/backtrace-data.c:110: > maps_lookup: Assertion `errno == 0' failed. > /home/engshare/elfutils/0.174/src/elfutils-0.174/tests/test-subr.sh: > line 84: 3123578 Aborted (core dumped) > LD_LIBRARY_PATH="${built_library_path}${LD_LIBRARY_PATH:+:}$LD_LIBRARY_PATH" > $VALGRIND_CMD "$@" > data: no main > -bash-4.4$ > > The reason is errno is ERANGE. Aha. Thanks. That is what I get for not testing against the actual testcase. I wasn't checking errno. But that certainly explains the issue. The %*x would parse the number as an hex number, causing the number to be so big that it generated an ERANGE (also it would accept some things that it shouldn't). But with %*u it uses base-10 encoding, so the number doesn't get too big. So your fix is correct. But the testcase is also slightly wrong. It really shouldn't check errno if the function didn't fail. There is no guarantee that it will be zero. > "%u" works as well. Let me submit another patch for this. Thanks, Mark ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh 2019-01-31 21:14 ` Mark Wielaard @ 2019-01-31 22:26 ` Mark Wielaard 0 siblings, 0 replies; 8+ messages in thread From: Mark Wielaard @ 2019-01-31 22:26 UTC (permalink / raw) To: Yonghong Song; +Cc: elfutils-devel, Alexei Starovoitov [-- Attachment #1: Type: text/plain, Size: 504 bytes --] On Thu, Jan 31, 2019 at 10:14:32PM +0100, Mark Wielaard wrote: > So your fix is correct. > But the testcase is also slightly wrong. > It really shouldn't check errno if the function didn't fail. > There is no guarantee that it will be zero. So, even though it found a bug, I am removing these asserts. They really aren't correct. When a function fails, it will set errno. But if a function succeeds, there is no guarantee that it will set errno to zero. Attached patch pushed to master. Cheers, Mark [-- Attachment #2: 0001-tests-Remove-assert-errno-0-from-tests.patch --] [-- Type: text/x-diff, Size: 5639 bytes --] From fe7d3f3635e66fe8ec1fde91f886857b0dea7d22 Mon Sep 17 00:00:00 2001 From: Mark Wielaard <mark@klomp.org> Date: Thu, 31 Jan 2019 23:18:25 +0100 Subject: [PATCH] tests: Remove assert (errno == 0) from tests. When a function fails it might set errno. But it isn't a guarantee that if a function succeeds that it sets errno to zero. Signed-off-by: Mark Wielaard <mark@klomp.org> --- tests/ChangeLog | 10 ++++++++++ tests/backtrace-child.c | 2 -- tests/backtrace-data.c | 10 ---------- tests/backtrace.c | 5 ----- 4 files changed, 10 insertions(+), 17 deletions(-) diff --git a/tests/ChangeLog b/tests/ChangeLog index c91764fb3..c2d868878 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -1,3 +1,13 @@ +2019-01-31 Mark Wielaard <mark@klomp.org> + + * backtrace-child.c (stdarg): Remove assert (errno == 0). + (main): Likewise. + * backtrace-data.c (maps_lookup): Likewise. + (set_initial_registers): Likewise. + (main): Likewise. + * backtrace.c (prepare_thread): Likewise. + (exec_dump): Likewise. + 2019-01-29 Yonghong Song <yhs@fb.com> * backtrace-data.c (maps_lookup): Use %*u, not %*x, to parse diff --git a/tests/backtrace-child.c b/tests/backtrace-child.c index 9c6ba94f1..8bfed478c 100644 --- a/tests/backtrace-child.c +++ b/tests/backtrace-child.c @@ -164,7 +164,6 @@ stdarg (int f UNUSED, ...) if (ptraceme) { long l = ptrace (PTRACE_TRACEME, 0, NULL, NULL); - assert (errno == 0); assert (l == 0); } #ifdef RAISE_JMP_PATCHING @@ -236,7 +235,6 @@ main (int argc UNUSED, char **argv) { errno = 0; long l = ptrace (PTRACE_TRACEME, 0, NULL, NULL); - assert (errno == 0); assert (l == 0); } if (gencore) diff --git a/tests/backtrace-data.c b/tests/backtrace-data.c index b389d6aff..907b47801 100644 --- a/tests/backtrace-data.c +++ b/tests/backtrace-data.c @@ -96,10 +96,8 @@ maps_lookup (pid_t pid, Dwarf_Addr addr, GElf_Addr *basep) { char *fname; int i = asprintf (&fname, "/proc/%ld/maps", (long) pid); - assert (errno == 0); assert (i > 0); FILE *f = fopen (fname, "r"); - assert (errno == 0); assert (f); free (fname); for (;;) @@ -107,7 +105,6 @@ maps_lookup (pid_t pid, Dwarf_Addr addr, GElf_Addr *basep) // 37e3c22000-37e3c23000 rw-p 00022000 00:11 49532 /lib64/ld-2.14.90.so */ unsigned long start, end, offset; i = fscanf (f, "%lx-%lx %*s %lx %*x:%*x %*u", &start, &end, &offset); - assert (errno == 0); if (i != 3) break; char *filename = strdup (""); @@ -129,7 +126,6 @@ maps_lookup (pid_t pid, Dwarf_Addr addr, GElf_Addr *basep) if (start <= addr && addr < end) { i = fclose (f); - assert (errno == 0); assert (i == 0); *basep = start - offset; @@ -183,7 +179,6 @@ set_initial_registers (Dwfl_Thread *thread, struct user_regs_struct user_regs; long l = ptrace (PTRACE_GETREGS, child, NULL, &user_regs); - assert (errno == 0); assert (l == 0); Dwarf_Word dwarf_regs[17]; @@ -284,11 +279,9 @@ main (int argc __attribute__ ((unused)), char **argv __attribute__ ((unused))) switch (child) { case -1: - assert (errno == 0); assert (0); case 0:; long l = ptrace (PTRACE_TRACEME, 0, NULL, NULL); - assert (errno == 0); assert (l == 0); raise (SIGUSR1); return 0; @@ -298,7 +291,6 @@ main (int argc __attribute__ ((unused)), char **argv __attribute__ ((unused))) int status; pid_t pid = waitpid (child, &status, 0); - assert (errno == 0); assert (pid == child); assert (WIFSTOPPED (status)); assert (WSTOPSIG (status) == SIGUSR1); @@ -316,7 +308,6 @@ main (int argc __attribute__ ((unused)), char **argv __attribute__ ((unused))) struct user_regs_struct user_regs; long l = ptrace (PTRACE_GETREGS, child, NULL, &user_regs); - assert (errno == 0); assert (l == 0); report_module (dwfl, child, user_regs.rip); @@ -330,7 +321,6 @@ main (int argc __attribute__ ((unused)), char **argv __attribute__ ((unused))) dwfl_end (dwfl); kill (child, SIGKILL); pid = waitpid (child, &status, 0); - assert (errno == 0); assert (pid == child); assert (WIFSIGNALED (status)); assert (WTERMSIG (status) == SIGKILL); diff --git a/tests/backtrace.c b/tests/backtrace.c index 24ab68dd4..05e8ef82e 100644 --- a/tests/backtrace.c +++ b/tests/backtrace.c @@ -281,16 +281,13 @@ prepare_thread (pid_t pid2 __attribute__ ((unused)), struct user_regs_struct user_regs; errno = 0; l = ptrace (PTRACE_GETREGS, pid2, 0, (intptr_t) &user_regs); - assert (errno == 0); assert (l == 0); user_regs.rip = (intptr_t) jmp; l = ptrace (PTRACE_SETREGS, pid2, 0, (intptr_t) &user_regs); - assert (errno == 0); assert (l == 0); l = ptrace (PTRACE_CONT, pid2, NULL, (void *) (intptr_t) SIGUSR2); int status; pid_t got = waitpid (pid2, &status, __WALL); - assert (errno == 0); assert (got == pid2); assert (WIFSTOPPED (status)); assert (WSTOPSIG (status) == SIGUSR1); @@ -358,7 +355,6 @@ exec_dump (const char *exec) errno = 0; int status; pid_t got = waitpid (pid, &status, 0); - assert (errno == 0); assert (got == pid); assert (WIFSTOPPED (status)); // Main thread will signal SIGUSR2. Other thread will signal SIGUSR1. @@ -368,7 +364,6 @@ exec_dump (const char *exec) __WCLONE, probably despite pthread_create already had to be called the new task is not yet alive enough for waitpid. */ pid_t pid2 = waitpid (-1, &status, __WALL); - assert (errno == 0); assert (pid2 > 0); assert (pid2 != pid); assert (WIFSTOPPED (status)); -- 2.20.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly 2019-01-25 21:20 [PATCH elfutils 0/2] fix two /proc/pid/maps inode parsing issues Yonghong Song 2019-01-25 21:20 ` [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh Yonghong Song @ 2019-01-25 21:20 ` Yonghong Song 2019-01-29 20:10 ` Mark Wielaard 1 sibling, 1 reply; 8+ messages in thread From: Yonghong Song @ 2019-01-25 21:20 UTC (permalink / raw) To: elfutils-devel, ast; +Cc: yhs The inode number in /proc/pid/maps is displayed as "unsigned long" type. In one of our x64 system, we have inode number exceeding valid "long" type range, which caused the following test failure: FAIL: dwfl-bug-fd-leak FAIL: run-backtrace-dwarf.sh FAIL: vdsosyms The offending map entry: 7f269246b000-7f269246c000 rw-p 0002e000 00:50 10224326387095067468 /home/... This patch changed sscanf inode number type from PRIi64 to PRIu64 and fixed the problem. Signed-off-by: Yonghong Song <yhs@fb.com> --- libdwfl/linux-proc-maps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libdwfl/linux-proc-maps.c b/libdwfl/linux-proc-maps.c index c4438c0f..719cba68 100644 --- a/libdwfl/linux-proc-maps.c +++ b/libdwfl/linux-proc-maps.c @@ -217,7 +217,7 @@ proc_maps_report (Dwfl *dwfl, FILE *f, GElf_Addr sysinfo_ehdr, pid_t pid) uint64_t ino; int nread = -1; if (sscanf (line, "%" PRIx64 "-%" PRIx64 " %*s %" PRIx64 - " %x:%x %" PRIi64 " %n", + " %x:%x %" PRIu64 " %n", &start, &end, &offset, &dmajor, &dminor, &ino, &nread) < 6 || nread <= 0) { -- 2.17.1 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly 2019-01-25 21:20 ` [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly Yonghong Song @ 2019-01-29 20:10 ` Mark Wielaard 0 siblings, 0 replies; 8+ messages in thread From: Mark Wielaard @ 2019-01-29 20:10 UTC (permalink / raw) To: Yonghong Song; +Cc: elfutils-devel, ast On Fri, Jan 25, 2019 at 01:20:08PM -0800, Yonghong Song wrote: > The inode number in /proc/pid/maps is displayed as "unsigned long" > type. > > In one of our x64 system, we have inode number exceeding valid "long" > type range, which caused the following test failure: > FAIL: dwfl-bug-fd-leak > FAIL: run-backtrace-dwarf.sh > FAIL: vdsosyms > > The offending map entry: > 7f269246b000-7f269246c000 rw-p 0002e000 00:50 10224326387095067468 /home/... > > This patch changed sscanf inode number type from PRIi64 to PRIu64 > and fixed the problem. Reading the inode number as a signed value was indeed odd. Added a ChangeLog entry and pused to master. Thanks, Mark ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-31 22:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-25 21:20 [PATCH elfutils 0/2] fix two /proc/pid/maps inode parsing issues Yonghong Song 2019-01-25 21:20 ` [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh Yonghong Song 2019-01-29 20:50 ` Mark Wielaard 2019-01-29 21:23 ` Yonghong Song 2019-01-31 21:14 ` Mark Wielaard 2019-01-31 22:26 ` Mark Wielaard 2019-01-25 21:20 ` [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly Yonghong Song 2019-01-29 20:10 ` Mark Wielaard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).