pkg_tar: Fix #948: Don't add duplicate directory members to tar file when a symlink with the same path exists (#949)
Fixes #948.
I've modified the deduplication logic to account for the fact that
directories must not have the same path as other types of files.
Previously, the paths `/lib` and `/lib/` would be treated differently.
Two test cases are included: one for symlinks preceding directories, and
one for directories preceding symlinks. In both cases, the first member
is preserved and the second member with the same path is discarded as a
duplicate.
diff --git a/pkg/private/tar/tar_writer.py b/pkg/private/tar/tar_writer.py
index 6c0d926..b81f6db 100644
--- a/pkg/private/tar/tar_writer.py
+++ b/pkg/private/tar/tar_writer.py
@@ -35,6 +35,18 @@
_DEBUG_VERBOSITY = 0
+TARFILE_MEMBER_TYPE_TO_STR = {
+ b"0": "REGTYPE",
+ b"\0": "AREGTYPE",
+ b"1": "LNKTYPE",
+ b"2": "SYMTYPE",
+ b"3": "CHRTYPE",
+ b"4": "BLKTYPE",
+ b"5": "DIRTYPE",
+ b"6": "FIFOTYPE",
+ b"7": "CONTTYPE",
+}
+
class TarFileWriter(object):
"""A wrapper to write tar files."""
@@ -103,13 +115,7 @@
self.tar = tarfile.open(name=name, mode=mode, fileobj=self.fileobj,
format=tarfile.GNU_FORMAT)
- self.members = set()
- self.directories = set()
- # Preseed the added directory list with things we should not add. If we
- # some day need to allow '.' or '/' as an explicit member of the archive,
- # we can adjust that here based on the setting of root_directory.
- self.directories.add('/')
- self.directories.add('./')
+ self.existing_members = {}
self.create_parents = create_parents
self.allow_dups_from_deps = allow_dups_from_deps
@@ -119,28 +125,43 @@
def __exit__(self, t, v, traceback):
self.close()
- def _have_added(self, path):
- """Have we added this file before."""
- return (path in self.members) or (path in self.directories)
+ def _existing_member_type(self, path):
+ """Retrieve an existing tar file member's type if we have added it previously,
+ return None otherwise."""
+ # Things we should not add.
+ # If we some day need to allow '.' or '/' as an explicit member of the archive,
+ # we can adjust that here based on the setting of root_directory.
+ if path == '/' or path == './':
+ return tarfile.DIRTYPE
+
+ normalized_path = path.rstrip("/")
+ return self.existing_members.get(normalized_path, None)
def _addfile(self, info, fileobj=None):
"""Add a file in the tar file if there is no conflict."""
if info.type == tarfile.DIRTYPE:
- # Enforce the ending / for directories so we correctly deduplicate.
+ # Enforce the ending / for directories.
if not info.name.endswith('/'):
info.name += '/'
- if not self.allow_dups_from_deps and self._have_added(info.name):
+ existing_member_type = self._existing_member_type(info.name)
+ if not self.allow_dups_from_deps and existing_member_type is not None:
# Directories with different contents should get merged without warnings.
# If they have overlapping content, the warning will be on their duplicate *files* instead
if info.type != tarfile.DIRTYPE:
print('Duplicate file in archive: %s, '
'picking first occurrence' % info.name)
+ # Directories that shadow
+ elif existing_member_type != tarfile.DIRTYPE and existing_member_type != tarfile.SYMTYPE:
+ print('Directory shadows a member of type %s in archive: %s, '
+ 'picking first occurrence' % (TARFILE_MEMBER_TYPE_TO_STR.get(
+ existing_member_type, "UNKNOWN"), info.name))
+
return
self.tar.addfile(info, fileobj)
- self.members.add(info.name)
- if info.type == tarfile.DIRTYPE:
- self.directories.add(info.name)
+ # Strip the trailing slash from the path so that we can detect when, for example, we are
+ # trying to overwrite a symbolic link with a directory.
+ self.existing_members[info.name.rstrip("/")] = info.type
def add_directory_path(self,
path,
@@ -182,7 +203,8 @@
for next_level in dirs[0:-1]:
parent_path = parent_path + next_level + '/'
- if self.create_parents and not self._have_added(parent_path):
+ if self.create_parents and self._existing_member_type(
+ parent_path) is None:
self.add_directory_path(
parent_path,
uid=uid,
@@ -224,7 +246,8 @@
return
if name == '.':
return
- if not self.allow_dups_from_deps and name in self.members:
+ if not self.allow_dups_from_deps and self._existing_member_type(
+ name) is not None:
return
if mtime is None:
diff --git a/tests/tar/tar_writer_test.py b/tests/tar/tar_writer_test.py
index e7d9c59..fcc5889 100644
--- a/tests/tar/tar_writer_test.py
+++ b/tests/tar/tar_writer_test.py
@@ -162,7 +162,7 @@
"rules_pkg/tests/testdata/tar_test.tar")
f.add_tar(input_tar_path)
input_tar = tarfile.open(input_tar_path, "r")
- for file_name in f.members:
+ for file_name in f.existing_members.keys():
input_file = input_tar.getmember(file_name)
output_file = f.tar.getmember(file_name)
self.assertEqual(input_file.mtime, output_file.mtime)
@@ -300,6 +300,31 @@
self.assertTarFileContent(self.tempfile, expected_content)
+ def testDirectoryDoesNotShadowSymlink(self):
+ with tar_writer.TarFileWriter(self.tempfile, create_parents=True, allow_dups_from_deps=False) as f:
+ f.add_file("target_dir", tarfile.DIRTYPE)
+ f.add_file("symlink", tarfile.SYMTYPE, link="target_dir")
+ f.add_file("symlink", tarfile.DIRTYPE)
+ f.add_file('symlink/a', content="q")
+ content = [
+ {"name": "target_dir", "type": tarfile.DIRTYPE},
+ {"name": "symlink", "type": tarfile.SYMTYPE},
+ {"name": "symlink/a", "type": tarfile.REGTYPE},
+ ]
+ self.assertTarFileContent(self.tempfile, content)
+
+ def testSymlinkDoesNotShadowDirectory(self):
+ with tar_writer.TarFileWriter(self.tempfile, create_parents=True, allow_dups_from_deps=False) as f:
+ f.add_file("target_dir", tarfile.DIRTYPE)
+ f.add_file("not_a_symlink", tarfile.DIRTYPE)
+ f.add_file("not_a_symlink", tarfile.SYMTYPE, link="target_dir")
+ f.add_file('not_a_symlink/a', content="q")
+ content = [
+ {"name": "target_dir", "type": tarfile.DIRTYPE},
+ {"name": "not_a_symlink", "type": tarfile.DIRTYPE},
+ {"name": "not_a_symlink/a", "type": tarfile.REGTYPE},
+ ]
+ self.assertTarFileContent(self.tempfile, content)
if __name__ == "__main__":
