diff mbox series

[08/16] abipkgdiff: Avoid comparing binaries that are outside of the package

Message ID 87il8mf6cl.fsf@redhat.com
State New
Headers
Series Fixing various issues found while working on PR30309 |

Commit Message

Dodji Seketeli Sept. 7, 2023, 1:41 p.m. UTC 
  Hello,

Some symlinks in some RPMs resolve to binaries outside of the package. In
those cases, avoid considering them.

	* src/abg-tools-utils.cc (maybe_get_symlink_target_file_path): Do
	not require that the file path points to a symlink.  A file path
	can point to a file that is not a symlink and yet the whole path
	can be in parent directory that is a symlink.  In this case,
	realpath will correctly resolve to the correct target file.
	* tools/abipkgdiff.cc (maybe_update_package_content): A path to a
	binary that is not inside the RPM (because a symlink resolved to a
	file outside of the RPM) should not be added to the set of
	binaries to be analyzed.
	* tests/data/test-diff-pkg/symlink-dir-test1-report1.txt: Add new
	test file.
	* tests/data/test-diff-pkg-ctf/symlink-dir-test1-report1.txt:
	Likewise.
	* tests/data/Makefile.am: Add new test file to source
	distribution.
	* tests/test-diff-pkg.cc (in_out_specs): for the
	data/test-diff-pkg/symlink-dir-test1/dir{1,2}/symlinks test, the
	root dir of the package is
	data/test-diff-pkg/symlink-dir-test1/dir{1,2}.  Use that to test
	that the symlinks are properly handled.  Also, use the
	data/test-diff-pkg/symlink-dir-test1/dir{1,2}/symlinks as a root
	of an alternative package for which the symlinks resolve outside
	the package, under
	data/test-diff-pkg/symlink-dir-test1/dir{1,2}/targets.  In this
	later case, the symlinked files should be ignored in the
	comparison.  Likewise for
	data/test-diff-pkg-ctf/symlink-dir-test1/dir1/symlinks.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to master.
---
 src/abg-tools-utils.cc                        |  3 --
 tests/data/Makefile.am                        |  2 ++
 .../symlink-dir-test1-report1.txt             |  0
 .../symlink-dir-test1-report1.txt             |  0
 tests/test-diff-pkg.cc                        | 32 ++++++++++++++++---
 tools/abipkgdiff.cc                           | 25 ++++++++++++---
 6 files changed, 50 insertions(+), 12 deletions(-)
 create mode 100644 tests/data/test-diff-pkg-ctf/symlink-dir-test1-report1.txt
 create mode 100644 tests/data/test-diff-pkg/symlink-dir-test1-report1.txt
diff mbox series

Patch

diff --git a/src/abg-tools-utils.cc b/src/abg-tools-utils.cc
index e7155846..08d50347 100644
--- a/src/abg-tools-utils.cc
+++ b/src/abg-tools-utils.cc
@@ -723,9 +723,6 @@  maybe_get_symlink_target_file_path(const string& file_path,
   if (!get_stat(file_path, &st))
     return false;
 
-  if (!S_ISLNK(st.st_mode))
-    return false;
-
   char *link_target_path = realpath(file_path.c_str(), NULL);
   if (!link_target_path)
     return false;
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 03361fab..b3434da5 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -2055,6 +2055,7 @@  test-diff-pkg/dirpkg-3-report-1.txt \
 test-diff-pkg/dirpkg-3-report-2.txt \
 test-diff-pkg/dirpkg-3.suppr \
 test-diff-pkg/symlink-dir-test1-report0.txt \
+test-diff-pkg/symlink-dir-test1-report1.txt \
 test-diff-pkg/symlink-dir-test1/dir1/symlinks/foo.o \
 test-diff-pkg/symlink-dir-test1/dir1/symlinks/libfoo.so \
 test-diff-pkg/symlink-dir-test1/dir1/targets/foo.c \
@@ -2300,6 +2301,7 @@  test-diff-pkg-ctf/cracklib-2.9.6-15-ol8u0.x86_64.rpm \
 test-diff-pkg-ctf/isl-debuginfo-0.16.1-6.x86_64.rpm	\
 test-diff-pkg-ctf/tarpkg-1-dir1.tar.gz \
 test-diff-pkg-ctf/symlink-dir-test1-report0.txt \
+test-diff-pkg-ctf/symlink-dir-test1-report1.txt \
 \
 test-fedabipkgdiff/dbus-glib-0.104-3.fc23.x86_64.rpm \
 test-fedabipkgdiff/dbus-glib-debuginfo-0.104-3.fc23.x86_64.rpm \
diff --git a/tests/data/test-diff-pkg-ctf/symlink-dir-test1-report1.txt b/tests/data/test-diff-pkg-ctf/symlink-dir-test1-report1.txt
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/data/test-diff-pkg/symlink-dir-test1-report1.txt b/tests/data/test-diff-pkg/symlink-dir-test1-report1.txt
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/test-diff-pkg.cc b/tests/test-diff-pkg.cc
index 99dc381d..fbfa50d3 100644
--- a/tests/test-diff-pkg.cc
+++ b/tests/test-diff-pkg.cc
@@ -149,6 +149,18 @@  static InOutSpec in_out_specs[] =
     "data/test-diff-pkg/dirpkg-3-report-2.txt",
     "output/test-diff-pkg/dirpkg-3-report-2.txt"
   },
