public inbox for frysk-cvs@sourceware.org
help / color / mirror / Atom feed
* [SCM]  master: Push image mapping management into UnwindH.hxx.
@ 2008-05-24 19:18 cagney
  0 siblings, 0 replies; only message in thread
From: cagney @ 2008-05-24 19:18 UTC (permalink / raw)
  To: frysk-cvs

The branch, master has been updated
       via  1936bb8c8e85e822981a1f8e6a6a3b189447ecbb (commit)
      from  d1d421d9f90c359b825250b02887eebe586be8f5 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email.

- Log -----------------------------------------------------------------
commit 1936bb8c8e85e822981a1f8e6a6a3b189447ecbb
Author: Andrew Cagney <cagney@redhat.com>
Date:   Sat May 24 15:03:43 2008 -0400

    Push image mapping management into UnwindH.hxx.
    
    Fyi, there's a pre-existing memory-free race bug here; this patch
    should help to better understand it.  Prior to this patch the sequence
    was:
    
    - frysk calls libunwind to do a step (a.k.a. unwind a frame)
    
    - libunwind's step code calls frysk to populate a proc-info describing
      how to unwind the current frame
    
    - frysk finds and then mmaps in the required image
    
    - frysk creates an ElfImage object from the mmap; during garbage
      collection ElfImage will un-map the mmapped region.
    
    - frysk passes the mmaped region to libunwind's dwarf code which 1)
      fills in the proc-info and 2) retains a reference to the mmaped
      image
    
    - frysk returns to libunwind's step code; this drops all references to
      the ElfImage
    
    - libunwind's dwarf step code uses the supplied unwind information
      including the mmaped region completing the step
    
    - libunwind's dwarf step code "releases" the proc-info locally (frysk
      doesn't know that the mmapped region is unused), and then returns
      control to frysk
    
    Notice how the ElfImage object is dropped before libunwind has
    finished with it - a Syste.gc() will destroy the ElfImage and hence
    the mmapped region before libunwind has finished with it; and that
    frysk doesn't know exactly when it has been finished with.
    
    This patch, consolidates some methods into UnwindH and in the process
    makes the race more obvious by blatently creating and then dropping
    the ElfImage object.
    
    frysk-sys/lib/unwind/ChangeLog
    2008-05-24  Andrew Cagney  <cagney@redhat.com>
    
     	* Unwind.java (createElfImageFromVDSO): Delete.
     	(createProcInfoFromElfImage): Delete.
     	(fillProcInfoFromVDSO): New.
     	(fillProcInfoFromElfImage): New.
     	* cni/UnwindH.hxx: Implement.
     	* ProcInfo.java: Use.
     	* ElfImage.java: Simplify.
     	* cni/ElfImage.cxx (ElfImage::mapElfImage): Delete.

-----------------------------------------------------------------------

Summary of changes:
 frysk-sys/lib/unwind/ChangeLog        |    9 ++
 frysk-sys/lib/unwind/ElfImage.java    |   67 +++-------
 frysk-sys/lib/unwind/ProcInfo.java    |   18 +--
 frysk-sys/lib/unwind/Unwind.java      |   26 ++--
 frysk-sys/lib/unwind/cni/ElfImage.cxx |   45 +------
 frysk-sys/lib/unwind/cni/UnwindH.hxx  |  235 ++++++++++++++++++++-------------
 6 files changed, 196 insertions(+), 204 deletions(-)

First 500 lines of diff:
diff --git a/frysk-sys/lib/unwind/ChangeLog b/frysk-sys/lib/unwind/ChangeLog
index e3409c3..4c93d20 100644
--- a/frysk-sys/lib/unwind/ChangeLog
+++ b/frysk-sys/lib/unwind/ChangeLog
@@ -1,5 +1,14 @@
 2008-05-24  Andrew Cagney  <cagney@redhat.com>
 
