Bug 13445 - Fuzzy searching in link-dest tries to open regular file as directory
Summary: Fuzzy searching in link-dest tries to open regular file as directory
Alias: None
Product: rsync
Classification: Unclassified
Component: core (show other bugs)
Version: 3.1.3
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Wayne Davison
QA Contact: Rsync QA Contact
Depends on:
Reported: 2018-05-18 20:18 UTC by Einhard Leichtfuß
Modified: 2020-04-06 20:22 UTC (History)
0 users

See Also:

Patch to handle ENOTDIR in flist.c:send_directory() (616 bytes, patch)
2018-05-18 20:18 UTC, Einhard Leichtfuß
no flags Details
Patch using FLAG_PERHAPS_DIR (1.97 KB, patch)
2018-06-09 15:24 UTC, Ben RUBSON
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Einhard Leichtfuß 2018-05-18 20:18:51 UTC
Created attachment 14205 [details]
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

Let there be the following directory structure

`- 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

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?
Comment 1 Einhard Leichtfuß 2018-05-18 20:37:19 UTC
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
Comment 2 Ben RUBSON 2018-05-19 16:20:56 UTC
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).
Comment 3 Ben RUBSON 2018-05-19 16:22:32 UTC
We could also stat() fnamecmpbuf in recv_generator(), but I think it's rather interesting to save such calls.
Comment 4 Einhard Leichtfuß 2018-05-21 17:27:44 UTC
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.
Comment 5 Ben RUBSON 2018-05-26 16:21:37 UTC
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.
Comment 6 Ben RUBSON 2018-06-09 15:24:48 UTC
Created attachment 14231 [details]

Here is a working patch using the method detailed in comment #2.
Comment 7 Ben RUBSON 2019-10-03 13:32:39 UTC
Wayne, let's merge this ?
Many thanks !
Comment 8 Wayne Davison 2020-04-05 23:44:27 UTC
Thanks for the patch!  I've committed a slightly tweaked version to git.
Comment 9 Ben RUBSON 2020-04-06 20:22:40 UTC
Good news, thank you very much Wayne !
Glad to see you back :)