Bug 784467

Summary: inconsistent recursive directory copy
Product: [Fedora] Fedora Reporter: JW <ohtmvyyn>
Component: rshAssignee: Adam Tkac <atkac>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 16CC: atkac, ovasik
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: rsh-0.17-68.fc16 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-02-17 00:53:01 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:

Description JW 2012-01-25 02:45:17 UTC
Description of problem:
Recursively copying a directory is inconsistent.  It changes according to whether the source directory has a trailing / or not.  The copy should be consistent regardless of the trailing /.


Version-Release number of selected component (if applicable):
/rsh-0.17-65

How reproducible:
Always

Steps to Reproduce:
1. on local: mkdir src; touch src/file; chmod o-rx src
2. on remote: mkdir dest; chmod a+rwx dest
3. on local: rcp -pr src remote:dest
4. on remote ls -al dest
5. on remote: rm -fr dest; mkdir dest; chmod a+rwx dest\
6. on local: rcp -pr src/ remote:dest
  
Actual results:
4. 
drwxrwxrwx 2 root root 4096 2012-01-25 13:28 ./
drwxr-xr-x 3 root root 4096 2012-01-25 13:28 ../
-rw-r--r-- 1 root root    0 2012-01-25 13:28 src
5.
drwxr--x--- 2 root root 4096 2012-01-25 13:28 ./
drwxr-xr-x 3 root root 4096 2012-01-25 13:28 ../
-rw-r--r-- 1 root root    0 2012-01-25 13:28 file

Expected results:
4. ok, exactly as expected
5. same as 4

Additional info:
Note very carefully that steps 3 and 6 only differ by the trailing / on src.

This is certainly a bug and not reasonable behavior.  The src is a directory regardless of whether a / is appended. The trailing / should never make a difference.

In the code there is (in source() and in rsource()):

    last = strrchr(name, '/');
    if (last == 0)
        last = name;
    else
        last++;

With a trailing / 'last' points to a null charactor which results in the sending of "D0750 0 " with an empty name component! Normally would be "D0750 0 src".  That certainly has never been an acceptable implementation of the rcp protocol. On the remote the faulty line gets conveniently converted into "dest/" which means that src never gets created on the remote.

Clearly what is really needed is:
    last = strrchr_which_isnt_the_last_character(name, '/');
but that doesn't seem reasonable.

Either the command-line args such as src/ should have their trailing / removed in main() else the following code could be added after each such strrchr():
    if (last && !last[1])
    {
        *last = '\0';
        last = strrchr(name, '/');
    }

Comment 1 JW 2012-01-25 02:59:18 UTC
Also worth mentioning are the disastrous consequences to be had if root recursively copies a directory like this: "rcp -pr src/ remote:/tmp".  That will have the effect of changing the modes on /tmp to the same as src and many processes will find themselves unable to write into /tmp.

Note also, that a user always has the option of using "rcp -pr src/* remote:dest" if they really do want to copy the contents of src only and not the directory itself.

As it stands the rcp implementation is plainly defective.

AND PLEASE DONT BOTHER TELLING ME I SHOULD NOT BE USING rcp.  Not that you need to know, but in a heavily congested server environment on a closed network there is a lot to be said for using rcp instead of scp. And have you ever tried reading scp traffic over a LAN when you really need to be able to?

Comment 2 Adam Tkac 2012-01-31 15:08:50 UTC
(In reply to comment #1)
> AND PLEASE DONT BOTHER TELLING ME I SHOULD NOT BE USING rcp.  Not that you need
> to know, but in a heavily congested server environment on a closed network
> there is a lot to be said for using rcp instead of scp. And have you ever tried
> reading scp traffic over a LAN when you really need to be able to?

If you shouldn't use rcp, it wouldn't be in distribution. You are right it's better than scp in certain networks.

If we are going to handle all corner cases then

    if (last && !last[1])
    {
        *last = '\0';
        last = strrchr(name, '/');
    }

after strrchr() is not sufficient because one can supply 'dirname///' as a directory name. I'm investigating more general fix.

Comment 3 Adam Tkac 2012-01-31 16:37:18 UTC
I've just pushed following patch which should handle all corner cases:

http://pkgs.fedoraproject.org/gitweb/?p=rsh.git;a=blob_plain;f=netkit-rsh-0.17-rh784467.patch;hb=c0f15b59f4dd0967d6099eb3ba8de841f01840ac

Comment 4 Fedora Update System 2012-01-31 16:42:03 UTC
rsh-0.17-68.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/rsh-0.17-68.fc16

Comment 5 Fedora Update System 2012-01-31 22:05:24 UTC
Package rsh-0.17-68.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing rsh-0.17-68.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-1094/rsh-0.17-68.fc16
then log in and leave karma (feedback).

Comment 6 Fedora Update System 2012-02-17 00:53:01 UTC
rsh-0.17-68.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.