[PATCH] Scan for Mach-O start address in LC_MAIN and properly check bfd_mach_o_scan_start_address's return value

David Albert davidbalbert@gmail.com
Tue Nov 20 19:27:00 GMT 2012


Hi there,

I sent this change to BFD to the gdb-patches list a couple of days ago
by mistake. I've included the patch as well as an updated summary
below. Joel Brobecker on the gdb-patches list suggested that I cc
Tristan Gingold, which I've done.

Last week there was a change committed to BFD that added support for
the new Mach-O load commands in OS X 10.8. This patch includes two
related changes.

1) On December 7th of last year, bfd_mach_o_scan_start_address was
changed to return a bfd_boolean, but no change was made to the code
that checks its return value in bfd_mach_o_scan. This means that
bfd_mach_o_scan always assumes that bfd_mach_o_scan_start_address
succeeds. This patch fixes that.

2) A program's start address can now be found in LC_MAIN in addition
to LC_THREAD and LC_UNIXTHREAD. I've updated
bfd_mach_o_scan_start_address to reflect this. I moved a call to
bfd_mach_o_flatten_sections to before bfd_mach_o_scan_start_address,
because the code that gets the start address from LC_MAIN relies on
nsects being set.

I've never submitted a patch to binutils (or any other GNU project)
before, so please let me know if there are things I can fix. I also
have a few notes:

- I used the latest version of Apple's GDB 6.3 fork (gdb-1822) as a
reference for some of my research. The changes I made are quite small
(most of the patch consists of placing a block of existing code inside
an if statement), but parts of the patch are similar to what I found
in Apple's fork. The code is simple enough that there are not really
many ways to solve the problem, but I want to make sure there aren't
issues with copyright or licensing.

