diff mbox series

abipkgdiff: Fix race condition while using private types suppr specs

Message ID 87r1wdavmr.fsf@seketeli.org
State Committed
Headers show
Series abipkgdiff: Fix race condition while using private types suppr specs | expand

Commit Message

Dodji Seketeli April 24, 2020, 12:58 p.m. UTC
Hello,

When comparing two packages, abipkgdiff potentially spawns one thread
per pair of binaries to compare.  The suppression specifications
generated for the private types of the package are shared among those
threads.  As some internals of the suppression specifications are
constructed lazily, that can lead do some data races when more than
two threads access one of those internals.

One effective way to handle that is to make sure each thread has its
own copy of suppression specifications.


	* tools/abipkgdiff.cc (compare_args::private_types_suppr{1,2}):
	Make these data member *not* be a reference anymore.
	(maybe_create_private_types_suppressions): Rename this into ...
	(create_private_types_suppressions): ... this.  Also, make this
	function return a copy of the vector of suppression specifications
	for private types created.
	(compare_prepared_userspace_packages): Use the new
	create_private_types_suppressions to create a copy of private
	types suppression specifications, rather than sharing it from
	package::private_types_suppressions_.
	(extract_package_and_map_its_content): Adjust to avoid creating
	the shared suppression specifications for private types.
	(package::private_types_suppressions_): Remove this data member
	that was holding the shared suppressions for private types.
	(package::private_types_suppressions): Remove these accessors.

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

Applied to master.

---
 tools/abipkgdiff.cc | 60 ++++++++++++++-------------------------------
 1 file changed, 18 insertions(+), 42 deletions(-)
diff mbox series

Patch

diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index 611b4d07..7d946e72 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -346,7 +346,6 @@  private:
   vector<package_sptr>			debug_info_packages_;
   package_sptr				devel_package_;
   package_sptr				kabi_whitelist_package_;
-  suppressions_type			private_types_suppressions_;
   vector<string>			elf_file_paths_;
   set<string>				public_dso_sonames_;
 
@@ -554,24 +553,6 @@  public:
   kabi_whitelist_package(const package_sptr& p)
   {kabi_whitelist_package_ = p;}
 
-  /// Getter of the specifications to suppress change reports about
-  /// private types.
-  ///
-  /// @return the vector of specifications to suppress change reports
-  /// about private types.
-  const suppressions_type&
-  private_types_suppressions() const
-  {return private_types_suppressions_;}
-
-  /// Getter of the specifications to suppress change reports about
-  /// private types.
-  ///
-  /// @return the vector of specifications to suppress change reports
-  /// about private types.
-  suppressions_type&
-  private_types_suppressions()
-  {return private_types_suppressions_;}
-
   /// Getter of the path to the elf files of the package.
   ///
   /// @return the path tothe elf files of the package.
@@ -727,10 +708,10 @@  struct compare_args
 {
   const elf_file		elf1;
   const string&		debug_dir1;
-  const suppressions_type&	private_types_suppr1;
+  const suppressions_type	private_types_suppr1;
   const elf_file		elf2;
   const string&		debug_dir2;
-  const suppressions_type&	private_types_suppr2;
+  const suppressions_type	private_types_suppr2;
   const options&		opts;
 
   /// Constructor for compare_args, which is used to pass
@@ -1496,28 +1477,25 @@  compare(const elf_file& elf1,
 /// out types that are deemed private to the package we are looking
 /// at.
 ///
-/// If the function succeeds, the generated private type suppressions
-/// are available by invoking the
-/// package::private_types_suppressions() accessor of the @p pkg
-/// parameter.
+/// If the function succeeds, it returns a non-empty vector of
+/// suppression specifications.
 ///
 /// @param pkg the main package we are looking at.
 ///
 /// @param opts the options of the current program.
 ///
-/// @return true iff suppression specifications were generated for
-/// types private to the package.
-static bool
-maybe_create_private_types_suppressions(package& pkg, const options &opts)
+/// @return a vector of suppression_sptr.  If no suppressions
+/// specification were constructed, the returned vector is empty.
+static suppressions_type
+create_private_types_suppressions(const package& pkg, const options &opts)
 {
-  if (!pkg.private_types_suppressions().empty())
-    return false;
+  suppressions_type supprs;
 
   package_sptr devel_pkg = pkg.devel_package();
   if (!devel_pkg
       || !file_exists(devel_pkg->extracted_dir_path())
       || !is_dir(devel_pkg->extracted_dir_path()))
-    return false;
+    return supprs;
 
   string headers_path = devel_pkg->extracted_dir_path();
   if (devel_pkg->type() == abigail::tools_utils::FILE_TYPE_RPM
@@ -1527,7 +1505,7 @@  maybe_create_private_types_suppressions(package& pkg, const options &opts)
     headers_path += "/usr/include";
 
   if (!is_dir(headers_path))
-    return false;
+    return supprs;
 
   suppression_sptr suppr =
     gen_suppr_spec_from_headers(headers_path);
@@ -1536,10 +1514,10 @@  maybe_create_private_types_suppressions(package& pkg, const options &opts)
     {
       if (opts.drop_private_types)
 	suppr->set_drops_artifact_from_ir(true);
-      pkg.private_types_suppressions().push_back(suppr);
+      supprs.push_back(suppr);
     }
 
-  return bool(suppr);
+  return supprs;
 }
 
 /// If the user wants to avoid comparing DSOs that are private to this
@@ -2197,10 +2175,7 @@  extract_package_and_map_its_content(const package_sptr &pkg, options &opts)
     is_ok = create_maps_of_package_content(*pkg, opts);
 
   if (is_ok)
-    {
-      maybe_create_private_types_suppressions(*pkg, opts);
-      maybe_handle_kabi_whitelist_pkg(*pkg, opts);
-    }
+    maybe_handle_kabi_whitelist_pkg(*pkg, opts);
 
   return is_ok;
 }
@@ -2397,11 +2372,12 @@  compare_prepared_userspace_packages(package& first_package,
 	      compare_args_sptr args
 		(new compare_args(*it->second,
 				  debug_dir1,
-				  first_package.private_types_suppressions(),
+				  create_private_types_suppressions
+				  (first_package, opts),
 				  *iter->second,
 				  debug_dir2,
-				  second_package.private_types_suppressions(),
-				  opts));
+				  create_private_types_suppressions
+				  (second_package, opts), opts));
 	      compare_task_sptr t(new compare_task(args));
 	      compare_tasks.push_back(t);
 	    }