From: Frank Ch. Eigler <fche@elastic.org>
Date: Tue, 2 Nov 2010 03:05:56 +0000 (-0400)
Subject: fix memory double-free in module_cache
X-Git-Tag: release-1.4~76
X-Git-Url: https://sourceware.org/git/?a=commitdiff_plain;h=435f53a7d750fa27c9a73de4716a62c9363b5a91;p=systemtap.git

fix memory double-free in module_cache

The kmodule.stp test case could evoke a double-free of module-info
structs, since these were shared pointers, and not managed by GC.
Oh yeah, what GC, in the year 2010.

* dwflpp.cxx (dwflpp dtor): Don't delete shared mod_info ptr.
  (module_cache dtor): New function.
* dwflpp.h: Declare.
* tapsets.cxx (delete_session_module_cache): New function.
  (several::build_no_more): Call it.
  (dwarf_build_no_more): Use delete_map<> template.
---

diff --git a/dwflpp.cxx b/dwflpp.cxx
index 8377fa012..3945770de 100644
--- a/dwflpp.cxx
+++ b/dwflpp.cxx
@@ -105,7 +105,15 @@ dwflpp::~dwflpp()
   delete_map(cu_die_parent_cache);
 
   dwfl_ptr.reset();
-  delete mod_info;
+  // NB: don't "delete mod_info;", as that may be shared
+  // between dwflpp instances, and are stored in
+  // session.module_cache[] anyway.
+}
+
+
+module_cache::~module_cache ()
+{
+  delete_map(cache);
 }
 
 
diff --git a/dwflpp.h b/dwflpp.h
index 2e0e118d3..f4b571cc1 100644
--- a/dwflpp.h
+++ b/dwflpp.h
@@ -121,6 +121,7 @@ module_cache
   bool dwarf_collected;
 
   module_cache() : paths_collected(false), dwarf_collected(false) {}
+  ~module_cache();
 };
 
 
diff --git a/tapsets.cxx b/tapsets.cxx
index b7bce7dbc..123a3fb60 100644
--- a/tapsets.cxx
+++ b/tapsets.cxx
@@ -683,6 +683,9 @@ struct dwarf_query : public base_query
 };
 
 
+static void delete_session_module_cache (systemtap_session& s); // forward decl
+
+
 struct dwarf_builder: public derived_probe_builder
 {
   map <string,dwflpp*> kern_dw; /* NB: key string could be a wildcard */
@@ -708,30 +711,14 @@ struct dwarf_builder: public derived_probe_builder
   /* NB: not virtual, so can be called from dtor too: */
   void dwarf_build_no_more (bool verbose)
   {
-    for (map<string,dwflpp*>::iterator udi = kern_dw.begin();
-         udi != kern_dw.end();
-         udi ++)
-      {
-        if (verbose)
-          clog << "dwarf_builder releasing kernel dwflpp " << udi->first << endl;
-        delete udi->second;
-      }
-    kern_dw.erase (kern_dw.begin(), kern_dw.end());
-
-    for (map<string,dwflpp*>::iterator udi = user_dw.begin();
-         udi != user_dw.end();
-         udi ++)
-      {
-        if (verbose)
-          clog << "dwarf_builder releasing user dwflpp " << udi->first << endl;
-        delete udi->second;
-      }
-    user_dw.erase (user_dw.begin(), user_dw.end());
+    delete_map(kern_dw);
+    delete_map(user_dw);
   }
 
   void build_no_more (systemtap_session &s)
   {
     dwarf_build_no_more (s.verbose > 3);
+    delete_session_module_cache (s);
   }
 
   ~dwarf_builder()
@@ -1954,6 +1941,22 @@ query_module (Dwfl_Module *mod,
 }
 
 
+// This would more naturally fit into elaborate.cxx:semantic_pass_symbols,
+// but the needed declaration for module_cache is not available there.
+// Nor for that matter in session.cxx.  Only in this CU is that field ever
+// set (in query_module() above), so we clean it up here too.
+static void
+delete_session_module_cache (systemtap_session& s)
+{
+  if (s.module_cache) {
+    if (s.verbose > 3)
+      clog << "deleting module_cache" << endl;
+    delete s.module_cache;
+    s.module_cache = 0;
+  }
+}
+
+
 struct dwarf_var_expanding_visitor: public var_expanding_visitor
 {
   dwarf_query & q;
@@ -8268,6 +8271,8 @@ public:
       clog << "tracepoint_builder releasing dwflpp" << endl;
     delete dw;
     dw = NULL;
+
+    delete_session_module_cache (s);
   }
 
   void build(systemtap_session& s,