[PATCH elfutils 0/2] fix two /proc/pid/maps inode parsing issues

[PATCH elfutils 0/2] fix two /proc/pid/maps inode parsing issues

[Yonghong Song]

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

[PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly

[Yonghong Song]

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

Re: [PATCH elfutils 1/2] [libdwfl] parse inode in /proc/pid/maps correctly

[Mark Wielaard]

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

[PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh

[Yonghong Song]

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

Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh

[Mark Wielaard]

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

Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh

[Yonghong Song]

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
> 

Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh

[Mark Wielaard]

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

Re: [PATCH elfutils 2/2] [tests] parse inode in /proc/pid/maps correctly in run-backtrace-data.sh

[Mark Wielaard]

[-- 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