Plug leak of diff_context_sptr after calling compute_diff

After the function compute_diff is invoked to compute the diff against
to corpora, the reference count of the diff_context_sptr that it takes
get incremented several times.

So the diff_context_sptr is not automatically released (freed) when
its scope is left.

The reason why the reference count of diff_context_sptr is incremented
is because several diff types (the diff type itself or types extending
it) actually holds a reference to the diff_context_sptr they get
created with.  So the lifetime of the diff_context_sptr becomes tied
to the lifetime the different diff objects in a hard-to-predict way.
It actually creates some cyclic references that, in this case, creates
a leak.  The diff_context_sptr never gets released.

This patch fixes that by making the diff types hold a weak reference
to the diff_context_sptr they are created with.

	* src/abg-comparison.cc (diff::priv::ctxt_): Make this a weak_ptr.
	(diff::priv::get_context): Convert the weak pointer to the context
	into a shared_ptr and return it.
	(diff::priv::is_filtered_out): Adjust to use
	diff::priv::get_context() to access the context.
	(diff::context): Likewise.
	(corpus_diff::priv::ctxt_): Make this a weak_ptr.
	(corpus_diff::priv::priv): Add a new overload that takes two
	corpora and a diff context.
	(corpus_diff::priv::get_context): Convert the weak pointer to the
	context into a shared_ptr and return it.
	(corpus_diff::priv::ensure_lookup_tables_populated): Adjust to use
	the new corpus_diff::priv::get_context to get the context.
	(variable_is_suppressed): Likewise.
	(corpus_diff::priv::{apply_suppressions_to_added_removed_fns_vars,
	apply_filters_and_compute_diff_stats, emit_diff_stats,
	categorize_redundant_changed_sub_nodes,
	clear_redundancy_categorization}): Likewise.
	(corpus_diff::{corpus_diff, context,
	apply_filters_and_suppressions_before_reporting}): Adjust.
	* tools/abipkgdiff.cc (compare): Make the overload that compares
	elf binaries take a diff context output parameter.  After the
	context is created by this function, it's return to the caller, so
	that it's life time is bound to the scope this function was
	called from.
	(pthread_routine_compare): Create a shared pointer to hold a
	reference on a diff context.  Pass that shared pointer by
	reference to the compare function that compares elf binaries.
	Rather than storing corpora in the reports_map, (as those corpora
	would then out-live the diff context and thus create memory
	corruption issues), emit the report directly into an ostringstream
	and store that stream in reports_map.
	(compare): In the overoad that compares packages, rather than
	trying to get corpora from the report_map, just emit the content
	of the ostringstream that is now there.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

src/abg-comparison.cc

@@ -1313,7 +1313,7 @@ struct diff::priv
   vector<diff_sptr>		children_;
   diff*			parent_;
   diff*			canonical_diff_;
-  diff_context_sptr		ctxt_;
+  diff_context_wptr		ctxt_;
   diff_category		local_category_;
   diff_category		category_;
   mutable bool			reported_once_;
@@ -1343,6 +1343,17 @@ public:
       currently_reporting_(currently_reporting)
   {}
 
+  /// Getter of the diff context associated with this diff.
+  ///
+  /// @returnt a smart pointer to the diff context.
+  diff_context_sptr
+  get_context() const
+  {
+    if (ctxt_.expired())
+      return diff_context_sptr();
+    return diff_context_sptr(ctxt_);
+  }
+
   /// Check if a given categorization of a diff node should make it be
   /// filtered out.
   ///