+  {
+    "data/test-diff-pkg/symlink-dir-test1/dir1",
+    "data/test-diff-pkg/symlink-dir-test1/dir2",
+    "--no-default-suppression ",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "data/test-diff-pkg/symlink-dir-test1-report0.txt",
+    "output/test-diff-pkg/symlink-dir-test1-report0.txt"
+  },
   {
     "data/test-diff-pkg/symlink-dir-test1/dir1/symlinks",
     "data/test-diff-pkg/symlink-dir-test1/dir2/symlinks",
@@ -158,8 +170,8 @@  static InOutSpec in_out_specs[] =
     "",
     "",
     "",
-    "data/test-diff-pkg/symlink-dir-test1-report0.txt ",
-    "output/test-diff-pkg/symlink-dir-test1-report0.txt "
+    "data/test-diff-pkg/symlink-dir-test1-report1.txt",
+    "output/test-diff-pkg/symlink-dir-test1-report1.txt"
   },
 #if WITH_TAR
   {
@@ -864,6 +876,18 @@  static InOutSpec in_out_specs[] =
     "data/test-diff-pkg-ctf/dirpkg-3-report-2.txt",
     "output/test-diff-pkg-ctf/dirpkg-3-report-2.txt"
   },
+  {
+    "data/test-diff-pkg-ctf/symlink-dir-test1/dir1",
+    "data/test-diff-pkg-ctf/symlink-dir-test1/dir2",
+    "--ctf --no-default-suppression ",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "data/test-diff-pkg-ctf/symlink-dir-test1-report0.txt",
+    "output/test-diff-pkg-ctf/symlink-dir-test1-report0.txt"
+  },
   {
     "data/test-diff-pkg-ctf/symlink-dir-test1/dir1/symlinks",
     "data/test-diff-pkg-ctf/symlink-dir-test1/dir2/symlinks",
@@ -873,8 +897,8 @@  static InOutSpec in_out_specs[] =
     "",
     "",
     "",
-    "data/test-diff-pkg-ctf/symlink-dir-test1-report0.txt ",
-    "output/test-diff-pkg-ctf/symlink-dir-test1-report0.txt "
+    "data/test-diff-pkg-ctf/symlink-dir-test1-report1.txt",
+    "output/test-diff-pkg-ctf/symlink-dir-test1-report1.txt"
   },
 #if WITH_TAR
   {
diff --git a/tools/abipkgdiff.cc b/tools/abipkgdiff.cc
index 5ab5c22c..3a601075 100644
--- a/tools/abipkgdiff.cc
+++ b/tools/abipkgdiff.cc
@@ -2323,14 +2323,20 @@  typedef shared_ptr<self_compare_task> self_compare_task_sptr;
 /// function only looks for a file name which name is the same as the
 /// value of this parameter.
 ///
+/// @param parent_dir_name the name of the directory that the file
+/// name denoted by @p entry should belong to.  If it doesn't (because
+/// it's a symlink that resolves to a file outside of that directory)
+/// then the vector of paths of is not updated.
+///
 /// @param paths out parameter.  This is the set of meaningful paths
 /// of the current directory tree being analyzed.  These paths are
 /// those that are going to be involved in ABI comparison.
 static void
-maybe_update_package_content(const FTSENT *entry,
-			     options &opts,
-			     const string& file_name_to_look_for,
-			     unordered_set<string>& paths)
+maybe_update_package_content(const FTSENT*		entry,
+			     options&			opts,
+			     const string&		file_name_to_look_for,
+			     const string&		parent_dir_name,
+			     unordered_set<string>&	paths)
 {
   if (entry == NULL
       || (entry->fts_info != FTS_F && entry->fts_info != FTS_SL)
@@ -2340,6 +2346,15 @@  maybe_update_package_content(const FTSENT *entry,
 
   string path = entry->fts_path;
   maybe_get_symlink_target_file_path(path, path);
+  string parent_dir = parent_dir_name;
+  maybe_get_symlink_target_file_path(parent_dir, parent_dir);
+
+  if (!parent_dir_name.empty())
+    {
+      string s;
+      if (!string_suffix(path, parent_dir, s))
+	return;
+    }
 
   if (!file_name_to_look_for.empty())
     {
@@ -2391,7 +2406,7 @@  get_interesting_files_under_dir(const string dir,
   FTSENT *entry;
   unordered_set<string> files;
   while ((entry = fts_read(file_hierarchy)))
-    maybe_update_package_content(entry, opts, file_name_to_look_for, files);
+    maybe_update_package_content(entry, opts, file_name_to_look_for, dir, files);
 
   for (unordered_set<string>::const_iterator i = files.begin();
        i != files.end();