rsync - May 2018 - [Bug 13445] New: Fuzzy searching in link-dest tries to open regular file as directory

https://bugzilla.samba.org/show_bug.cgi?id=13445

            Bug ID: 13445
           Summary: Fuzzy searching in link-dest tries to open regular
                    file as directory
           Product: rsync
           Version: 3.1.3
          Hardware: All
                OS: All
            Status: NEW
          Severity: normal
          Priority: P5
         Component: core
          Assignee: wayned at samba.org
          Reporter: alguien at respiranto.de
        QA Contact: rsync-qa at samba.org

Created attachment 14205
  --> https://bugzilla.samba.org/attachment.cgi?id=14205&action=edit
Patch to handle ENOTDIR in flist.c:send_directory()

If rsync is called with doubled --fuzzy flag and --link-dest
(or --compare-dest, --copy-dest), it fails in a situation as described below.
The problem appears in both version 3.1.3 and the current development version
3.1.4dev.


Let there be the following directory structure

$PWD/
`- src/
   `- a/
      `- f
`- ref/
   `- a

where src/a/f and ref/a are regular files and the files src/a and ref/a
actually have the same name.

To create it:

$ mkdir -p src/a ref; touch src/a/f ref/a


Now let rsync copy files from src/ to tgt/ linking to (fuzzy matched) files
in ref/, as follows:

$ rsync --recursive --fuzzy --fuzzy --link-dest=../ref src/ tgt

Rsync fails with:

rsync: opendir "/tmp/rsync-test/tgt/../ref/a" failed: Not a directory
(20)
rsync error: some files/attrs were not transferred (see previous errors) (code
23) at main.c(1189) [sender=3.1.3]


This is due to rsync trying to find a fuzzy match for src/a/f in ref/a, which
obviously fails, as ref/a is not a directory.

The problem arises in flist.c:send_directory() when called by
flist.c:get_dirlist() which in turn is called by
generator.c:recv_generator(), where the former two are called with ../ref/a .

Since the case of an inexistant directory is already handled in 
flist.c:send_directory(), that is probably also where to handle the case of
the filename referring to a file not being a directory.  See the attached
patch.

In fact, I am not sure that it is a good idea to just ignore ENOENT and
ENOTDIR if !am_sender, since those should also possibly mean a real error.
Or am I mistaken here?

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #1 from Einhard Leichtfuß <alguien at respiranto.de> ---
Maybe more or less related: Bug 11866 [0], Bug 12489 [1]

[0] https://bugzilla.samba.org/show_bug.cgi?id=11866
[1] https://bugzilla.samba.org/show_bug.cgi?id=12489

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #2 from Ben RUBSON <ben.rubson at gmail.com> ---
Nice catch, I was able to easily reproduce this issue just creating a directory
with the name of a just-deleted file.

The path you mention Einhard seems to be the only one where no check is done to
be sure a directory is given to send_directory().

I think it would however be better to explicitly tells send_directory() that
fbuf could not be a directory.

Perhaps creating a new FLAG_PERHAPS_NOTDIR flag ?
recv_generator() would add FLAG_PERHAPS_NOTDIR to GDL_IGNORE_FILTER_RULES in
its get_dirlist() call, get_dirlist() would then send this flag to
send_directory(), and send_directory() would properly return if fbuf is not a
directory and (flags & FLAG_PERHAPS_NOTDIR).

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #3 from Ben RUBSON <ben.rubson at gmail.com> ---
We could also stat() fnamecmpbuf in recv_generator(), but I think it's
rather
interesting to save such calls.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #4 from Einhard Leichtfuß <alguien at respiranto.de> ---
Did I understand correctly that you were able to reproduce this in a notably
different way?

I had not sufficiently examined the code to see that in all the other cases
the existance of a directory is made sure before.  At least under this
precondition, I like your idea to add such a flag.

However, wouldn't it then be consequent to also use flags in order to say
when a file may not exist at all?  Again, I have not thoroughly read the
whole code, however I presume there are cases (also, if !am_sender) where
the existance of a file (being a directory) is assumed and a violation of
this assumption causes trouble and hence should not be ignored.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #5 from Ben RUBSON <ben.rubson at gmail.com> ---
I reproduced the issue the same way, I meant just creating a directory in my
backed-up tree with the name of a just-deleted file, this file remaining in the
link-dest folder.

I'm not sure the opposite (an existing directory used as if it were a file)
exists, as several tests (stating, opening) must be performed by rsync on files
before using them.
Though I did not go through the whole code path looking for such a case.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #6 from Ben RUBSON <ben.rubson at gmail.com> ---
Created attachment 14231
  --> https://bugzilla.samba.org/attachment.cgi?id=14231&action=edit
Patch using FLAG_PERHAPS_DIR

Here is a working patch using the method detailed in comment #2.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #7 from Ben RUBSON <ben.rubson at gmail.com> ---
Wayne, let's merge this ?
Many thanks !

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

Wayne Davison <wayne at opencoder.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #8 from Wayne Davison <wayne at opencoder.net> ---
Thanks for the patch!  I've committed a slightly tweaked version to git.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

https://bugzilla.samba.org/show_bug.cgi?id=13445

--- Comment #9 from Ben RUBSON <ben.rubson at gmx.com> ---
Good news, thank you very much Wayne !
Glad to see you back :)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.