- The one thing I'm not sure of is line 4047 which reads `if
(mdata->nsects > 1)`. This is the same check found in Apple's GDB
fork, however it's unclear why we'd need two or more sections, given
that we only use the first section to calculate the start address. I
thought about changing it to `if (mdata->nsects > 0)`, but I wanted to
get the opinion of someone who knows more about Mach-O than I do.

- Currently, if no appropriate load command is found,
bfd_mach_o_scan_start_address returns FALSE. I changed this to return
TRUE because there are valid Mach-O files that don't have start
addresses (shared libraries, etc). This bug wasn't causing problems
before because the return value of bfd_mach_o_scan_start_address was
always assumed to be true.

- I ran `make check` in the binutils tree both before and after
applying the patch and found no visible. I've included the results
below. You can find the full output here:
https://gist.github.com/4120289. Are there other tests I need to run?

		=== binutils Summary ===

# of expected passes		28
# of unexpected failures	4
# of unresolved testcases	3
# of untested testcases		6
# of unsupported tests		2
make[5]: *** [check-DEJAGNU] Error 1
make[4]: *** [check-am] Error 2
make[3]: *** [check-recursive] Error 1
make[2]: *** [check] Error 2
make[1]: *** [check-binutils] Error 2
make: *** [do-check] Error 2

Best,
-David

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 95f9aef..c8d2571 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,10 @@
+2012-11-20  David Albert  <davidbalbert@gmail.com>
+
+	* mach-o.c (bfd_mach_o_scan): Properly check return value of
+	bfd_mach_o_scan_start_address.
+	(bfd_mach_o_scan_start_address): Also search for start address in
+	BFD_MACH_O_LC_MAIN.
+
 2012-11-20  Alan Modra  <amodra@gmail.com>

 	* elf32-rx.c (rx_elf_print_private_bfd_data): Warning fix.
diff --git a/bfd/mach-o.c b/bfd/mach-o.c
index e44cf6d..4546540 100644
--- a/bfd/mach-o.c
+++ b/bfd/mach-o.c
@@ -3970,66 +3970,89 @@ bfd_mach_o_scan_start_address (bfd *abfd)
 {
   bfd_mach_o_data_struct *mdata = bfd_mach_o_get_data (abfd);
   bfd_mach_o_thread_command *cmd = NULL;
+  bfd_mach_o_main_command *main_cmd = NULL;
   unsigned long i;

   for (i = 0; i < mdata->header.ncmds; i++)
-    if ((mdata->commands[i].type == BFD_MACH_O_LC_THREAD) ||
-        (mdata->commands[i].type == BFD_MACH_O_LC_UNIXTHREAD))
-      {
-        cmd = &mdata->commands[i].command.thread;
-        break;
-      }
-
-  if (cmd == NULL)
-    return FALSE;
+    {
+      if ((mdata->commands[i].type == BFD_MACH_O_LC_THREAD) ||
+          (mdata->commands[i].type == BFD_MACH_O_LC_UNIXTHREAD))
+        {
+          cmd = &mdata->commands[i].command.thread;
+          break;
+        }
+      else if (mdata->commands[i].type == BFD_MACH_O_LC_MAIN)
+        {
+          main_cmd = &mdata->commands[i].command.main;
+          break;
+        }
+    }

-  /* FIXME: create a subtarget hook ?  */
-  for (i = 0; i < cmd->nflavours; i++)
+  if (cmd)
     {
-      if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_I386)
-	  && (cmd->flavours[i].flavour
-	      == (unsigned long) BFD_MACH_O_x86_THREAD_STATE32))
-	{
-	  unsigned char buf[4];
+      /* FIXME: create a subtarget hook ?  */
+      for (i = 0; i < cmd->nflavours; i++)
+        {
+          if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_I386)
+              && (cmd->flavours[i].flavour
+                  == (unsigned long) BFD_MACH_O_x86_THREAD_STATE32))
+            {
+              unsigned char buf[4];

-	  if (bfd_seek (abfd, cmd->flavours[i].offset + 40, SEEK_SET) != 0
-              || bfd_bread (buf, 4, abfd) != 4)
-	    return FALSE;
+              if (bfd_seek (abfd, cmd->flavours[i].offset + 40, SEEK_SET) != 0
+                  || bfd_bread (buf, 4, abfd) != 4)
+                return FALSE;

-	  abfd->start_address = bfd_h_get_32 (abfd, buf);
-	}
-      else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC)
-	       && (cmd->flavours[i].flavour == BFD_MACH_O_PPC_THREAD_STATE))
-	{
-	  unsigned char buf[4];
+              abfd->start_address = bfd_h_get_32 (abfd, buf);
+            }
+          else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC)
+                   && (cmd->flavours[i].flavour ==
BFD_MACH_O_PPC_THREAD_STATE))
+            {
+              unsigned char buf[4];

-	  if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
-              || bfd_bread (buf, 4, abfd) != 4)
-	    return FALSE;
+              if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
+                  || bfd_bread (buf, 4, abfd) != 4)
+                return FALSE;

-	  abfd->start_address = bfd_h_get_32 (abfd, buf);
-	}
-      else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC_64)
-               && (cmd->flavours[i].flavour == BFD_MACH_O_PPC_THREAD_STATE64))
-        {
-          unsigned char buf[8];
+              abfd->start_address = bfd_h_get_32 (abfd, buf);
+            }
+          else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_POWERPC_64)
+                   && (cmd->flavours[i].flavour ==
BFD_MACH_O_PPC_THREAD_STATE64))
+            {
+              unsigned char buf[8];

-          if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
-              || bfd_bread (buf, 8, abfd) != 8)
-            return FALSE;
+              if (bfd_seek (abfd, cmd->flavours[i].offset + 0, SEEK_SET) != 0
+                  || bfd_bread (buf, 8, abfd) != 8)
+                return FALSE;

-          abfd->start_address = bfd_h_get_64 (abfd, buf);
-        }
-      else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_X86_64)
-               && (cmd->flavours[i].flavour == BFD_MACH_O_x86_THREAD_STATE64))
-        {
-          unsigned char buf[8];
+              abfd->start_address = bfd_h_get_64 (abfd, buf);
+            }
+          else if ((mdata->header.cputype == BFD_MACH_O_CPU_TYPE_X86_64)
+                   && (cmd->flavours[i].flavour ==
BFD_MACH_O_x86_THREAD_STATE64))
+            {
+              unsigned char buf[8];

-          if (bfd_seek (abfd, cmd->flavours[i].offset + (16 * 8),
SEEK_SET) != 0
-              || bfd_bread (buf, 8, abfd) != 8)
-            return FALSE;
+              if (bfd_seek (abfd, cmd->flavours[i].offset + (16 * 8),
SEEK_SET) != 0
+                  || bfd_bread (buf, 8, abfd) != 8)
+                return FALSE;

-          abfd->start_address = bfd_h_get_64 (abfd, buf);
+              abfd->start_address = bfd_h_get_64 (abfd, buf);
+            }
+        }
+    }
+  else if (main_cmd)
+    {
+      bfd_vma text_address = 0;
+
+      if (mdata->nsects > 1)
+        {
+          bfd_mach_o_section *text_sect = mdata->sections[0];
+          if (text_sect)
+            {
+              text_address = text_sect->addr - text_sect->offset;
+              abfd->start_address = main_cmd->entryoff + text_address;
+              return TRUE;
+            }
         }
     }

@@ -4121,10 +4144,11 @@ bfd_mach_o_scan (bfd *abfd,
 	}
     }

-  if (bfd_mach_o_scan_start_address (abfd) < 0)
+  bfd_mach_o_flatten_sections (abfd);
+
+  if (!bfd_mach_o_scan_start_address (abfd))
     return FALSE;

-  bfd_mach_o_flatten_sections (abfd);
   return TRUE;
 }



More information about the Binutils mailing list