Re: [PATCH 1/2] Add is_executable to Dwfl_Module.

[Mark Wielaard <mjw@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 909 bytes --]

This is somewhat separate from the rest of your proposed patch, but...

> +  // !execlike && ET_EXEC is PIE.
> +  // execlike && !ET_EXEC is a static executable.
> +  if (mod != NULL && (execlike || ehdr.e32.e_type == ET_EXEC))
> +    mod->is_executable = true;

This made me look in the function and there are a couple more places
that just use ehdr.e32 fields without checking whether we are dealing
with a ELFCLASS32/Elf32_Ehdr or ELFCLASS64/Elf64_Ehdr (or worse use the
e32 variants even when we just checked it is Elf64). ehdr is a union of
those two types. It happens to work for the first few struct members
since they have the same underlying base type (but different type
names). But that makes it somewhat hard to review this code is correct.
You have to double check every time the field offsets really match up.

So I propose a cleanup like the attached first.

Cheers,

Mark

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-libdwfl-dwfl_segment_report_module-use-ei_class-ei_d.patch --]
[-- Type: text/x-patch, Size: 6430 bytes --]

From d25ff20853035a8aaf7bf517db8e5c082770e83f Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Wed, 10 Sep 2014 21:57:36 +0200
Subject: [PATCH] libdwfl: dwfl_segment_report_module use ei_class, ei_data and
 e_type.

To make it easier to review the code is using the correct fields of
the ehdr e32/e64 union extract ei_class, ei_data and e_type early and use
them directly.

Signed-off-by: Mark Wielaard <mjw@redhat.com>
---
 libdwfl/ChangeLog                    |  5 +++++
 libdwfl/dwfl_segment_report_module.c | 41 ++++++++++++++++++------------------
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 3de772e..bb039b5 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2014-09-10  Mark Wielaard  <mjw@redhat.com>
+
+	* dwfl_segment_report_module.c (dwfl_segment_report_module):
+	Extract ei_class, ei_data and e_type early and use the result.
+
 2014-08-28  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
 	* dwfl_module_getdwarf.c (find_offsets): Add parameter main_bias, use
diff --git a/libdwfl/dwfl_segment_report_module.c b/libdwfl/dwfl_segment_report_module.c
index dfecb51..572f15b 100644
--- a/libdwfl/dwfl_segment_report_module.c
+++ b/libdwfl/dwfl_segment_report_module.c
@@ -1,5 +1,5 @@
 /* Sniff out modules from ELF headers visible in memory segments.
-   Copyright (C) 2008-2012 Red Hat, Inc.
+   Copyright (C) 2008-2012, 2014 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -161,6 +161,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
   }
 
   /* Extract the information we need from the file header.  */
+  unsigned char ei_class;
+  unsigned char ei_data;
+  uint16_t e_type;
   union
   {
     Elf32_Ehdr e32;
@@ -183,13 +186,15 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
       .d_size = sizeof ehdr,
       .d_version = EV_CURRENT,
     };
-  switch (((const unsigned char *) buffer)[EI_CLASS])
+  ei_class = ((const unsigned char *) buffer)[EI_CLASS];
+  ei_data = ((const unsigned char *) buffer)[EI_DATA];
+  switch (ei_class)
     {
     case ELFCLASS32:
       xlatefrom.d_size = sizeof (Elf32_Ehdr);
-      if (elf32_xlatetom (&xlateto, &xlatefrom,
-			  ((const unsigned char *) buffer)[EI_DATA]) == NULL)
+      if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL)
 	return finish ();
+      e_type = ehdr.e32.e_type;
       phoff = ehdr.e32.e_phoff;
       phnum = ehdr.e32.e_phnum;
       phentsize = ehdr.e32.e_phentsize;
@@ -200,9 +205,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 
     case ELFCLASS64:
       xlatefrom.d_size = sizeof (Elf64_Ehdr);
-      if (elf64_xlatetom (&xlateto, &xlatefrom,
-			  ((const unsigned char *) buffer)[EI_DATA]) == NULL)
+      if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL)
 	return finish ();
