[patch] Fix 64-bit->32-bit vDSO reporting

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi Mark,

jankratochvil/auxv32

On Tue, 29 Jan 2013 11:22:17 +0100, Mark Wielaard wrote:
> > Re: [patch] Fix 64-bit->32-bit vDSO reporting
> > https://lists.fedorahosted.org/pipermail/elfutils-devel/2012-October/002704.html
> > Message-ID: <20121012150900.GA24645@host2.jankratochvil.net>
>
> I think this one is fine. Though I admit that I found the first less
> efficient version a bit easier to understand. Could add a comment in the
> code to explain when/why the valid32/64 heuristic works/doesn't work and
> that it is there to prevent having to open pid/exe in normal cases?

added the comment.


Thanks,
Jan


commit 911d38861fb6abe4d1305e594dc0005e283f33e9
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date:   Wed Oct 10 20:42:30 2012 +0200

    libdwfl/
    linux-proc-maps.c: Include system.h.
    (PROCEXEFMT, get_pid_class): New.
    (grovel_auxv): Detect 32-bit vs. 64-bit auxv, possibly call get_pid_class.
    
    Signed-off-by: Jan Kratochvil <jan.kratochvil@redhat.com>

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index cf5b42e..f38c495 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,10 @@
+2013-01-29  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* linux-proc-maps.c: Include system.h.
+	(PROCEXEFMT, get_pid_class): New.
+	(grovel_auxv): Detect 32-bit vs. 64-bit auxv, possibly call
+	get_pid_class.
+
 2013-01-23  Mark Wielaard  <mjw@redhat.com>
 
 	* dwfl_module_getdwarf.c (find_aux_sym): Don't substract one
diff --git a/libdwfl/linux-proc-maps.c b/libdwfl/linux-proc-maps.c
index 4fbe90d..e96b008 100644
--- a/libdwfl/linux-proc-maps.c
+++ b/libdwfl/linux-proc-maps.c
@@ -39,14 +39,53 @@
 #include <unistd.h>
 #include <assert.h>
 #include <endian.h>
+#include "system.h"
 
 
 #define PROCMAPSFMT	"/proc/%d/maps"
 #define PROCMEMFMT	"/proc/%d/mem"
 #define PROCAUXVFMT	"/proc/%d/auxv"
+#define PROCEXEFMT	"/proc/%d/exe"
 
 
-/* Search /proc/PID/auxv for the AT_SYSINFO_EHDR tag.  */
+/* Return ELFCLASS64 or ELFCLASS32 for the main ELF executable.  Return
+   ELFCLASSNONE for an error.  */
+
+static unsigned char
+get_pid_class (pid_t pid)
+{
+  char *fname;
+  if (asprintf (&fname, PROCEXEFMT, pid) < 0)
+    return ELFCLASSNONE;
+
+  int fd = open64 (fname, O_RDONLY);
+  free (fname);
+  if (fd < 0)
+    return ELFCLASSNONE;
+
+  unsigned char buf[EI_CLASS + 1];
+  ssize_t nread = pread_retry (fd, &buf, sizeof buf, 0);
+  close (fd);
+  if (nread != sizeof (buf) || buf[EI_MAG0] != ELFMAG0
+      || buf[EI_MAG1] != ELFMAG1 || buf[EI_MAG2] != ELFMAG2
+      || buf[EI_MAG3] != ELFMAG3
+      || (buf[EI_CLASS] != ELFCLASS64 && buf[EI_CLASS] != ELFCLASS32))
+    return ELFCLASSNONE;
+
+  return buf[EI_CLASS];
+}
+
+/* Search /proc/PID/auxv for the AT_SYSINFO_EHDR tag.
+
+   It would be most easier to call get_pid_class and parse everything according
+   to the 32-bit or 64-bit class.  But this would bring an overhead of syscalls
+   opening the "/proc/%d/exe" file.
+
+   Therefore this function tries to parse the "/proc/%d/auxv" content both
+   ways, as if it was the 32-bit format and also if it was the 64-bit format.
+   Only if it gives some valid data in both cases get_pid_class gets called.
+   In most cases only one of the format bit sizes gives valid data and the
+   get_pid_class call overhead can be saved.  */
 
 static int
 grovel_auxv (pid_t pid, Dwfl *dwfl, GElf_Addr *sysinfo_ehdr)
@@ -60,61 +99,72 @@ grovel_auxv (pid_t pid, Dwfl *dwfl, GElf_Addr *sysinfo_ehdr)
   if (fd < 0)
     return errno == ENOENT ? 0 : errno;
 