@@ -1350,7 +1361,8 @@ public:
   bool
   is_filtered_out(diff_category category)
   {
-    if (ctxt_->get_allowed_category() == EVERYTHING_CATEGORY)
+    diff_context_sptr ctxt = get_context();
+    if (ctxt->get_allowed_category() == EVERYTHING_CATEGORY)
     return false;
 
   /// We don't want to display nodes suppressed by a user-provided
@@ -1360,7 +1372,7 @@ public:
 
   // We don't want to display redundant diff nodes, when the user
   // asked to avoid seeing redundant diff nodes.
-  if (!ctxt_->show_redundant_changes()
+  if (!ctxt->show_redundant_changes()
       && (category & REDUNDANT_CATEGORY))
     return true;
 
@@ -1370,7 +1382,7 @@ public:
   // Ignore the REDUNDANT_CATEGORY bit when comparing allowed
   // categories and the current set of categories.
   return !((category & ~REDUNDANT_CATEGORY)
-	   & (ctxt_->get_allowed_category()
+	   & (ctxt->get_allowed_category()
 	      & ~REDUNDANT_CATEGORY));
   }
 };// end class diff::priv
@@ -1578,7 +1590,7 @@ diff::append_child_node(diff_sptr d)
 /// @return the context of the current diff.
 const diff_context_sptr
 diff::context() const
-{return priv_->ctxt_;}
+{return priv_->get_context();}
 
 /// Setter of the context of the current diff.
 ///
@@ -9610,9 +9622,9 @@ struct corpus_diff::priv
   bool					finished_;
   string				pretty_representation_;
   vector<diff_sptr>			children_;
-  diff_context_sptr			ctxt_;
   corpus_sptr				first_;
   corpus_sptr				second_;
+  diff_context_wptr			ctxt_;
   corpus_diff::diff_stats_sptr		diff_stats_;
   bool					sonames_equal_;
   bool					architectures_equal_;
@@ -9641,12 +9653,34 @@ struct corpus_diff::priv
   string_elf_symbol_map		deleted_unrefed_var_syms_;
   string_elf_symbol_map		suppressed_deleted_unrefed_var_syms_;
 
+  /// Default constructor of corpus_diff::priv.
   priv()
     : finished_(false),
       sonames_equal_(false),
       architectures_equal_(false)
   {}
 
+  /// Constructor of corpus_diff::priv.
+  ///
+  /// @param first the first corpus of this diff.
+  ///
+  /// @param second the second corpus of this diff.
+  ///
+  /// @param ctxt the context of the diff.
+  priv(corpus_sptr first,
+       corpus_sptr second,
+       diff_context_sptr ctxt)
+    : finished_(false),
+      first_(first),
+      second_(second),
+      ctxt_(ctxt),
+      sonames_equal_(false),
+      architectures_equal_(false)
+  {}
+
+  diff_context_sptr
+  get_context();
+
   bool
   lookup_tables_empty() const;
 
@@ -9701,6 +9735,17 @@ struct corpus_diff::priv
   maybe_dump_diff_tree();
 }; // end corpus::priv
 
+/// Getter of the context associated with this corpus.
+///
+/// @return a smart pointer to the context associate with the corpus.
+diff_context_sptr
+corpus_diff::priv::get_context()
+{
+  if (ctxt_.expired())
+    return diff_context_sptr();
+  return diff_context_sptr(ctxt_);
+}
+
 /// Tests if the lookup tables are empty.
 ///
 /// @return true if the lookup tables are empty, false otherwise.
@@ -9735,6 +9780,8 @@ corpus_diff::priv::ensure_lookup_tables_populated()
   if (!lookup_tables_empty())
     return;
 
+  diff_context_sptr ctxt = get_context();
+
   {
     edit_script& e = fns_edit_script_;
 
@@ -9778,7 +9825,7 @@ corpus_diff::priv::ensure_lookup_tables_populated()
 	      {
 		function_decl_sptr f(j->second, noop_deleter());
 		function_decl_sptr s(added_fn, noop_deleter());
-		function_decl_diff_sptr d = compute_diff(f, s, ctxt_);
+		function_decl_diff_sptr d = compute_diff(f, s, ctxt);
 		if (*j->second != *added_fn)
 		  changed_fns_map_[j->first] = d;
 		deleted_fns_.erase(j);
@@ -9882,7 +9929,7 @@ corpus_diff::priv::ensure_lookup_tables_populated()
 		  {
 		    var_decl_sptr f(j->second, noop_deleter());
 		    var_decl_sptr s(added_var, noop_deleter());
-		    changed_vars_map_[n] = compute_diff(f, s, ctxt_);
+		    changed_vars_map_[n] = compute_diff(f, s, ctxt);
 		  }
 		deleted_vars_.erase(j);
 	      }
@@ -10119,7 +10166,9 @@ variable_is_suppressed(const var_decl* var,
 void
 corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 {
-  const suppressions_type& suppressions = ctxt_->suppressions();
+  diff_context_sptr ctxt = get_context();
+
+  const suppressions_type& suppressions = ctxt->suppressions();
   for (suppressions_type::const_iterator i = suppressions.begin();
        i != suppressions.end();
        ++i)
@@ -10133,7 +10182,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (function_is_suppressed(e->second, fn_suppr,
 				       function_suppression::ADDED_FUNCTION_CHANGE_KIND,
-				       ctxt_))
+				       ctxt))
 	      suppressed_added_fns_[e->first] = e->second;
 
 	  // Deleted functions.
@@ -10142,7 +10191,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (function_is_suppressed(e->second, fn_suppr,
 				       function_suppression::DELETED_FUNCTION_CHANGE_KIND,
-				       ctxt_))
+				       ctxt))
 	      suppressed_deleted_fns_[e->first] = e->second;
 
 	  // Added function symbols not referenced by any debug info