+      e_type = ehdr.e64.e_type;
       phoff = ehdr.e64.e_phoff;
       phnum = ehdr.e64.e_phnum;
       phentsize = ehdr.e64.e_phentsize;
@@ -281,7 +286,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
     assert (sizeof (Elf32_Nhdr) == sizeof (Elf64_Nhdr));
 
     void *notes;
-    if (ehdr.e32.e_ident[EI_DATA] == MY_ELFDATA)
+    if (ei_data == MY_ELFDATA)
       notes = data;
     else
       {
@@ -397,10 +402,9 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 	break;
       }
   }
-  if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32)
+  if (ei_class == ELFCLASS32)
     {
-      if (elf32_xlatetom (&xlateto, &xlatefrom,
-			  ehdr.e32.e_ident[EI_DATA]) == NULL)
+      if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL)
 	found_bias = false;	/* Trigger error check.  */
       else
 	for (uint_fast16_t i = 0; i < phnum; ++i)
@@ -411,8 +415,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
     }
   else
     {
-      if (elf64_xlatetom (&xlateto, &xlatefrom,
-			  ehdr.e32.e_ident[EI_DATA]) == NULL)
+      if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) == NULL)
 	found_bias = false;	/* Trigger error check.  */
       else
 	for (uint_fast16_t i = 0; i < phnum; ++i)
@@ -568,7 +571,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
     return soname_stroff != 0 && dynstr_vaddr != 0 && dynstrsz != 0;
   }
 
-  const size_t dyn_entsize = (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32
+  const size_t dyn_entsize = (ei_class == ELFCLASS32
 			      ? sizeof (Elf32_Dyn) : sizeof (Elf64_Dyn));
   void *dyn_data = NULL;
   size_t dyn_data_size = 0;
@@ -587,18 +590,16 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
       xlateto.d_buf = &dyn;
       xlateto.d_size = sizeof dyn;
 
-      if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32)
+      if (ei_class == ELFCLASS32)
 	{
-	  if (elf32_xlatetom (&xlateto, &xlatefrom,
-			      ehdr.e32.e_ident[EI_DATA]) != NULL)
+	  if (elf32_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL)
 	    for (size_t i = 0; i < dyn_filesz / sizeof dyn.d32[0]; ++i)
 	      if (consider_dyn (dyn.d32[i].d_tag, dyn.d32[i].d_un.d_val))
 		break;
 	}
       else
 	{
-	  if (elf64_xlatetom (&xlateto, &xlatefrom,
-			      ehdr.e32.e_ident[EI_DATA]) != NULL)
+	  if (elf64_xlatetom (&xlateto, &xlatefrom, ei_data) != NULL)
 	    for (size_t i = 0; i < dyn_filesz / sizeof dyn.d64[0]; ++i)
 	      if (consider_dyn (dyn.d64[i].d_tag, dyn.d64[i].d_un.d_val))
 		break;
@@ -608,7 +609,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 
   /* We'll use the name passed in or a stupid default if not DT_SONAME.  */
   if (name == NULL)
-    name = ehdr.e32.e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]";
+    name = e_type == ET_EXEC ? "[exe]" : execlike ? "[pie]" : "[dso]";
 
   void *soname = NULL;
   size_t soname_size = 0;
@@ -710,7 +711,7 @@ dwfl_segment_report_module (Dwfl *dwfl, int ndx, const char *name,
 	      final_read (offset, vaddr + bias, filesz);
 	  }
 
-	  if (ehdr.e32.e_ident[EI_CLASS] == ELFCLASS32)
+	  if (ei_class == ELFCLASS32)
 	    for (uint_fast16_t i = 0; i < phnum; ++i)
 	      read_phdr (phdrs.p32[i].p_type, phdrs.p32[i].p_vaddr,
 			 phdrs.p32[i].p_offset, phdrs.p32[i].p_filesz);
-- 
1.8.3.1