+  GElf_Addr sysinfo_ehdr64 = 0, sysinfo_ehdr32 = 0;
+  GElf_Addr segment_align64 = dwfl->segment_align;
+  GElf_Addr segment_align32 = dwfl->segment_align;
+  off_t offset = 0;
   ssize_t nread;
+  union
+  {
+    Elf64_auxv_t a64[64];
+    Elf32_auxv_t a32[128];
+  } d;
   do
     {
-      union
-      {
-	char buffer[sizeof (long int) * 2 * 64];
-	Elf64_auxv_t a64[sizeof (long int) * 2 * 64 / sizeof (Elf64_auxv_t)];
-	Elf32_auxv_t a32[sizeof (long int) * 2 * 32 / sizeof (Elf32_auxv_t)];
-      } d;
-      nread = read (fd, &d, sizeof d);
-      if (nread > 0)
+      eu_static_assert (sizeof d.a64 == sizeof d.a32);
+      nread = pread_retry (fd, d.a64, sizeof d.a64, offset);
+      if (nread < 0)
+	return errno;
+      for (unsigned a32i = 0; a32i < nread / sizeof d.a32[0]; a32i++)
 	{
-	  switch (sizeof (long int))
-	    {
-	    case 4:
-	      for (size_t i = 0; (char *) &d.a32[i] < &d.buffer[nread]; ++i)
-		if (d.a32[i].a_type == AT_SYSINFO_EHDR)
-		  {
-		    *sysinfo_ehdr = d.a32[i].a_un.a_val;
-		    if (dwfl->segment_align > 1)
-		      {
-			nread = 0;
-			break;
-		      }
-		  }
-		else if (d.a32[i].a_type == AT_PAGESZ
-			 && dwfl->segment_align <= 1)
-		  dwfl->segment_align = d.a32[i].a_un.a_val;
+	  const Elf32_auxv_t *a32 = d.a32 + a32i;
+	  switch (a32->a_type)
+	  {
+	    case AT_SYSINFO_EHDR:
+	      sysinfo_ehdr32 = a32->a_un.a_val;
+	      break;
+	    case AT_PAGESZ:
+	      segment_align32 = a32->a_un.a_val;
 	      break;
-	    case 8:
-	      for (size_t i = 0; (char *) &d.a64[i] < &d.buffer[nread]; ++i)
-		if (d.a64[i].a_type == AT_SYSINFO_EHDR)
-		  {
-		    *sysinfo_ehdr = d.a64[i].a_un.a_val;
-		    if (dwfl->segment_align > 1)
-		      {
-			nread = 0;
-			break;
-		      }
-		  }
-		else if (d.a64[i].a_type == AT_PAGESZ
-			 && dwfl->segment_align <= 1)
-		  dwfl->segment_align = d.a64[i].a_un.a_val;
+	  }
+	}
+      for (unsigned a64i = 0; a64i < nread / sizeof d.a64[0]; a64i++)
+	{
+	  const Elf64_auxv_t *a64 = d.a64 + a64i;
+	  switch (a64->a_type)
+	  {
+	    case AT_SYSINFO_EHDR:
+	      sysinfo_ehdr64 = a64->a_un.a_val;
 	      break;
-	    default:
-	      abort ();
+	    case AT_PAGESZ:
+	      segment_align64 = a64->a_un.a_val;
 	      break;
-	    }
+	  }
 	}
+      offset += nread;
     }
-  while (nread > 0);
+  while (nread == sizeof d.a64);
 
   close (fd);
 
-  return nread < 0 ? errno : 0;
+  bool valid64 = sysinfo_ehdr64 || segment_align64 != dwfl->segment_align;
+  bool valid32 = sysinfo_ehdr32 || segment_align32 != dwfl->segment_align;
+
+  unsigned char pid_class = ELFCLASSNONE;
+  if (valid64 && valid32)
+    pid_class = get_pid_class (pid);
+
+  if (pid_class == ELFCLASS64 || (valid64 && ! valid32))
+    {
+      *sysinfo_ehdr = sysinfo_ehdr64;
+      dwfl->segment_align = segment_align64;
+    }
+  if (pid_class == ELFCLASS32 || (! valid64 && valid32))
+    {
+      *sysinfo_ehdr = sysinfo_ehdr32;
+      dwfl->segment_align = segment_align32;
+    }
+  return 0;
 }
 
 static int