Skip to content

Commit

Permalink
Fix another cornercase with symlink handling
Browse files Browse the repository at this point in the history 
* Symlinks to a directory inside of /etc/pki were being created as empty directories.
* Note: unittests being added for both this problem and a second problem:
  two links to the same external file will be copied as two separate files but ideally we want one
  to be a copy and the other to link to the copy.  The unittests for the second problem are
  commented out.

Fixes: https://issues.redhat.com/browse/RHEL-3284
  • Loading branch information
@abadger @pirat89
abadger authored and pirat89 committed Jan 22, 2024
1 parent 353cd03 commit bec6615
Show file tree
Hide file tree
Showing 2 changed files with 173 additions and 21 deletions.
66 changes: 45 additions & 21 deletions repos/system_upgrade/common/actors/targetuserspacecreator/libraries/userspacegen.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ def _mkdir_with_copied_mode(path, mode_from):

def _choose_copy_or_link(symlink, srcdir):
"""
Copy file contents or create a symlink depending on where the pointee resides.
Determine whether to copy file contents or create a symlink depending on where the pointee resides.
:param symlink: The source symlink to follow. This must be an absolute path.
:param srcdir: The root directory that every piece of content must be present in.
Expand Down Expand Up @@ -415,7 +415,7 @@ def _choose_copy_or_link(symlink, srcdir):
# To make comparisons, we need to resolve all symlinks in the directory
# structure leading up to pointee. However, we can't include pointee
# itself otherwise it will resolve to the file that it points to in the
# end.
# end (which would be wrong if pointee_filename is a symlink).
canonical_pointee_dir, pointee_filename = os.path.split(pointee_as_abspath)
canonical_pointee_dir = os.path.realpath(canonical_pointee_dir)

Expand Down Expand Up @@ -454,6 +454,35 @@ def _choose_copy_or_link(symlink, srcdir):
return ('copy', pointee_as_abspath)


def _copy_symlinks(symlinks_to_process, srcdir):
"""
Copy file contents or create a symlink depending on where the pointee resides.
:param symlinks_to_process: List of 2-tuples of (src_path, target_path). Each src_path
should be an absolute path to the symlink. target_path is the path to where we
need to create either a link or a copy.
:param srcdir: The root directory that every piece of content must be present in.
:raises ValueError: if the arguments are not correct
"""
for source_linkpath, target_linkpath in symlinks_to_process:
try:
action, source_path = _choose_copy_or_link(source_linkpath, srcdir)
except BrokenSymlinkError as e:
# Skip and report broken symlinks
api.current_logger().warning('{} Will not copy the file!'.format(str(e)))
continue

if action == "copy":
# Note: source_path could be a directory, so '-a' or '-r' must be
# given to cp.
run(['cp', '-a', source_path, target_linkpath])
elif action == 'link':
run(["ln", "-s", source_path, target_linkpath])
else:
# This will not happen unless _copy_or_link() has a bug.
raise RuntimeError("Programming error: _copy_or_link() returned an unknown action:{}".format(action))


def _copy_decouple(srcdir, dstdir):
"""
Copy files inside of `srcdir` to `dstdir` while decoupling symlinks.
Expand All @@ -467,7 +496,6 @@ def _copy_decouple(srcdir, dstdir):
.. warning::
`dstdir` must already exist.
"""
symlinks_to_process = []
for root, directories, files in os.walk(srcdir):
# relative path from srcdir because srcdir is replaced with dstdir for
# the copy.
Expand All @@ -476,11 +504,24 @@ def _copy_decouple(srcdir, dstdir):
# Create all directories with proper permissions for security
# reasons (Putting private data into directories that haven't had their
# permissions set appropriately may leak the private information.)
symlinks_to_process = []
for directory in directories:
source_dirpath = os.path.join(root, directory)
target_dirpath = os.path.join(dstdir, relpath, directory)

# Defer symlinks until later because we may end up having to copy
# the file contents and the directory may not exist yet.
if os.path.islink(source_dirpath):
symlinks_to_process.append((source_dirpath, target_dirpath))
continue

_mkdir_with_copied_mode(target_dirpath, source_dirpath)

# Link or create all directories that were pointed to by symlinks and
# then reset symlinks_to_process for use by files.
_copy_symlinks(symlinks_to_process, srcdir)
symlinks_to_process = []

for filename in files:
source_filepath = os.path.join(root, filename)
target_filepath = os.path.join(dstdir, relpath, filename)
Expand All @@ -494,24 +535,7 @@ def _copy_decouple(srcdir, dstdir):
# Not a symlink so we can copy it now too
run(['cp', '-a', source_filepath, target_filepath])

# Now process all symlinks
for source_linkpath, target_linkpath in symlinks_to_process:
try:
action, source_path = _choose_copy_or_link(source_linkpath, srcdir)
except BrokenSymlinkError as e:
# Skip and report broken symlinks
api.current_logger().warning('{} Will not copy the file!'.format(str(e)))
continue

if action == "copy":
# Note: source_path could be a directory, so '-a' or '-r' must be
# given to cp.
run(['cp', '-a', source_path, target_linkpath])
elif action == 'link':
run(["ln", "-s", source_path, target_linkpath])
else:
# This will not happen unless _copy_or_link() has a bug.
raise RuntimeError("Programming error: _copy_or_link() returned an unknown action:{}".format(action))
_copy_symlinks(symlinks_to_process, srcdir)


def _copy_certificates(context, target_userspace):
Expand Down
128 changes: 128 additions & 0 deletions ...em_upgrade/common/actors/targetuserspacecreator/tests/unit_test_targetuserspacecreator.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,70 @@ def temp_directory_layout(tmp_path, initial_structure):
},
id="Absolute_symlink_to_a_file_inside_via_a_symlink_to_the_rootdir"
)),
# This should be fixed but not necessarily for this release.
# It makes sure that when we have two separate links to the
# same file outside of /etc/pki, one of the links is copied
# as a real file and the other is made a link to the copy.
# (Right now, the real file is copied in place of both links.)
# (pytest.param(
# {
# 'dir': {
# 'fileA': '/outside/fileC',
# 'fileB': '/outside/fileC',
# },
# 'outside': {
# 'fileC': None,
# },
# },
# {
# 'dir': {
# 'fileA': None,
# 'fileB': '/dir/fileA',
# },
# },
# id="Absolute_two_symlinks_to_the_same_copied_file"
# )),
(pytest.param(
{
'dir': {
'fileA': None,
'link_to_dir': '/dir/inside',
'inside': {
'fileB': None,
},
},
},
{
'dir': {
'fileA': None,
'link_to_dir': '/dir/inside',
'inside': {
'fileB': None,
},
},
},
id="Absolute_symlink_to_a_dir_inside"
)),
(pytest.param(
{
'dir': {
'fileA': None,
'link_to_dir': '/outside',
},
'outside': {
'fileB': None,
},
},
{
'dir': {
'fileA': None,
'link_to_dir': {
'fileB': None,
},
},
},
id="Absolute_symlink_to_a_dir_outside"
)),
(pytest.param(
# This one is very tricky:
# * The user has made /etc/pki a symlink to some other directory that
Expand Down Expand Up @@ -671,6 +735,70 @@ def temp_directory_layout(tmp_path, initial_structure):
},
id="Relative_symlink_to_a_file_inside_via_a_symlink_to_the_rootdir"
)),
# This should be fixed but not necessarily for this release.
# It makes sure that when we have two separate links to the
# same file outside of /etc/pki, one of the links is copied
# as a real file and the other is made a link to the copy.
# (Right now, the real file is copied in place of both links.)
# (pytest.param(
# {
# 'dir': {
# 'fileA': '../outside/fileC',
# 'fileB': '../outside/fileC',
# },
# 'outside': {
# 'fileC': None,
# },
# },
# {
# 'dir': {
# 'fileA': None,
# 'fileB': 'fileA',
# },
# },
# id="Relative_two_symlinks_to_the_same_copied_file"
# )),
(pytest.param(
{
'dir': {
'fileA': None,
'link_to_dir': '../outside',
},
'outside': {
'fileB': None,
},
},
{
'dir': {
'fileA': None,
'link_to_dir': {
'fileB': None,
},
},
},
id="Relative_symlink_to_a_dir_outside"
)),
(pytest.param(
{
'dir': {
'fileA': None,
'link_to_dir': 'inside',
'inside': {
'fileB': None,
},
},
},
{
'dir': {
'fileA': None,
'link_to_dir': 'inside',
'inside': {
'fileB': None,
},
},
},
id="Relative_symlink_to_a_dir_inside"
)),
(pytest.param(
# This one is very tricky:
# * The user has made /etc/pki a symlink to some other directory that
Expand Down

0 comments on commit bec6615

Please sign in to comment.