Bug 647231 - RHEL7: unable to backup and restore a symlink
RHEL7: unable to backup and restore a symlink
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: beakerlib (Show other bugs)
19
Unspecified Unspecified
urgent Severity high
: ---
: ---
Assigned To: Karel Srot
: Patch
: 839716 (view as bug list)
Depends On:
Blocks: 893062
  Show dependency treegraph
 
Reported: 2010-10-27 12:01 EDT by Martin Cermak
Modified: 2013-06-27 12:34 EDT (History)
9 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 893062 (view as bug list)
Environment:
Last Closed: 2013-06-27 12:34:43 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Patch: backing up symlinks + test suite adjustment (2.89 KB, patch)
2010-10-29 05:27 EDT, Petr Šplíchal
no flags Details | Diff
proposed fix for issue with symlink in parent dir, including test case (2.22 KB, patch)
2012-07-19 08:08 EDT, Karel Srot
no flags Details | Diff
proposed fix for issue with symlink in parent dir, including test case (updated) (2.23 KB, patch)
2012-07-20 03:24 EDT, Karel Srot
no flags Details | Diff

  None (edit)
Comment 1 Petr Muller 2010-10-27 12:21:03 EDT
Petr, this is you code, would you look on this?
Comment 2 Petr Šplíchal 2010-10-29 05:16:46 EDT
Sure, seems the problem has been introduced by the following
commit: 

http://git.fedorahosted.org/git/?p=beakerlib.git;a=commitdiff;h=ab63f048

Where "readlink -f" is used to get the full path of the file to be
backed up. It expands to the real file name instead to keep the
symlink. I'll revert the change, which purpose seems to be only
"get rid of that long ugly regexp" (which worked better).
Comment 3 Petr Šplíchal 2010-10-29 05:27:38 EDT
Created attachment 456421 [details]
Patch: backing up symlinks + test suite adjustment

Adding patch. Includes a small adjustment to the assertRun() to be
able to expect multiple values (as rlRun() does).

 * fixed symlink backup
 * added test coverage for the bug
 * tiny test suite adjustment [multiple expected values]