+ 	* Unwind.java (createElfImageFromVDSO): Delete.
+ 	(createProcInfoFromElfImage): Delete.
+ 	(fillProcInfoFromVDSO): New.
+ 	(fillProcInfoFromElfImage): New.
+ 	* cni/UnwindH.hxx: Implement.
+ 	* ProcInfo.java: Use.
+ 	* ElfImage.java: Simplify.
+ 	* cni/ElfImage.cxx (ElfImage::mapElfImage): Delete.
+ 	
 	* ProcInfo.java (fillNotAvailable): New.
 	* Unwind.java (fillProcInfoNotAvailable): New.
 	
diff --git a/frysk-sys/lib/unwind/ElfImage.java b/frysk-sys/lib/unwind/ElfImage.java
index e938c96..872c45d 100644
--- a/frysk-sys/lib/unwind/ElfImage.java
+++ b/frysk-sys/lib/unwind/ElfImage.java
@@ -1,6 +1,6 @@
 // This file is part of the program FRYSK.
 //
-// Copyright 2007, Red Hat Inc.
+// Copyright 2007, 2008, Red Hat Inc.
 //
 // FRYSK is free software; you can redistribute it and/or modify it
 // under the terms of the GNU General Public License as published by
@@ -37,53 +37,30 @@
 // version and license this file solely under the GPL without
 // exception.
 
-
 package lib.unwind;
 