@@ -10152,7 +10201,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (fn_suppr->suppresses_function_symbol(e->second,
 						     function_suppression::ADDED_FUNCTION_CHANGE_KIND,
-						     ctxt_))
+						     ctxt))
 	      suppressed_added_unrefed_fn_syms_[e->first] = e->second;
 
 	  // Removed function symbols not referenced by any debug info
@@ -10162,7 +10211,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (fn_suppr->suppresses_function_symbol(e->second,
 						     function_suppression::DELETED_FUNCTION_CHANGE_KIND,
-						     ctxt_))
+						     ctxt))
 	      suppressed_deleted_unrefed_fn_syms_[e->first] = e->second;
 	}
       // Added/Deleted variables
@@ -10175,7 +10224,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (variable_is_suppressed(e->second, var_suppr,
 				       variable_suppression::ADDED_VARIABLE_CHANGE_KIND,
-				       ctxt_))
+				       ctxt))
 	      suppressed_added_vars_[e->first] = e->second;
 
 	  //Deleted variables
@@ -10184,7 +10233,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (variable_is_suppressed(e->second, var_suppr,
 				       variable_suppression::DELETED_VARIABLE_CHANGE_KIND,
-				       ctxt_))
+				       ctxt))
 	      suppressed_deleted_vars_[e->first] = e->second;
 
 	  // Added variable symbols not referenced by any debug info
@@ -10194,7 +10243,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (var_suppr->suppresses_variable_symbol(e->second,
 						      variable_suppression::ADDED_VARIABLE_CHANGE_KIND,
-						      ctxt_))
+						      ctxt))
 	      suppressed_added_unrefed_var_syms_[e->first] = e->second;
 
 	  // Removed variable symbols not referenced by any debug info
@@ -10204,7 +10253,7 @@ corpus_diff::priv::apply_suppressions_to_added_removed_fns_vars()
 	       ++e)
 	    if (var_suppr->suppresses_variable_symbol(e->second,
 						      variable_suppression::DELETED_VARIABLE_CHANGE_KIND,
-						      ctxt_))
+						      ctxt))
 	      suppressed_deleted_unrefed_var_syms_[e->first] = e->second;
 	}
     }
@@ -10392,6 +10441,8 @@ corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat)
   stat.num_added_vars_filtered_out(suppressed_added_vars_.size());
   stat.num_vars_changed(changed_vars_map_.size());
 
+  diff_context_sptr ctxt = get_context();
+
   // Walk the changed function diff nodes to apply the categorization
   // filters.
   diff_sptr diff;
@@ -10401,7 +10452,7 @@ corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat)
        ++i)
     {
       diff_sptr diff = *i;
-      ctxt_->maybe_apply_filters(diff);
+      ctxt->maybe_apply_filters(diff);
     }
 
   // Walk the changed variable diff nodes to apply the categorization
@@ -10411,7 +10462,7 @@ corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat)
        ++i)
     {
       diff_sptr diff = *i;
-      ctxt_->maybe_apply_filters(diff);
+      ctxt->maybe_apply_filters(diff);
     }
 
   categorize_redundant_changed_sub_nodes();