Please, review.
Comment 4 Martin Cermak 2010-10-29 08:32:51 EDT
Thank you, Petr, for digging into this. Yes, it seems that replacing the readlink command with that ugly regexp solves the issue :-)
Comment 5 Petr Šplíchal 2010-10-29 08:42:23 EDT
Great, thanks for verifying! Any objections to the subtle test
suite enhancement from anybody else? Otherwise I'll push.
Comment 6 Ales Zelinka 2010-11-01 05:59:15 EDT
(In reply to comment #5)
>Any objections to the subtle test
> suite enhancement from anybody else? Otherwise I'll push.

> -    if [ "$status" -eq "$expected" ]; then
> +    if [[ "$status" =~ ^$expected$ ]]; then

no objections
Comment 7 Petr Šplíchal 2010-11-04 05:09:23 EDT
Thanks. Pushed to git:

http://git.fedorahosted.org/git/?p=beakerlib.git;a=commitdiff;h=63a93da
Comment 10 Dalibor Pospíšil 2012-05-31 11:13:28 EDT
I have experienced the issue too. It would be cool if it is resolved.
Comment 11 Miroslav Vadkerti 2012-06-18 09:31:12 EDT
Please note that symlink support is crucial in RHEL7. Adjusting bug summary
Comment 12 Karel Srot 2012-07-19 03:56:35 EDT
increasing priority/severity, I have several tests failing on RHEL7 because of deleted items in /var/run, rlFileRestore breaks the environment for other tests.
Comment 13 Karel Srot 2012-07-19 04:12:37 EDT
Pasting reproducer. Imagine /var/run instead of /tmp/run and you see why we are f****d up on RHEL7.


# mkdir -p /run/a /run/b
# ln -s /run /tmp/run
You have mail in /var/spool/mail/root
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:10 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:00 a
drwxr-xr-x 2 root root 4096 Jul 19 04:00 b
# . /usr/lib/beakerlib/beakerlib.sh
# rlJournalStart
# rlFileBackup /tmp/run/a 
# rmdir /tmp/run/a
# rlFileRestore
# ls -l /tmp/run
total 8
drwxr-xr-x 2 root root 4096 Jul 19 04:00 a
# ls -l /run
total 8
drwxr-xr-x 2 root root 4096 Jul 19 04:00 b
Comment 14 Karel Srot 2012-07-19 04:16:38 EDT
No I can see that the proposed patch is applied in beakerlib-1.5-1.el5 but the rlFileRestore is broken anyway
Comment 15 Karel Srot 2012-07-19 04:38:00 EDT
Following change fixed it for me, but I didn't do any detailed testing.

--- /usr/share/beakerlib/infrastructure.sh	2012-05-15 11:45:38.000000000 -0400
+++ /usr/share/beakerlib/infrastructure.sh.new	2012-07-19 04:33:42.000000000 -0400
@@ -302,6 +302,8 @@
     for file in "$@"; do
         # convert relative path to absolute, remove trailing slash
         file=$(echo "$file" | sed "s|^\([^/]\)|$PWD/\1|" | sed "s|/$||")
+	 # follow symlinks
+	 file=$( readlink -n -f $file )
         path=$(dirname "$file")
 
         # bail out if the file does not exist
Comment 16 Karel Srot 2012-07-19 06:17:07 EDT
Previous fix breaks the behavior expected in #c0. We had a discussion with Petr and proper symlink handling seems to be a challenge (backup of symlinks in the directory tree and also backup of their targets). Anyway, this is a better quick fix for "symlink in parent" issue, preserving the symlink backup behavior. This should be enough to fix bad behavior on RHEL7 and should not break anything. :-) Could you please review it ASAP?

--- /usr/share/beakerlib/infrastructure.sh	2012-05-15 11:45:38.000000000 -0400
+++ /usr/share/beakerlib/infrastructure.sh.new	2012-07-19 06:02:19.000000000 -0400
@@ -302,7 +302,10 @@
     for file in "$@"; do
         # convert relative path to absolute, remove trailing slash
         file=$(echo "$file" | sed "s|^\([^/]\)|$PWD/\1|" | sed "s|/$||")
+        # follow symlinks
         path=$(dirname "$file")
+        path=$(readlink -n -f $path)
+        file=$path/$(basename $file)
 
         # bail out if the file does not exist
         if ! [ -e "$file" ]; then



check that we do not rewrite symlink in the parent

# . /usr/lib/beakerlib/beakerlib.sh
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:31 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b
# rlJournalStart
# rlFileBackup /tmp/run/a
# rmdir /tmp/run/a
# rlFileRestore
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:31 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b


check that we are able to backup symlink itself

# rlJournalStart
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 04:31 /tmp/run -> /run
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b
# rlFileBackup /tmp/run
# rm /tmp/run
rm: remove symbolic link `/tmp/run'? y
# rlFileRestore 
# ls -l /tmp/run/
total 16
drwxr-xr-x 2 root root 4096 Jul 19 04:31 a
drwxr-xr-x 2 root root 4096 Jul 19 04:30 b
# ls -l /tmp/run
lrwxrwxrwx 1 root root 4 Jul 19 06:07 /tmp/run -> /run


This is just to illustrate the no-follow symlink backup problem:

# rlJournalStart
# ls -l /tmp/test
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:12 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# ls -l /tmp/test/
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:12 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# cat /tmp/test/run/a/y
1
# rlFileBackup /tmp/test
# echo 2 > /tmp/test/run/a/y
# rm -rf /tmp/test
# rlFileRestore 
# ls -l /tmp/test
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:15 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# ls -l /tmp/test/
total 8
lrwxrwxrwx 1 root root 4 Jul 19 06:15 run -> /run
-rw-r--r-- 1 root root 0 Jul 19 06:11 x
# cat /tmp/test/run/a/y
2
This is bad! I want the file to be restored with content 1!!!
Comment 17 Ales Zelinka 2012-07-19 06:44:23 EDT
Karel, can you please included a test for the fix as well? Thanks
Comment 18 Karel Srot 2012-07-19 07:13:06 EDT
Sure, I will prepare a git patch with both.
Comment 19 Karel Srot 2012-07-19 08:08:49 EDT
Created attachment 599125 [details]
proposed fix for issue with symlink in parent dir, including test case

Attached proposed patch. I hope this is my last comment in this bug today. :-)
Comment 20 Petr Muller 2012-07-19 11:23:54 EDT
The tests FAIL for me after applying:

 [ INFO ] Running test_rlFileBackup_SymlinkInParent
 [ PASS ] Preparing tmp directory
 [ PASS ] Preparing target directory
 [ PASS ] Preparing linked directory
 [ FAIL ] Backing up the link/file1
 [ PASS ] Removing link/file1
 [ FAIL ] Restoring the file1
 [ PASS ] Testing that link points to target
 [ PASS ] Testing that link/file1 was restored
 [ PASS ] Testing that link/file2 is still present
 [ PASS ] Clean up
Comment 21 Petr Muller 2012-07-19 11:36:23 EDT
(In reply to comment #20)
> The tests FAIL for me after applying:

Please disregard. rlBackup* & friends apparently need root. Mea culpa.
Comment 22 Karel Srot 2012-07-20 03:24:38 EDT
Created attachment 599321 [details]
proposed fix for issue with symlink in parent dir, including test case (updated)

Apparently other test cases are ignoring rlFileBackup/Restore errors if the test is executed as non-root. I have updated the patch accordingly.
Comment 24 Petr Muller 2012-07-24 18:46:37 EDT
*** Bug 839716 has been marked as a duplicate of this bug. ***
Comment 25 Petr Šplíchal 2012-07-25 11:30:10 EDT
Thanks for the fix, Karel.
Comment 27 Petr Muller 2013-01-08 09:21:48 EST
beakerlib-1.6-1 is already in fedora
Comment 28 Fedora End Of Life 2013-04-03 16:38:26 EDT
This bug appears to have been reported against 'rawhide' during the Fedora 19 development cycle.
Changing version to '19'.

(As we did not run this process for some time, it could affect also pre-Fedora 19 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 19 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora19

Note You need to log in before you can comment on or make changes to this bug.