-public class ElfImage
-{
-
-  final String name;
-
-  long elfImage;
-
-  long size;
-
-  long segbase;
-
-  long mapoff;
-
-  int ret = 0;
-  
-  public ElfImage (String name, long elfImage, long size, long segbase, long mapoff)
-  {
-    this.name = name;
-    this.elfImage = elfImage;
-    this.size = size;
-    this.segbase = segbase;
-    this.mapoff = mapoff;
-  }
-
-  public ElfImage (int ret)
-  {
-    this.name = "ERROR: " + Integer.toString(ret);
-    this.ret = ret;
-  }
-
-  public String toString ()
-  {
-    if (ret != 0)
-      return "Bad Elf Image, ret: " + ret;
-
-    return "Elf Image (" + name + "): 0x" + Long.toHexString(elfImage) + " size: " + size
-	   + " segbase: 0x" + Long.toHexString(segbase) + " mapoff: 0x"
-	   + Long.toHexString(mapoff);
-  }
+/**
+ * An object that, eventually, releases its underlying mmaped [elf]
+ * data during a garbage collect.
+ *
+ * Since the garbage collect will, typically, occure after libunwind's
+ * step has finished with the data this operation is safe.  However,
+ * there is the real possibility of the GC occuring early, unmapping
+ * the data causing an NPE.
+ */
 
-  public static native ElfImage mapElfImage (String elfImageName, long segbase,
-					     long hi, long mapoff);
+public class ElfImage {
 
-  // @Override
-  native protected void finalize () throws Throwable;
+    private final long image;
+    private final long size;
 
+    public ElfImage(long image, long size) {
+	this.image = image;
+	this.size = size;
+    }
+    // @Override
+    protected void finalize() {
+	unmap(image, size);
+    }
+    private static native void unmap(long image, long size);
 }
diff --git a/frysk-sys/lib/unwind/ProcInfo.java b/frysk-sys/lib/unwind/ProcInfo.java
index 99f85e0..a76f0db 100644
--- a/frysk-sys/lib/unwind/ProcInfo.java
+++ b/frysk-sys/lib/unwind/ProcInfo.java
@@ -63,13 +63,9 @@ public class ProcInfo {
 			    long addressLow, long addressHigh,
 			    long offset, long ip,
 			    boolean needUnwindInfo) {
-	ElfImage elfImage = unwinder.createElfImageFromVDSO(addressSpace,
-							    addressLow,
-							    addressHigh,
-							    offset);
-	return unwinder.createProcInfoFromElfImage(addressSpace, ip,
-						   needUnwindInfo, elfImage,
-						   this);
+	return unwinder.fillProcInfoFromVDSO(unwProcInfo, ip, needUnwindInfo,
+					     addressSpace,
+					     addressLow, addressHigh, offset);
     }
 
     public int fillFromElfImage(AddressSpace addressSpace,
@@ -77,11 +73,11 @@ public class ProcInfo {
 				long addressLow, long addressHigh,
 				long offset, long ip,
 				boolean needUnwindInfo) {
-	ElfImage elfImage = ElfImage.mapElfImage(name, addressLow, addressHigh,
+	return unwinder.fillProcInfoFromElfImage(unwProcInfo, ip,
+						 needUnwindInfo,
+						 addressSpace,
+						 name, addressLow, addressHigh,
 						 offset);
-	return unwinder.createProcInfoFromElfImage(addressSpace, ip,
-						   needUnwindInfo, elfImage,
-						   this);
     }
 
     public long getStartIP() {
diff --git a/frysk-sys/lib/unwind/Unwind.java b/frysk-sys/lib/unwind/Unwind.java
index db1d852..540888c 100644
--- a/frysk-sys/lib/unwind/Unwind.java
+++ b/frysk-sys/lib/unwind/Unwind.java
@@ -73,20 +73,18 @@ public abstract class Unwind {
     abstract int getContext(long context);
  
     abstract int fillProcInfoNotAvailable(long unwProcInfo);
-
-    // FIXME: shouldn't be public.
-    public abstract int createProcInfoFromElfImage(AddressSpace addressSpace,
-						   long ip, 
-						   boolean needUnwindInfo,
-						   ElfImage elfImage,
-						   ProcInfo procInfo);
- 
-    // FIXME: shouldn't be public.
-    public abstract ElfImage createElfImageFromVDSO(AddressSpace addressSpace, 
-						    long segbase, long hi, 
-						    long mapoff);
-
-
+    abstract int fillProcInfoFromElfImage(long unwProcInfo,
+ 					  long ip, boolean needUnwindInfo,
+ 					  AddressSpace addressSpace,
+ 					  String name,
+ 					  long addressLow, long addressHigh,
+ 					  long offset);
+    abstract int fillProcInfoFromVDSO(long unwProcInfo,
+ 				      long ip, boolean needUnwindInfo,
+ 				      AddressSpace addressSpace, 
+ 				      long addressLow, long addressHigh, 
+ 				      long offset);
+    
     abstract long getProcInfo(long unwCursor);
     abstract void destroyProcInfo(long unwProcInfo);
 
diff --git a/frysk-sys/lib/unwind/cni/ElfImage.cxx b/frysk-sys/lib/unwind/cni/ElfImage.cxx
index 3f7fdd3..68dc3c3 100644
--- a/frysk-sys/lib/unwind/cni/ElfImage.cxx
+++ b/frysk-sys/lib/unwind/cni/ElfImage.cxx
@@ -1,6 +1,6 @@
 // This file is part of the program FRYSK.
 //
-// Copyright 2007, Red Hat Inc.
+// Copyright 2007, 2008, Red Hat Inc.
 //
 // FRYSK is free software; you can redistribute it and/or modify it
 // under the terms of the GNU General Public License as published by
@@ -48,46 +48,7 @@
 
 #include "lib/unwind/ElfImage.h"
 
-lib::unwind::ElfImage*
-lib::unwind::ElfImage::mapElfImage(jstring elfImageName, jlong segbase, jlong hi,
-                                   jlong mapoff)
-{
-  struct stat stat;
-  void *image;		/* pointer to mmap'd image */
-  size_t size;		/* (file-) size of the image */
-  int nameSize = JvGetStringUTFLength(elfImageName);
-  char name[nameSize+1];
-  //JvGetStringUTFRegion(jstring STR, jsize START, jsize LEN, char* BUF);
-  JvGetStringUTFRegion(elfImageName, 0, nameSize, name);
-  name[nameSize] = '\0';
-
-  int fd = open (name, O_RDONLY);
-  if (fd < 0)
-    return new lib::unwind::ElfImage((jint) fd);
-
-  int ret = fstat (fd, &stat);
-  if (ret < 0)
-    {
-      close (fd);
-      return new lib::unwind::ElfImage((jint) ret);
-    }
-
-  size = stat.st_size;
-  image = mmap (NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
-
-  close (fd);
-  if (image == MAP_FAILED)
-    return new lib::unwind::ElfImage((jint) -1);
-
-  lib::unwind::ElfImage* elfImage
-    = new lib::unwind::ElfImage(elfImageName, (jlong) image, (jlong) size,
-                              (jlong) segbase, (jlong) mapoff);
-
-  return elfImage;
-}
-
 void
-lib::unwind::ElfImage::finalize()
-{
-  munmap((void *) elfImage, (size_t) size);
+lib::unwind::ElfImage::unmap(jlong image, jlong size) {
+  munmap((void *) image, (size_t) size);
 }
diff --git a/frysk-sys/lib/unwind/cni/UnwindH.hxx b/frysk-sys/lib/unwind/cni/UnwindH.hxx
index eaaf681..dda792f 100644
--- a/frysk-sys/lib/unwind/cni/UnwindH.hxx
+++ b/frysk-sys/lib/unwind/cni/UnwindH.hxx
@@ -44,6 +44,8 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <errno.h>
+#include <string.h>
 
 #include <libdwfl.h>
 #include LIBUNWIND_TARGET_H
@@ -583,11 +585,15 @@ get_eh_frame_hdr_addr(unw_proc_info_t *pi, char *image, size_t size,
   return hdr;
 }
 
-// The following is a local address space memory accessor used by
-// unw_get_unwind_table to access the eh_frame_hdr.  The arg pointer
-// is the base address, addr is the offset from the base address.
+/**
+ * The following are local memory image address space memory accessors
+ * used by unw_get_unwind_table to access the eh_frame_hdr.  The arg
+ * pointer is the base address, addr is the offset from the base
+ * address.
+ */
+
 static int 
-local_access_mem(unw_addr_space_t as, unw_word_t addr,
+image_access_mem(unw_addr_space_t as, unw_word_t addr,
 		 unw_word_t *val, int write, void *baseAddress) {
   // Writing is not supported
   if (write)
@@ -598,67 +604,75 @@ local_access_mem(unw_addr_space_t as, unw_word_t addr,
   return UNW_ESUCCESS;
 }
   
-static unw_accessors_t local_accessors
-= {NULL, NULL, NULL, local_access_mem, NULL, NULL, NULL, NULL};
-
-jint
-TARGET::createProcInfoFromElfImage(AddressSpace* addressSpace,
-				   jlong ip,
-				   jboolean needUnwindInfo,
-				   ElfImage* elfImage,
-				   ProcInfo* procInfo) {
-  if (elfImage == NULL || elfImage->ret != 0)
-    return -UNW_ENOINFO;
-
-  unw_proc_info_t* unwProcInfo = (unw_proc_info_t*) procInfo->unwProcInfo;
-  logf(fine, this, "Pre unw_get_unwind_table");
+static unw_accessors_t image_accessors = {
+    NULL, NULL, NULL, image_access_mem, NULL, NULL, NULL, NULL
+};
+
+static jint
+fillProcInfoFromImage(frysk::rsl::Log* fine,
+		      const char* name,
+		      jlong unwProcInfo,
+		      jlong ip,
+		      jboolean needUnwindInfo,
+		      void *image,
+		      long size,
+		      long segbase) {
+  unw_proc_info_t *procInfo = (::unw_proc_info_t *) unwProcInfo;
+  logf(fine, "fillProcInfoFromImage"
+       " %s unwProcInfo %lx, ip %lx, image %p, size %ld, segBase %lx",
+       name, (long) unwProcInfo, (long)ip, image, size, segbase);
   
   unw_word_t peh_vaddr = 0;
-  char *eh_table_hdr = get_eh_frame_hdr_addr(unwProcInfo,
-					     (char *) elfImage->elfImage,
-					     elfImage->size,
-					     elfImage->segbase,
+  char *eh_table_hdr = get_eh_frame_hdr_addr(procInfo,
+					     (char *) image,
+					     size, segbase,
 					     &peh_vaddr);
-
-  //jsize length = JvGetStringUTFLength (elfImage->name);
-  //char buffer[length + 1];
-  //JvGetStringUTFRegion (elfImage->name, 0, elfImage->name->length(), buffer);
-  //buffer[length] = '\0';
-  //fprintf(stderr, "%s: %p\n", buffer, eh_table_hdr);
-
-  if (eh_table_hdr == NULL)
+  if (eh_table_hdr == NULL) {
+    logf(fine, "get_eh_frame_hdr failed");
+    munmap(image, size);
     return -UNW_ENOINFO;
+  }
 
   int ret;
-  if (unwProcInfo->format == UNW_INFO_FORMAT_REMOTE_TABLE)
+  if (procInfo->format == UNW_INFO_FORMAT_REMOTE_TABLE)
     ret = unw_get_unwind_table((unw_word_t) ip,
-			       unwProcInfo,
+			       procInfo,
 			       (int) needUnwindInfo,
-			       &local_accessors,
+			       &image_accessors,
 			       // virtual address
 			       peh_vaddr,
 			       // address adjustment
 			       eh_table_hdr - peh_vaddr);
   else
     ret = unw_get_unwind_table((unw_word_t) ip,
-                               unwProcInfo,
+                               procInfo,
                                (int) needUnwindInfo,
-                               &local_accessors,
+                               &image_accessors,
                                // virtual address
                                0,
                                // address adjustment
                                eh_table_hdr);
-  
-  logf(fine, this, "Post unw_get_unwind_table %d", ret);
+
+  // FIXME: The munmap can't be done until after the step code has
+  // finished with the proc-info.  For moment ack around this by
+  // creating an ElfImage object that does the unmap in the finaliser.
+  // Of course this A) leaves many mmaped objects around; and B) has a
+  // race where the object could be unmapped before step has finished
+  // with it.
+  new ElfImage((jlong) image, (jlong) size);
+  // munmap(image, size);
+  logf(fine, "Post unw_get_unwind_table %d", ret);
   return ret;
 }
 
-ElfImage*
-TARGET::createElfImageFromVDSO(AddressSpace* addressSpace,
-			       jlong lowAddress, jlong highAddress,
-			       jlong offset) {
-  logf(fine, this,
-       "entering segbase: 0x%lx, highAddress: 0x%lx, mapoff: 0x%lx",
+jint
+TARGET::fillProcInfoFromVDSO(jlong unwProcInfo, jlong ip,
+			     jboolean needUnwindInfo,
+			     AddressSpace* addressSpace,
+			     jlong lowAddress, jlong highAddress,
+			     jlong offset) {
+  logf(fine, this, "fillProcInfoFromVDSO"
+       " segbase: 0x%lx, highAddress: 0x%lx, mapoff: 0x%lx",
        (unsigned long) lowAddress, (unsigned long) highAddress,
        (unsigned long) offset);
   void *image;
@@ -667,77 +681,114 @@ TARGET::createElfImageFromVDSO(AddressSpace* addressSpace,
   unw_accessors_t *a;
   unsigned long segbase = (unsigned long) lowAddress;
   unsigned long hi = (unsigned long) highAddress;
-  unsigned long mapoff = (unsigned long) offset;
 
   size = hi - segbase;
   if (size > MAX_VDSO_SIZE)
-    return new ElfImage((jint) -1);
-
+    return -UNW_ENOINFO;
   logf(fine, this, "checked size, 0x%lx", (unsigned long) size);
+
+  logf(fine, this, "checking access_mem");
   unw_addr_space_t as = (unw_addr_space_t) addressSpace->unwAddressSpace;
   a = unw_get_accessors (as);
   if (! a->access_mem)
-    return new ElfImage((jint) -1);
+    return -UNW_ENOINFO;
 
-  logf(fine, this, "checked access_mem");
-  /* Try to decide whether it's an ELF image before bringing it all
-     in.  */
+  // Try to decide whether it's an ELF image before bringing it all
+  // in.
+  logf(fine, this, "checking magic");
   if (size <= EI_CLASS || size <= sizeof (magic))
-    return new ElfImage((jint) -1);
-
-  if (sizeof (magic) >= SELFMAG)
-    {
-      int ret = (*a->access_mem) (as, (unw_word_t) segbase, &magic,
-                                  0, (void *) addressSpace);
-      if (ret < 0)
-        return new ElfImage((jint) ret);
-
-      if (memcmp (&magic, ELFMAG, SELFMAG) != 0)
-        return new ElfImage((jint) -1);
+    return -UNW_ENOINFO;
+  if (sizeof (magic) >= SELFMAG) {
+    int ret = (*a->access_mem) (as, (unw_word_t) segbase, &magic,
+				0, (void *) addressSpace);
+    if (ret < 0) {
+      logf(fine, this, "error accessing VDSO %d", ret);
+      return ret;
     }
+    if (memcmp (&magic, ELFMAG, SELFMAG) != 0) {
+      logf(fine, this, "VDSO has bad magic");
+      return -UNW_ENOINFO;
+    }
+  }
 
-  logf(fine, this, "checked magic size");
-
+  logf(fine, this, "mapping memory for image (using mmap, so can umaped)");
   image = mmap (0, size, PROT_READ | PROT_WRITE,
                 MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
   if (image == MAP_FAILED)
-    return new ElfImage((jint) -1);
-
-  logf(fine, this, "mapped elfImage");
-  if (sizeof (magic) >= SELFMAG)
-    {
-      *(unw_word_t *)image = magic;
-      hi = sizeof (magic);
-    }
-  else
-    hi = 0;
+    return -UNW_ENOINFO;
 
   logf(fine, this, "checked magic");
-  for (; hi < size; hi += sizeof (unw_word_t))
-    {
-      logf(finest, this, "Reading memory segbase: 0x%lx, image: %p, hi: 0x%lx, at: 0x%lx to location: %p",
-	   segbase , image , hi, segbase+hi, (char *) image + hi);
-      int ret = (*a->access_mem) (as, segbase + hi,(unw_word_t *) ((char *) image + hi),
-                                  0, (void *) addressSpace);
+  if (sizeof (magic) >= SELFMAG) {
+    *(unw_word_t *)image = magic;
+    hi = sizeof (magic);
+  } else {
+    hi = 0;
+  }
 
-      if (ret < 0)
-        {
-          munmap (image, size);
-          return new ElfImage((jint) ret);
-        }
+  logf(fine, this, "reading in VDSO");
+  for (; hi < size; hi += sizeof (unw_word_t)) {
+    logf(finest, this, "Reading memory segbase: 0x%lx, image: %p, hi: 0x%lx, at: 0x%lx to location: %p",
+	 segbase , image , hi, segbase+hi, (char *) image + hi);
+    int ret = (*a->access_mem) (as, segbase + hi,(unw_word_t *) ((char *) image + hi),
+				0, (void *) addressSpace);
+    if (ret < 0) {
+      logf(fine, this, "error reading vdso");
+      munmap (image, size);
+      return ret;
     }
+  }
 
-  logf(fine, this, "read memory into elf image");
+  return fillProcInfoFromImage(fine, "[vdso]", unwProcInfo, ip, needUnwindInfo,
+			       image, size, segbase);
+}
 
-  if (segbase == mapoff)
-    mapoff = 0;
+jint
+TARGET::fillProcInfoFromElfImage(jlong unwProcInfo, jlong ip,


hooks/post-receive
--
frysk system monitor/debugger


^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2008-05-24 19:18 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-05-24 19:18 [SCM] master: Push image mapping management into UnwindH.hxx cagney

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).