@@ -10519,7 +10570,9 @@ corpus_diff::priv::emit_diff_stats(const diff_stats&	s,
 	<< " filtered out)";
   out << "\n";
 
-  if (ctxt_->show_symbols_unreferenced_by_debug_info()
+  diff_context_sptr ctxt = get_context();
+
+  if (ctxt->show_symbols_unreferenced_by_debug_info()
       && (s.num_func_syms_removed()
 	  || s.num_func_syms_added()
 	  || s.num_var_syms_removed()
@@ -10527,7 +10580,7 @@ corpus_diff::priv::emit_diff_stats(const diff_stats&	s,
     {
       // function symbols changes summary.
 
-      if (!ctxt_->show_added_symbols_unreferenced_by_debug_info()
+      if (!ctxt->show_added_symbols_unreferenced_by_debug_info()
 	  && s.num_func_syms_removed() == 0
 	  && s.num_func_syms_added() != 0)
 	// If the only unreferenced function symbol change is function
@@ -10555,7 +10608,7 @@ corpus_diff::priv::emit_diff_stats(const diff_stats&	s,
 
       // variable symbol changes summary.
 
-      if (!ctxt_->show_added_symbols_unreferenced_by_debug_info()
+      if (!ctxt->show_added_symbols_unreferenced_by_debug_info()
 	  && s.num_var_syms_removed() == 0
 	  && s.num_var_syms_added() != 0)
 	// If the only unreferenced variable symbol change is variable
@@ -10590,7 +10643,9 @@ corpus_diff::priv::categorize_redundant_changed_sub_nodes()
 {
   diff_sptr diff;
 
-  ctxt_->forget_visited_diffs();
+  diff_context_sptr ctxt = get_context();
+
+  ctxt->forget_visited_diffs();
   for (function_decl_diff_sptrs_type::const_iterator i =
 	 changed_fns_.begin();
        i!= changed_fns_.end();
@@ -10640,33 +10695,35 @@ corpus_diff::priv::clear_redundancy_categorization()
 void
 corpus_diff::priv::maybe_dump_diff_tree()
 {
-  if (!ctxt_->dump_diff_tree()
+  diff_context_sptr ctxt = get_context();
-      || ctxt_->error_output_stream() == 0)
+
+  if (!ctxt->dump_diff_tree()
+      || ctxt->error_output_stream() == 0)
     return;
 
   if (!changed_fns_.empty())
     {
-      *ctxt_->error_output_stream() << "changed functions diff tree: \n\n";
+      *ctxt->error_output_stream() << "changed functions diff tree: \n\n";
       for (function_decl_diff_sptrs_type::const_iterator i =
 	     changed_fns_.begin();
 	   i != changed_fns_.end();
 	   ++i)
 	{
 	  diff_sptr d = *i;
-	  print_diff_tree(d, *ctxt_->error_output_stream());
+	  print_diff_tree(d, *ctxt->error_output_stream());
 	}
     }
 
   if (!sorted_changed_vars_.empty())
     {
-      *ctxt_->error_output_stream() << "\nchanged variables diff tree: \n\n";
+      *ctxt->error_output_stream() << "\nchanged variables diff tree: \n\n";
       for (var_diff_sptrs_type::const_iterator i =
 	     sorted_changed_vars_.begin();
 	   i != sorted_changed_vars_.end();
 	   ++i)
 	{
 	  diff_sptr d = *i;
-	  print_diff_tree(d, *ctxt_->error_output_stream());
+	  print_diff_tree(d, *ctxt->error_output_stream());
 	}
     }
 }
@@ -10696,12 +10753,8 @@ corpus_diff::chain_into_hierarchy()
 corpus_diff::corpus_diff(corpus_sptr first,
 			 corpus_sptr second,
 			 diff_context_sptr ctxt)
-  : priv_(new priv)
+  : priv_(new priv(first, second, ctxt))
-{
+{}
-  priv_->first_ = first;
-  priv_->second_ = second;
-  priv_->ctxt_ = ctxt;
-}
 
 /// Finish building the current instance of @ref corpus_diff.
 void
@@ -10728,9 +10781,13 @@ const vector<diff_sptr>&
 corpus_diff::children_nodes() const
 {return priv_->children_;}
 
-/// Append a new child node to the vector of childre nodes for the
+/// Append a new child node to the vector of children nodes for the
 /// current instance of @ref corpus_diff node.
 ///
+/// Note that the vector of children nodes for the current instance of
+/// @ref corpus_diff node must remain sorted, using
+/// diff_less_than_functor.
+///
 /// @param d the new child node.  Note that the life time of the
 /// object held by @p d will thus equal the life time of the current
 /// instance of @ref corpus_diff.
@@ -10879,7 +10936,7 @@ corpus_diff::added_unrefed_variable_symbols() const
 /// @return the diff context for this diff.
 const diff_context_sptr
 corpus_diff::context() const
-{return priv_->ctxt_;}
+{return priv_->get_context();}
 
 /// @return the pretty representation for the current instance of @ref
 /// corpus_diff
@@ -11260,7 +11317,7 @@ corpus_diff::apply_filters_and_suppressions_before_reporting()
     return *priv_->diff_stats_;
 
   apply_suppressions(this);
-  priv_->diff_stats_.reset(new diff_stats(priv_->ctxt_));
+  priv_->diff_stats_.reset(new diff_stats(context()));
   priv_->apply_filters_and_compute_diff_stats(*priv_->diff_stats_);
   return *priv_->diff_stats_;
 }

tools/abipkgdiff.cc

@@ -86,6 +86,7 @@ using std::string;
 using std::ostream;
 using std::vector;
 using std::map;
+using std::ostringstream;
 using std::tr1::shared_ptr;
 using abigail::tools_utils::maybe_get_symlink_target_file_path;
 using abigail::tools_utils::emit_prefix;
@@ -120,7 +121,7 @@ static bool verbose;
 static pthread_key_t elf_file_paths_tls_key;
 
 /// A convenience typedef for a map of corpus diffs
-typedef map<string, corpus_diff_sptr> corpora_report_map;
+typedef map<string, shared_ptr<ostringstream> > corpora_report_map;
 /// This map is used to gather the computed diffs of ELF pairs
 static corpora_report_map reports_map;
 
@@ -933,7 +934,8 @@ compare(const elf_file& elf1,
 	const string&	debug_dir2,
 	const options&	opts,
 	abigail::ir::environment_sptr	&env,
-	corpus_diff_sptr	&diff)
+	corpus_diff_sptr	&diff,
+	diff_context_sptr	&ctxt)
 {
   char *di_dir1 = (char*) debug_dir1.c_str(),
 	*di_dir2 = (char*) debug_dir2.c_str();
@@ -949,7 +951,7 @@ compare(const elf_file& elf1,
   abigail::dwarf_reader::status c1_status = abigail::dwarf_reader::STATUS_OK,
     c2_status = abigail::dwarf_reader::STATUS_OK;
 
-  diff_context_sptr ctxt(new diff_context);
+  ctxt.reset(new diff_context);
   set_diff_context_from_opts(ctxt, opts);
   suppressions_type& supprs = ctxt->suppressions();
   bool files_suppressed = (file_is_suppressed(elf1.path, supprs)
@@ -1077,6 +1079,7 @@ pthread_routine_compare(vector<compare_args_sptr> *args)
   abidiff_status s, status = abigail::tools_utils::ABIDIFF_OK;
   compare_args_sptr a;
   corpus_diff_sptr diff;
+  diff_context_sptr ctxt;
 
   while (true)
     {
@@ -1096,14 +1099,17 @@ pthread_routine_compare(vector<compare_args_sptr> *args)
       abigail::ir::environment_sptr env(new abigail::ir::environment);
       status |= s = compare(a->elf1, a->debug_dir1,
 			    a->elf2, a->debug_dir2,
-			    a->opts, env, diff);
+			    a->opts, env, diff, ctxt);
 
       const string key = a->elf1.path;
       if ((s & abigail::tools_utils::ABIDIFF_ABI_CHANGE)
 	  || (verbose && diff->has_changes()))
 	{
+	  const string prefix = "  ";
+	  shared_ptr<ostringstream> out(new ostringstream);
+	  diff->report(*out, prefix);
 	  pthread_mutex_lock(&map_lock);
-	  reports_map[key] = diff;
+	  reports_map[key] = out;
 	  // We need to keep the environment around, until the corpus is
 	  // report()-ed.
 	  env_map[diff] = env;
@@ -1112,7 +1118,7 @@ pthread_routine_compare(vector<compare_args_sptr> *args)
       else
 	{
 	  pthread_mutex_lock(&map_lock);
-	  reports_map[key] = corpus_diff_sptr();
+	  reports_map[key] = shared_ptr<ostringstream>();
 	  pthread_mutex_unlock(&map_lock);
 	}
     }
@@ -1492,7 +1498,7 @@ compare(package&	first_package,
 		break;
 	      else
 		{
-		  // Diff created -> report it.
+		  // Report exist, emit it.
 		  string name = it->second->name;
 		  diff.changed_binaries.push_back(name);
 		  const string prefix = "  ";
@@ -1500,14 +1506,13 @@ compare(package&	first_package,
 		  cout << "================ changes of '"
 		       << name
 		       << "'===============\n";
-		  d->second->report(cout, prefix);
+		  cout << d->second->str();
 
 		  cout << "================ end of changes of '"
 		       << name
 		       << "'===============\n\n";
 
 		  pthread_mutex_lock(&map_lock);
-		  env_map.erase(d->second);
 		  reports_map.erase(d);
 		  pthread_mutex_unlock(&map_lock);