#2131 PR `merged`: Improve handling of broken symlinks (fix #2129, fix #2130)¶

Labels: enhancement, bug, fixed / solved / done

OliverO2 opened issue at 2019-04-30 12:14:¶

Pull Request Details:¶

Type: Bug Fix
Impact: Normal
Reference to related issue (URL): #2129, #2130
How was this pull request tested? On Ubuntu 18.04.2 LTS: Compared $ROOTFS_DIR contents before and after code changes, verified everything works as intended.
Brief description of the changes in this pull request:
1. Relative symbolic links are now properly resolved before checking the link's target.
2. Directories, which are link targets, no longer fail when trying to copy them. Instead, a meaningful hint is printed for the user to decide.

Note: On the test system, there is a relative symbolic link /etc/ssh/sshd_config -> sshd_config.company, which points to a customized configuration.

Output without this PR's changes¶

$ sudo usr/sbin/rear -v mkrescue
Relax-and-Recover 2.4 / Git
[...]
Copying all files in /lib*/firmware/
Broken symlink './etc/ssh/sshd_config' in recovery system because 'readlink' cannot determine its link target
Broken symlink './usr/share/misc/magic' in recovery system because 'readlink' cannot determine its link target
Failed to copy symlink target '/usr/src/linux-headers-4.18.0-18-generic'
Testing that the recovery system in /tmp/rear.2F8DRaogql1zNej/rootfs contains a usable system
[...]

Output with this PR's changes¶

$ sudo usr/sbin/rear -v mkrescue
Relax-and-Recover 2.4 / Git
Running rear mkrescue (PID 4478)
[...]
Copying all files in /lib*/firmware/
Symlink '/usr/share/misc/magic' -> '/usr/share/file/magic' refers to a non-existing directory on the rescue system.
It will not be copied by default. You can include '/usr/share/file/magic' via the 'COPY_AS_IS' configuration variable.
Symlink '/lib/modules/4.18.0-18-generic/build' -> '/usr/src/linux-headers-4.18.0-18-generic' refers to a non-existing directory on the rescue system.
It will not be copied by default. You can include '/usr/src/linux-headers-4.18.0-18-generic' via the 'COPY_AS_IS' configuration variable.
Testing that the recovery system in /tmp/rear.5TcC1r3dx1c336B/rootfs contains a usable system
[...]

jsmeix commented at 2019-04-30 14:15:¶

@OliverO2
many thanks for your improvement!

On first glance by plain looking at the code it looks good to me
but I like to test it a bit on Thursday before I approve it...

jsmeix commented at 2019-04-30 14:17:¶

@gdha @rmetrich
could you also have a look here and review it?

Do you think we can add it to ReaR 2.5?

Because of the reasoning in
https://github.com/rear/rear/issues/2129#issuecomment-487938670
I think this could be good to have in ReaR 2.5.

gdha commented at 2019-05-01 07:19:¶

@jsmeix I think it would be good to add it to the 2.5 release.

OliverO2 commented at 2019-05-03 10:08:¶

@jsmeix Thanks again for trying!

Given the current situation, it seems to be best to leave out code which would solve one problem and might introduce others.

In my view, the GNU coreutils documentation style really sucks: We have two sets of non-synchronized documentation, one of them (the html site) missing version information, the other one (manual page) being vague by failing to define terms. Seems to be the same with dirname: Could you find any definitive statement saying that dirname does a specific sort of canonicalization (as you have observed)?

jsmeix commented at 2019-05-03 12:32:¶

@OliverO2
many thanks for your patient laborious work.
It helps so much to make the ReaR code better.

Regarding my dirname usage in
https://github.com/rear/rear/pull/2131#discussion_r280711680

if [[ "$link_target" != /* ]]; then
    # convert a relative link target into an absolute one
    broken_symlink_dir=$( dirname $broken_symlink )
    link_target="$broken_symlink_dir/$link_target"
fi

I used dirname not for any kind of canonicalization.
I used it to split away the last part (i.e. the symlink name)
from $broken_symlink because $broken_symlink/$link_target is not valid like

/path/to/symlink_dir/relative_symlink_name/../../rel_path/to/target_file_or_dir

but $broken_symlink_dir/$link_target is valid like

/path/to/symlink_dir/../../rel_path/to/target_file_or_dir

Canonicalization of the latter would result

/path/rel_path/to/target_file_or_dir

By the way:
Do you still like symlinks?
(cf. https://github.com/rear/rear/issues/2129#issuecomment-487857644 ;-)

Again thank you for your valuable contributions to ReaR
and enjoy the weekend!

OliverO2 commented at 2019-05-03 13:00:¶

@jsmeix Ah, now I see. Thanks so much for your work. Although the commits were mine, your contribution was certainly the more laborious one!

And yes, I'm still fine with symbolic links. What I have really missed in this case is

proper tooling (debugger, code coverage analysis, automated testing) and
proper documentation of functions used.

Have an excellent weekend, too! Well deserved! :1st_place_medal:

jsmeix commented at 2019-05-03 13:03:¶

Via
https://github.com/rear/rear/commit/b4e70e067dbb90f080d725cc991536f852f62000
I added comments with examples how broken symlinks and their targets look like
so it is hopefully easier to imagine what goes on here when someone else
may have to look at that code at any later time.

[Export of Github issue for rear/rear.]

#2131 PR merged: Improve handling of broken symlinks (fix #2129, fix #2130)¶