From patchwork Fri Apr 24 12:58:04 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 39174 From: dodji@seketeli.org (Dodji Seketeli) Date: Fri, 24 Apr 2020 14:58:04 +0200 Subject: [PATCH] abipkgdiff: Fix race condition while using private types suppr specs Message-ID: <87r1wdavmr.fsf@seketeli.org> 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 Applied to master. --- tools/abipkgdiff.cc | 60 ++++++++++++++------------------------------- 1 file changed, 18 insertions(+), 42 deletions(-) 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 debug_info_packages_; package_sptr devel_package_; package_sptr kabi_whitelist_package_; - suppressions_type private_types_suppressions_; vector elf_file_paths_; set 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); }