From patchwork Wed Jan 1 00:00:00 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 38992 From: dodji@seketeli.org (Dodji Seketeli) Date: Wed, 01 Jan 2020 00:00:00 -0000 Subject: [PATCH] suppression: Better handle soname/filename properties evaluation Message-ID: <865zfzzzr4.fsf@seketeli.org> At the moment, we don't make any difference between these two cases: 1/ A suppression specification has no soname or file name related property. 2/ A suppression specification has a soname or file name related property that doesn't match the soname or file name of a given corpus. In both cases 1/ and 2/ libabigail currently assumes that the suppression specification does not match the given corpus we are looking at. This can be wrong, especially if we are in the case 1/ and if the suppression does have other properties that should make it match the corpus. This patch fixes this issue. * include/abg-suppression.h (suppression_base::has_{soname,file_name}_related_property): Add new member functions. * src/abg-dwarf-reader.cc (read_context::suppression_can_match): Fix the logic to make a difference between the case where the suppression doesn't have any soname/filename property and the case where the suppression does have a soname/filename property that does not match the current binary. * src/abg-reader.cc (read_context::suppression_can_match): Likewise. * src/abg-suppression-priv.h (suppression_base::priv::matches_soname): If the suppression does not have any soname related property then it doesn't match the soname we are looking at. (suppression_base::priv::matches_binary_name): If the suppression does not have any filename related property then it doesn't match the filename we are looking at. * src/abg-suppression.cc (suppression_base::has_{soname,file_name}_related_property): Define new member functions. (sonames_of_binaries_match): If the suppression does not have any soname related property then it doesn't match the corpora of the diff we are looking at. (names_of_binaries_match): If the suppression does not have any filename related property then it doesn't match the corpora of the diff we are looking at. (type_suppression::suppresses_type): Fix the logic to make a difference between the case where the suppression doesn't have any soname/filename property and the case where the suppression does have a soname/filename property that does not match the current binary. (function_suppression::suppresses_{function, function_symbol}): Likewise. (variable_suppression::suppresses_{variable, variable_symbol}): Likewise. (file_suppression::suppresses_file): Likewise. Signed-off-by: Dodji Seketeli --- include/abg-suppression.h | 6 +++ src/abg-dwarf-reader.cc | 30 +++++++++--- src/abg-reader.cc | 20 ++++++-- src/abg-suppression-priv.h | 58 +++++++++++++++++++---- src/abg-suppression.cc | 113 ++++++++++++++++++++++++++++++++++----------- 5 files changed, 180 insertions(+), 47 deletions(-) diff --git a/include/abg-suppression.h b/include/abg-suppression.h index 02f3fc4..05709d6 100644 --- a/include/abg-suppression.h +++ b/include/abg-suppression.h @@ -97,6 +97,9 @@ public: const string& get_file_name_not_regex_str() const; + bool + has_file_name_related_property() const; + void set_soname_regex_str(const string& regexp); @@ -109,6 +112,9 @@ public: const string& get_soname_not_regex_str() const; + bool + has_soname_related_property() const; + virtual bool suppresses_diff(const diff*) const = 0; diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc index 436f610..0697c7a 100644 --- a/src/abg-dwarf-reader.cc +++ b/src/abg-dwarf-reader.cc @@ -8820,17 +8820,35 @@ public: /// Tests if a suppression specification can match ABI artifacts /// coming from the binary being analyzed. /// - /// This tests if the suppression matches the soname of and binary - /// name of the ELF binary being analyzed. + /// This tests if the suppression can match the soname of and binary + /// name of the ELF binary being analyzed. More precisely, if there + /// are any soname or file name property in the suppression and if + /// those do *NOT* match the current binary, then the function + /// returns false. /// /// @param s the suppression specification to consider. + /// + /// @return true iff either there are no soname/filename related + /// property on the suppression, or if none of the soname/filename + /// properties of the suppression match the current binary. bool suppression_can_match(const suppr::suppression_base& s) const { - if (s.priv_->matches_soname(dt_soname()) - && s.priv_->matches_binary_name(elf_path())) - return true; - return false; + if (!s.priv_->matches_soname(dt_soname())) + if (s.has_soname_related_property()) + // The suppression has some SONAME related properties, but + // none of them match the SONAME of the current binary. So + // the suppression cannot match the current binary. + return false; + + if (!s.priv_->matches_binary_name(elf_path())) + if (s.has_file_name_related_property()) + // The suppression has some file_name related properties, but + // none of them match the file name of the current binary. So + // the suppression cannot match the current binary. + return false; + + return true; } /// Test whether if a given function suppression matches a function diff --git a/src/abg-reader.cc b/src/abg-reader.cc index 30b9abc..7c8a56a 100644 --- a/src/abg-reader.cc +++ b/src/abg-reader.cc @@ -948,10 +948,22 @@ public: suppression_can_match(const suppr::suppression_base& s) const { corpus_sptr corp = get_corpus(); - if (s.priv_->matches_soname(corp->get_soname()) - && s.priv_->matches_binary_name(corp->get_path())) - return true; - return false; + + if (!s.priv_->matches_soname(corp->get_soname())) + if (s.has_soname_related_property()) + // The suppression has some SONAME related properties, but + // none of them match the SONAME of the current binary. So + // the suppression cannot match the current binary. + return false; + + if (!s.priv_->matches_binary_name(corp->get_path())) + if (s.has_file_name_related_property()) + // The suppression has some file_name related properties, but + // none of them match the file name of the current binary. So + // the suppression cannot match the current binary. + return false; + + return true; } /// Test whether if a given function suppression matches a function diff --git a/src/abg-suppression-priv.h b/src/abg-suppression-priv.h index 449814c..d872168 100644 --- a/src/abg-suppression-priv.h +++ b/src/abg-suppression-priv.h @@ -179,32 +179,72 @@ public: return soname_not_regex_; } + /// Test if the current suppression matches a given SONAME. + /// + /// @param soname the SONAME to consider. + /// + /// @return true iff the suppression matches the SONAME denoted by + /// @p soname. + /// + /// Note that if the suppression contains no property that is + /// related to SONAMEs, the function returns false. bool matches_soname(const string& soname) const { + bool has_regexp = false; if (sptr_utils::regex_t_sptr regexp = get_soname_regex()) - if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) != 0) - return false; + { + has_regexp = true; + if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) != 0) + return false; + } if (sptr_utils::regex_t_sptr regexp = get_soname_not_regex()) - if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) == 0) + { + has_regexp = true; + if (regexec(regexp.get(), soname.c_str(), 0, NULL, 0) == 0) + return false; + } + + if (!has_regexp) return false; return true; } + /// Test if the current suppression matches the full file path to a + /// given binary. + /// + /// @param binary_name the full path to the binary. + /// + /// @return true iff the suppression matches the path denoted by @p + /// binary_name. + /// + /// Note that if the suppression contains no property that is + /// related to file name, the function returns false. bool matches_binary_name(const string& binary_name) const { + bool has_regexp = false; + if (sptr_utils::regex_t_sptr regexp = get_file_name_regex()) - if (regexec(regexp.get(), binary_name.c_str(), - 0, NULL, 0) != 0) - return false; + { + has_regexp = true; + if (regexec(regexp.get(), binary_name.c_str(), + 0, NULL, 0) != 0) + return false; + } if (sptr_utils::regex_t_sptr regexp = get_file_name_not_regex()) - if (regexec(regexp.get(), binary_name.c_str(), - 0, NULL, 0) == 0) - return false; + { + has_regexp = true; + if (regexec(regexp.get(), binary_name.c_str(), + 0, NULL, 0) == 0) + return false; + } + + if (!has_regexp) + return false; return true; } diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc index 05d3858..663749d 100644 --- a/src/abg-suppression.cc +++ b/src/abg-suppression.cc @@ -184,6 +184,18 @@ const string& suppression_base::get_file_name_not_regex_str() const {return priv_->file_name_not_regex_str_;} +/// Test if the current suppression has a property related to file +/// name. +/// +/// @return true iff the current suppression has either a +/// file_name_regex or a file_name_not_regex property. +bool +suppression_base::has_file_name_related_property() const +{ + return (!(get_file_name_regex_str().empty() + && get_file_name_not_regex_str().empty())); +} + /// Setter of the "soname_regex_str property of the current instance /// of @ref suppression_base. /// @@ -234,6 +246,17 @@ const string& suppression_base::get_soname_not_regex_str() const {return priv_->soname_not_regex_str_;} +/// Test if the current suppression has a property related to SONAMEs. +/// +/// @return true iff the current suppression has either a soname_regex +/// or a soname_not_regex property. +bool +suppression_base::has_soname_related_property() const +{ + return (!(get_soname_regex_str().empty() + && get_soname_not_regex_str().empty())); +} + /// Check if the SONAMEs of the two binaries being compared match the /// content of the properties "soname_regexp" and "soname_not_regexp" /// of the current suppression specification. @@ -254,6 +277,9 @@ sonames_of_binaries_match(const suppression_base& suppr, string first_soname = ctxt.get_corpus_diff()->first_corpus()->get_soname(), second_soname = ctxt.get_corpus_diff()->second_corpus()->get_soname(); + if (!suppr.has_soname_related_property()) + return false; + if (!suppr.priv_->matches_soname(first_soname) && !suppr.priv_->matches_soname(second_soname)) return false; @@ -281,6 +307,9 @@ names_of_binaries_match(const suppression_base& suppr, string first_binary_path = ctxt.get_corpus_diff()->first_corpus()->get_path(), second_binary_path = ctxt.get_corpus_diff()->second_corpus()->get_path(); + if (!suppr.has_file_name_related_property()) + return false; + if (!suppr.priv_->matches_binary_name(first_binary_path) && !suppr.priv_->matches_binary_name(second_binary_path)) return false; @@ -850,17 +879,18 @@ type_suppression::suppresses_type(const type_base_sptr& type, { if (ctxt) { - // Check if the names of the binaries match + // Check if the names of the binaries match the suppression if (!names_of_binaries_match(*this, *ctxt)) - return false; + if (has_file_name_related_property()) + return false; - // Check if the sonames of the binaries match + // Check if the sonames of the binaries match the suppression if (!sonames_of_binaries_match(*this, *ctxt)) - return false; + if (has_soname_related_property()) + return false; } return suppresses_type(type); - } /// Test if an instance of @ref type_suppression matches a given type. @@ -2419,16 +2449,19 @@ function_suppression::suppresses_function(const function_decl* fn, if (!(get_change_kind() & k)) return false; - // Check if the name and soname of the binaries match + // Check if the name and soname of the binaries match the current + // suppr spec if (ctxt) { - // Check if the name of the binaries match + // Check if the name of the binaries match the current suppr spec if (!names_of_binaries_match(*this, *ctxt)) - return false; + if (has_file_name_related_property()) + return false; - // Check if the soname of the binaries match + // Check if the soname of the binaries match the current suppr spec if (!sonames_of_binaries_match(*this, *ctxt)) - return false; + if (has_soname_related_property()) + return false; } string fname = fn->get_qualified_name(); @@ -2752,16 +2785,21 @@ function_suppression::suppresses_function_symbol(const elf_symbol* sym, ABG_ASSERT(k & function_suppression::ADDED_FUNCTION_CHANGE_KIND || k & function_suppression::DELETED_FUNCTION_CHANGE_KIND); - // Check if the name and soname of the binaries match + // Check if the name and soname of the binaries match the current + // suppr spect if (ctxt) { - // Check if the name of the binaries match + // Check if the name of the binaries match the current + // suppr spect if (!names_of_binaries_match(*this, *ctxt)) - return false; + if (has_file_name_related_property()) + return false; - // Check if the soname of the binaries match + // Check if the soname of the binaries match the current + // suppr spect if (!sonames_of_binaries_match(*this, *ctxt)) - return false; + if (has_soname_related_property()) + return false; } string sym_name = sym->get_name(), sym_version = sym->get_version().str(); @@ -3715,13 +3753,17 @@ variable_suppression::suppresses_variable(const var_decl* var, // Check if the name and soname of the binaries match if (ctxt) { - // Check if the name of the binaries match + // Check if the name of the binaries match the current + // suppr spec if (!names_of_binaries_match(*this, *ctxt)) - return false; + if (has_file_name_related_property()) + return false; - // Check if the soname of the binaries match + // Check if the soname of the binaries match the current suppr + // spec if (!sonames_of_binaries_match(*this, *ctxt)) - return false; + if (has_soname_related_property()) + return false; } string var_name = var->get_qualified_name(); @@ -3871,16 +3913,20 @@ variable_suppression::suppresses_variable_symbol(const elf_symbol* sym, ABG_ASSERT(k & ADDED_VARIABLE_CHANGE_KIND || k & DELETED_VARIABLE_CHANGE_KIND); - // Check if the name and soname of the binaries match + // Check if the name and soname of the binaries match the current + // suppr spec. if (ctxt) { - // Check if the name of the binaries match + // Check if the name of the binaries match the current suppr + // spec if (!names_of_binaries_match(*this, *ctxt)) - return false; + if (has_file_name_related_property()) + return false; - // Check if the soname of the binaries match + // Check if the soname of the binaries match the current suppr spec if (!sonames_of_binaries_match(*this, *ctxt)) - return false; + if (has_soname_related_property()) + return false; } string sym_name = sym->get_name(), sym_version = sym->get_version().str(); @@ -4227,15 +4273,26 @@ file_suppression::suppresses_file(const string& file_path) string fname; tools_utils::base_name(file_path, fname); + bool has_regexp = false; + if (sptr_utils::regex_t_sptr regexp = suppression_base::priv_->get_file_name_regex()) - if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) != 0) - return false; + { + has_regexp = true; + if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) != 0) + return false; + } if (sptr_utils::regex_t_sptr regexp = suppression_base::priv_->get_file_name_not_regex()) - if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) == 0) - return false; + { + has_regexp = true; + if (regexec(regexp.get(), fname.c_str(), 0, NULL, 0) == 0) + return false; + } + + if (!has_regexp) + return false; return true; }