[1/2] KMI Whitelists: Add functionality to make whitelists additive
Commit Message
If multiple KMI whitelists are specified, either by passing
--kmi-whitelist several times or by having multiple whitelist sections
in the whitelist files, the generated suppressions are created as an
intersection of symbols. That is rather unusual, as whitelisting should
rather work additive. That means that the symbols (or expressions
thereof) defined across several sections or files shall be considered a
union of symbols. This patch combines the whitelist parsing to create
exactly one function_suppression and one variable suppression. A test
case has been added to ensure the functionality is working.
Please note, migrating the existing code to this new functionality is
done in a separate commit.
* include/abg-tools-utils.h
(gen_suppr_spec_from_kernel_abi_whitelists): New function.
* src/abg-tools-utils.cc
(gen_suppr_spec_from_kernel_abi_whitelists): Likewise.
* tests/.gitignore: Ignore new test executable.
* tests/Makefile.am: Add new test executable.
* tests/data/test-kmi-whitelist/whitelist-with-another-single-entry:
New test input file.
* tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry:
Likewise.
* tests/data/test-kmi-whitelist/whitelist-with-single-entry:
Likewise.
* tests/data/test-kmi-whitelist/whitelist-with-two-sections:
Likewise.
* tests/data/Makefile.am: Add above test material.
* tests/test-kmi-whitelist.cc: Add new test executable.
Signed-off-by: Matthias Maennich <maennich@google.com>
---
include/abg-tools-utils.h | 4 +
src/abg-tools-utils.cc | 113 ++++++++++++
tests/.gitignore | 1 +
tests/Makefile.am | 4 +
tests/data/Makefile.am | 7 +-
.../whitelist-with-another-single-entry | 2 +
.../whitelist-with-duplicate-entry | 3 +
.../whitelist-with-single-entry | 2 +
.../whitelist-with-two-sections | 5 +
tests/test-kmi-whitelist.cc | 162 ++++++++++++++++++
10 files changed, 302 insertions(+), 1 deletion(-)
create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-another-single-entry
create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry
create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-single-entry
create mode 100644 tests/data/test-kmi-whitelist/whitelist-with-two-sections
create mode 100644 tests/test-kmi-whitelist.cc
Comments
Matthias Maennich <maennich@google.com> a ?crit:
[...]
> If multiple KMI whitelists are specified, either by passing
> --kmi-whitelist several times or by having multiple whitelist sections
> in the whitelist files, the generated suppressions are created as an
> intersection of symbols. That is rather unusual, as whitelisting should
> rather work additive. That means that the symbols (or expressions
> thereof) defined across several sections or files shall be considered a
> union of symbols. This patch combines the whitelist parsing to create
> exactly one function_suppression and one variable suppression. A test
> case has been added to ensure the functionality is working.
The idea of the patch is great IMHO thank you for working on it.
I just have a few nits to raise, mostly stylistic, here and there.
[...]
> diff --git a/include/abg-tools-utils.h b/include/abg-tools-utils.h
> index a153af689740..ea44e4dd36aa 100644
> --- a/include/abg-tools-utils.h
> +++ b/include/abg-tools-utils.h
> @@ -90,6 +90,10 @@ bool
> gen_suppr_spec_from_kernel_abi_whitelist(const string& abi_whitelist_path,
> suppr::suppressions_type& s);
>
> +suppr::suppressions_type
> +gen_suppr_spec_from_kernel_abi_whitelists(
> + const std::vector<string>& abi_whitelist_paths);
> +
When breaking a "long" line that contains a fonction declaration, please
do it like this:
suppr::suppressions_type
gen_suppr_spec_from_kernel_abi_whitelists
(const std::vector<string>& abi_whitelist_paths);
or like this:
suppr::suppressions_type
gen_suppr_spec_from_kernel_abi_whitelists(int foo,
char bar);
This is for the sake of consistency with the existing code base,
because in the grand scheme of things, there is nothing wrong with the
way you wrote it.
[...]
> index 0495905253ed..dcc302546beb 100644
> --- a/src/abg-tools-utils.cc
> +++ b/src/abg-tools-utils.cc
[...]
> @@ -38,8 +38,10 @@
> #include <sys/time.h>
> #include <dirent.h>
> #include <time.h>
> +#include <algorithm>
> #include <cstdlib>
> #include <cstring>
> +#include <iterator>
> #include <ctype.h>
> #include <errno.h>
> #include <libgen.h>
> @@ -1870,6 +1872,117 @@ gen_suppr_spec_from_headers(const string& headers_root_dir)
> return result;
> }
>
> +/// Generate a suppression specification from kernel abi whitelist
> +/// files.
> +///
> +/// A kernel ABI whitelist file is an INI file that usually has only
> +/// one section. The name of the section is a string that ends up
> +/// with the sub-string "whitelist". For instance
> +/// RHEL7_x86_64_whitelist.
> +///
> +/// Then the content of the section is a set of function or variable
> +/// names, one name per line. Each function or variable name is the
> +/// name of a function or a variable whose changes are to be keept.
> +///
> +/// A whitelist file can have multiple sections (adhering to the naming
> +/// conventions and multiple files can be passed. The suppression that
> +/// is created takes all whitelist sections from all files into account.
> +/// Symbols (or expression of such) are deduplicated in the final
> +/// suppression expression.
> +///
> +/// This function reads the white lists and generates a
> +/// function_suppression_sptr and variable_suppression_sptr and returns
> +/// a vector containing those.
> +///
> +/// @param abi_whitelist_paths a vector of KMI whitelist paths
> +///
> +/// @return a vector or suppressions
> +suppressions_type
> +gen_suppr_spec_from_kernel_abi_whitelists(
> + const std::vector<std::string>& abi_whitelist_paths)
Likewise for breaking the long line:
gen_suppr_spec_from_kernel_abi_whitelists
(const std::vector<std::string>& abi_whitelist_paths)
> +{
> +
> + std::vector<std::string> whitelisted_names;
> + for (std::vector<std::string>::const_iterator path_iter
> + = abi_whitelist_paths.begin(),
The style used in the rest of the codebase for assignation statement is
to keep the '=' on the same line as the left-hand-side operand; i.e:
> + for (std::vector<std::string>::const_iterator path_iter > =
abi_whitelist_paths.begin(),
Also, when long lines are involved in the 'for' statement like that,
the convention used in the code base is to have the init, condition
and increment statements of the for loop start on their own line. So
that for loop would look like:
for (std::vector<std::string>::const_iterator path_iter =
abi_whitelist_paths.begin(),
path_end = abi_whitelist_paths.end();
path_iter != path_end;
++path_iter)
{
[...]
> +
> + abigail::ini::config whitelist;
> + if (!read_config(*path_iter, whitelist))
> + continue;
> +
> + const ini::config::sections_type& whitelist_sections
> + = whitelist.get_sections();
Likewise, please keep the '=' on the same line as the left-hand-side
operand.
> +
> + for (ini::config::sections_type::const_iterator section_iter
> + = whitelist_sections.begin(),
> + section_end = whitelist_sections.end();
> + section_iter != section_end; ++section_iter)
Likewise, loop style.
> + {
> + std::string section_name = (*section_iter)->get_name();
> + if (!string_ends_with(section_name, "whitelist"))
> + continue;
> + for (ini::config::properties_type::const_iterator prop_iter
> + = (*section_iter)->get_properties().begin(),
> + prop_end = (*section_iter)->get_properties().end();
> + prop_iter != prop_end; ++prop_iter)
Likewise, loop style.
> + {
> + if (const simple_property_sptr& prop
> + = is_simple_property(*prop_iter))
Likewise, equality.
> + if (prop->has_empty_value())
> + {
> + const std::string& name = prop->get_name();
> + if (!name.empty())
> + whitelisted_names.push_back(name);
> + }
> + }
> + }
> + }
> +
> + suppressions_type result;
> + if (!whitelisted_names.empty())
> + {
> + // Drop duplicates
> + whitelisted_names.erase(
> + std::unique(whitelisted_names.begin(), whitelisted_names.end()),
> + whitelisted_names.end());
The style of breaking the function call accross several lines would
be:
whitelisted_names.erase(std::unique(whitelisted_names.begin(),
whitelisted_names.end()),
whitelisted_names.end());
> +
> + // Build the regular expression to suppress non whitelisted symbols.
> + std::stringstream regex_ss;
I think, a more accurate way to put this would be to say something
like:
// Build a regular expression representing the union of all
// the function and variable names expressed in the white list.
> + regex_ss << "^";
> + std::copy(whitelisted_names.begin(), whitelisted_names.end(),
> + std::ostream_iterator<std::string>(regex_ss, "$|^"));
> + regex_ss.seekp(0, std::ios::end);
> + const std::string& regex = regex_ss.str().substr(
> + 0, static_cast<size_t>(regex_ss.tellp()) - 2);
Same line breaking style around function call expression.
[...]
>>+bool
>>+testTwoWhitelistsWithDuplicates()
>>+{
>>+ std::vector<std::string> abi_whitelist_paths;
>>+ abi_whitelist_paths.push_back(whitelist_with_duplicate_entry);
>>+ abi_whitelist_paths.push_back(whitelist_with_another_single_entry);
>>+ suppressions_type suppr
>>+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
>>+ return !suppr.empty()
>>+ && suppressions_are_consistent(suppr,
>>+ "^test_symbol$|^test_another_symbol$");
>
> Same here.
>
> I am not sending a v2 for this.
OK.
This is OK to commit to master with the comments above addressed.
Thanks a lot for working on this!
@@ -90,6 +90,10 @@ bool
gen_suppr_spec_from_kernel_abi_whitelist(const string& abi_whitelist_path,
suppr::suppressions_type& s);
+suppr::suppressions_type
+gen_suppr_spec_from_kernel_abi_whitelists(
+ const std::vector<string>& abi_whitelist_paths);
+
bool
get_vmlinux_path_from_kernel_dist(const string& from,
string& vmlinux_path);
@@ -38,8 +38,10 @@
#include <sys/time.h>
#include <dirent.h>
#include <time.h>
+#include <algorithm>
#include <cstdlib>
#include <cstring>
+#include <iterator>
#include <ctype.h>
#include <errno.h>
#include <libgen.h>
@@ -1870,6 +1872,117 @@ gen_suppr_spec_from_headers(const string& headers_root_dir)
return result;
}
+/// Generate a suppression specification from kernel abi whitelist
+/// files.
+///
+/// A kernel ABI whitelist file is an INI file that usually has only
+/// one section. The name of the section is a string that ends up
+/// with the sub-string "whitelist". For instance
+/// RHEL7_x86_64_whitelist.
+///
+/// Then the content of the section is a set of function or variable
+/// names, one name per line. Each function or variable name is the
+/// name of a function or a variable whose changes are to be keept.
+///
+/// A whitelist file can have multiple sections (adhering to the naming
+/// conventions and multiple files can be passed. The suppression that
+/// is created takes all whitelist sections from all files into account.
+/// Symbols (or expression of such) are deduplicated in the final
+/// suppression expression.
+///
+/// This function reads the white lists and generates a
+/// function_suppression_sptr and variable_suppression_sptr and returns
+/// a vector containing those.
+///
+/// @param abi_whitelist_paths a vector of KMI whitelist paths
+///
+/// @return a vector or suppressions
+suppressions_type
+gen_suppr_spec_from_kernel_abi_whitelists(
+ const std::vector<std::string>& abi_whitelist_paths)
+{
+
+ std::vector<std::string> whitelisted_names;
+ for (std::vector<std::string>::const_iterator path_iter
+ = abi_whitelist_paths.begin(),
+ path_end = abi_whitelist_paths.end();
+ path_iter != path_end; ++path_iter)
+ {
+
+ abigail::ini::config whitelist;
+ if (!read_config(*path_iter, whitelist))
+ continue;
+
+ const ini::config::sections_type& whitelist_sections
+ = whitelist.get_sections();
+
+ for (ini::config::sections_type::const_iterator section_iter
+ = whitelist_sections.begin(),
+ section_end = whitelist_sections.end();
+ section_iter != section_end; ++section_iter)
+ {
+ std::string section_name = (*section_iter)->get_name();
+ if (!string_ends_with(section_name, "whitelist"))
+ continue;
+ for (ini::config::properties_type::const_iterator prop_iter
+ = (*section_iter)->get_properties().begin(),
+ prop_end = (*section_iter)->get_properties().end();
+ prop_iter != prop_end; ++prop_iter)
+ {
+ if (const simple_property_sptr& prop
+ = is_simple_property(*prop_iter))
+ if (prop->has_empty_value())
+ {
+ const std::string& name = prop->get_name();
+ if (!name.empty())
+ whitelisted_names.push_back(name);
+ }
+ }
+ }
+ }
+
+ suppressions_type result;
+ if (!whitelisted_names.empty())
+ {
+ // Drop duplicates
+ whitelisted_names.erase(
+ std::unique(whitelisted_names.begin(), whitelisted_names.end()),
+ whitelisted_names.end());
+
+ // Build the regular expression to suppress non whitelisted symbols.
+ std::stringstream regex_ss;
+ regex_ss << "^";
+ std::copy(whitelisted_names.begin(), whitelisted_names.end(),
+ std::ostream_iterator<std::string>(regex_ss, "$|^"));
+ regex_ss.seekp(0, std::ios::end);
+ const std::string& regex = regex_ss.str().substr(
+ 0, static_cast<size_t>(regex_ss.tellp()) - 2);
+
+ // Build a suppression specification which *keeps* functions
+ // whose ELF symbols match the regular expression contained
+ // in function_names_regexp. This will also keep the ELF
+ // symbols (not designated by any debug info) whose names
+ // match this regexp.
+ function_suppression_sptr fn_suppr(new function_suppression);
+ fn_suppr->set_label("whitelist");
+ fn_suppr->set_symbol_name_not_regex_str(regex);
+ fn_suppr->set_drops_artifact_from_ir(true);
+ result.push_back(fn_suppr);
+
+ // Build a suppression specification which *keeps* variables
+ // whose ELF symbols match the regular expression contained
+ // in function_names_regexp. This will also keep the ELF
+ // symbols (not designated by any debug info) whose names
+ // match this regexp.
+ variable_suppression_sptr var_suppr(new variable_suppression);
+ var_suppr->set_label("whitelist");
+ var_suppr->set_symbol_name_not_regex_str(regex);
+ var_suppr->set_drops_artifact_from_ir(true);
+ result.push_back(var_suppr);
+ }
+ return result;
+}
+
/// Generate a suppression specification from kernel abi whitelist
/// files.
///
@@ -21,6 +21,7 @@ runtestdifffilter
runtestdiffpkg
runtestdiffsuppr
runtestini
+runtestkmiwhitelist
runtestlookupsyms
runtestreaddwarf
runtestreadwrite
@@ -45,6 +45,7 @@ runtestcorediff \
runtestabidiffexit \
runtestini \
runtesttoolsutils \
+runtestkmiwhitelist \
$(CXX11_TESTS)
if ENABLE_RUNNING_TESTS_WITH_PY3
@@ -136,6 +137,9 @@ runtestini_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
runtesttoolsutils_SOURCES = test-tools-utils.cc
runtesttoolsutils_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
+runtestkmiwhitelist_SOURCES = test-kmi-whitelist.cc
+runtestkmiwhitelist_LDADD = libtestutils.la $(top_builddir)/src/libabigail.la
+
runtestsvg_SOURCES=test-svg.cc
runtestsvg_LDADD=$(top_builddir)/src/libabigail.la
@@ -1590,4 +1590,9 @@ test-fedabipkgdiff/nss-util/nss-util-3.12.6-1.fc14.x86_64.rpm \
test-fedabipkgdiff/nss-util/nss-util-3.24.0-2.0.fc25.x86_64.rpm \
\
test-ini/test01-equal-in-property-string.abignore.expected \
-test-ini/test01-equal-in-property-string.abignore
+test-ini/test01-equal-in-property-string.abignore \
+\
+test-kmi-whitelist/whitelist-with-single-entry \
+test-kmi-whitelist/whitelist-with-another-single-entry \
+test-kmi-whitelist/whitelist-with-duplicate-entry \
+test-kmi-whitelist/whitelist-with-two-sections
new file mode 100644
@@ -0,0 +1,2 @@
+[abi_whitelist]
+ test_another_symbol
new file mode 100644
@@ -0,0 +1,3 @@
+[abi_whitelist]
+ test_symbol
+ test_symbol
new file mode 100644
@@ -0,0 +1,2 @@
+[abi_whitelist]
+ test_symbol
new file mode 100644
@@ -0,0 +1,5 @@
+[abi_whitelist]
+ test_symbol1
+
+[abi2_whitelist]
+ test_symbol2
new file mode 100644
@@ -0,0 +1,162 @@
+// -*- Mode: C++ -*-
+//
+// Copyright (C) 2020 Google, Inc.
+//
+// This file is part of the GNU Application Binary Interface Generic
+// Analysis and Instrumentation Library (libabigail). This library is
+// free software; you can redistribute it and/or modify it under the
+// terms of the GNU Lesser General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option) any
+// later version.
+
+// This library is distributed in the hope that it will be useful, but
+// WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+// General Lesser Public License for more details.
+
+// You should have received a copy of the GNU Lesser General Public
+// License along with this program; see the file COPYING-LGPLV3. If
+// not, see <http://www.gnu.org/licenses/>.
+
+// Author: Matthias Maennich
+
+/// @file
+///
+/// This program tests suppression generation from KMI whitelists.
+
+#include <string>
+
+#include "abg-fwd.h"
+#include "abg-suppression.h"
+#include "abg-tools-utils.h"
+#include "test-utils.h"
+
+using abigail::tools_utils::gen_suppr_spec_from_kernel_abi_whitelists;
+using abigail::suppr::suppression_sptr;
+using abigail::suppr::suppressions_type;
+using abigail::suppr::function_suppression_sptr;
+using abigail::suppr::variable_suppression_sptr;
+using abigail::suppr::is_function_suppression;
+using abigail::suppr::is_variable_suppression;
+
+const static std::string whitelist_with_single_entry
+ = std::string(abigail::tests::get_src_dir())
+ + "/tests/data/test-kmi-whitelist/whitelist-with-single-entry";
+
+const static std::string whitelist_with_another_single_entry
+ = std::string(abigail::tests::get_src_dir())
+ + "/tests/data/test-kmi-whitelist/whitelist-with-another-single-entry";
+
+const static std::string whitelist_with_two_sections
+ = std::string(abigail::tests::get_src_dir())
+ + "/tests/data/test-kmi-whitelist/whitelist-with-two-sections";
+
+const static std::string whitelist_with_duplicate_entry
+ = std::string(abigail::tests::get_src_dir())
+ + "/tests/data/test-kmi-whitelist/whitelist-with-duplicate-entry";
+
+bool
+suppressions_are_consistent(const suppressions_type& suppr,
+ const std::string& expr)
+{
+ if (suppr.size() != 2)
+ return false;
+
+ function_suppression_sptr left = is_function_suppression(suppr[0]);
+ variable_suppression_sptr right = is_variable_suppression(suppr[1]);
+
+ return // correctly casted
+ (left && right)
+ // same label
+ && (left->get_label() == right->get_label())
+ // same mode
+ && (left->get_drops_artifact_from_ir()
+ == right->get_drops_artifact_from_ir())
+ // same regex
+ && (left->get_symbol_name_not_regex_str()
+ == right->get_symbol_name_not_regex_str())
+ // regex as expected
+ && (left->get_symbol_name_not_regex_str() == expr);
+}
+
+bool
+testNoWhitelist()
+{
+ const std::vector<std::string> abi_whitelist_paths;
+ suppressions_type suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
+ return suppr.empty();
+}
+
+bool
+testSingleEntryWhitelist()
+{
+ std::vector<std::string> abi_whitelist_paths;
+ abi_whitelist_paths.push_back(whitelist_with_single_entry);
+ suppressions_type suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
+ return !suppr.empty() && suppressions_are_consistent(suppr, "^test_symbol$");
+}
+
+bool
+testWhitelistWithDuplicateEntries()
+{
+ std::vector<std::string> abi_whitelist_paths;
+ abi_whitelist_paths.push_back(whitelist_with_duplicate_entry);
+ suppressions_type suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
+ return !suppr.empty() && suppressions_are_consistent(suppr, "^test_symbol$");
+}
+
+bool
+testTwoWhitelists()
+{
+ std::vector<std::string> abi_whitelist_paths;
+ abi_whitelist_paths.push_back(whitelist_with_single_entry);
+ abi_whitelist_paths.push_back(whitelist_with_another_single_entry);
+ suppressions_type suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
+ return !suppr.empty()
+ && suppressions_are_consistent(suppr,
+ "^test_symbol$|^test_another_symbol$");
+}
+
+bool
+testTwoWhitelistsWithDuplicates()
+{
+ std::vector<std::string> abi_whitelist_paths;
+ abi_whitelist_paths.push_back(whitelist_with_duplicate_entry);
+ abi_whitelist_paths.push_back(whitelist_with_another_single_entry);
+ suppressions_type suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
+ return !suppr.empty()
+ && suppressions_are_consistent(suppr,
+ "^test_symbol$|^test_another_symbol$");
+}
+
+bool
+testWhitelistWithTwoSections()
+{
+ std::vector<std::string> abi_whitelist_paths;
+ abi_whitelist_paths.push_back(whitelist_with_two_sections);
+ suppressions_type suppr
+ = gen_suppr_spec_from_kernel_abi_whitelists(abi_whitelist_paths);
+ return !suppr.empty()
+ && suppressions_are_consistent(suppr,
+ "^test_symbol1$|^test_symbol2$");
+}
+
+int
+main(int, char*[])
+{
+ bool is_ok = true;
+
+ is_ok = is_ok && testNoWhitelist();
+ is_ok = is_ok && testSingleEntryWhitelist();
+ is_ok = is_ok && testWhitelistWithDuplicateEntries();
+ is_ok = is_ok && testTwoWhitelists();
+ is_ok = is_ok && testTwoWhitelistsWithDuplicates();
+ is_ok = is_ok && testWhitelistWithTwoSections();
+
+ return !is_ok;
+}