Make wxFileName and wxDir symlink-friendly

[dghart]

This patch was prompted by trying to use wxFileSystemWatcher to watch a directory that contained a symlink to another directory in the same tree. wxDir::Traverse always follows symlinks, so the destination dir was added multiple times, the symlink not at all.

The main patch does the following:

wxFileName: As suggested on wx-dev, there are two new functions, DontFollowLink() and ShouldFollowLink(). I've also added a dereferenceLink parameter to the static versions of FileExists(), DirExists() and Exists(), the current behaviour being the default. These changes are documented.

wxDir: The only alteration needed was to wxDirData::Read in src/unix/dir.cpp, which now uses wxFileName::DirExists with the new dereferenceLink parameter false. I've not made deferencing optional as I doubt if anyone would ever want to; but it could be added if you wish.

This fix also makes wxFileName::Rmdir(wxPATH_RMDIR_RECURSIVE) succeed when the directory contains a symlink-to-directory; previously the symlink wasn't removed, so the final wxRmdir() failed.

The other patch adds a section testing this to the 'filename' unit test.

[PB]

(In #14544) Replying to dghart:

(In #14488) Replying to vadz:

I wonder if we shouldn't just remove the filespec argument. If it's really going to simplify the implementation a lot, I'd be ready to sacrifice it. After all, you always can easily filter out the files you're not interested in in your event handler and usually you'd want to watch all files anyhow, wouldn't you? If we want to keep support for this feature, we'd need to add individual watches for all files matching it under the given tree. And possibly also internal watches for all directories that would be handled to add watches for any newly created files. This doesn't seem easy to do...

Fortunately the latest patch, #14544, changes how a filespec is handled and solves these issues.

Vadim may have meant it generally: underlying native MSW API doesn't support watching individual files so there's still that... :( I believe that the documentation should mention that watching individual files and using filter is not supported on MSW.

[vadz]

The patch looks mostly good but,  as usual, I don't like the use of bool parameters in public API, i.e. in FileExists() and DirExists() and would prefer to use some kind of flags.

In fact, we could make wxFileSystemObject_XXX public and allow calling Exists(wxFileSystemObject_File | wxFileSystemObject_Link) (perhaps with shorter names?) to check for the existence of a link to a file?

Boolean argument is, of course, quite all right for DontFollowSymlinks() itself. I could apply this part of the patch even now if you'd like.

[vadz]

(In #14544) I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

[dghart]

(In #14544) Replying to vadz:

I'll try to look at this series soon although to be honest I'm a bit intimidated. It might be better to redo a series of patches (0001-Something, 0002-SomethingElse, ...) if parts of the later patches change something done by the earlier ones as this should make reviewing them simpler.

I.e. perhaps it would be more convenient for both of us if you created a new branch, cherry pick all your commits there and used git rebase -i to reorganize them in the optimal way and then just used git format-patch?

If not, I'll try to look at this stuff starting from #14488 on Thursday.

I tried to keep the patches clean and simple. With the exception of the unit-test, there are only minor 'internal' reversions in the series; by minor I mean just a couple of lines. So I think you'll find it easier that you expect.

If not, I'll redo them differently.

[dghart]

Replying to vadz:

The patch looks mostly good but,  as usual, I don't like the use of bool parameters in public API

I know :) but I thought this might be an exception, as it's difficult to think of any other flag that might be needed in the future.

In fact, we could make wxFileSystemObject_XXX public and allow calling Exists(wxFileSystemObject_File | wxFileSystemObject_Link) (perhaps with shorter names?) to check for the existence of a link to a file?

I think I'd choose the first way, and I'll change the patch to do that; but let me know if you'd rather use wxFileSystemObject direct.

Boolean argument is, of course, quite all right for DontFollowSymlinks() itself. I could apply this part of the patch even now if you'd like.

Thanks, but it'll be easier for me to redo the whole patch.

[dghart]

I've updated the patch. I had to make the enum public anyway; it's now called 'wxFSItem', which is less descriptive but certainly shorter.

[vadz]

(In #14649) This is definitely pretty bad but we need to apply #14542 first, it contains a general solution to the same problem and it doesn't make much sense to have 2 of them.

[jdagresta]

(In #14649) OK, that makes sense.

I think the changes for #14542 will still not fix the problem where wxFileName::Rmdir() is passed a directory which is itself a symbolic link to another directory.

But it looks like it will fix the "indirect" problem where a real directory passed to Rmdir() contains a symbolic link to a directory, although I would think that a call to DontFollowLink() would have to be added to Rmdir().

In that first case I believe the wxFileName::Rmdir() will either still incorrectly follow the symbolic link by virtue of this code:

wxDir d(path);
if ( !d.IsOpened() )

return false;

or it will end up not deleting the top-level directory symbolic link if d.IsOpened() is false.

So I think something will still need to be changed in wxFileName::Rmdir().

[vadz]

(In #14649) When you call Rmdir() on a symlink to a directory you probably actually want to delete the directory and not the symlink. But just in case you don't, calling DontFollowLink() before doing it should work as expected -- i.e. result in an error because a symlink can't be rmdir'd. Or am I missing something here?

BTW, if you can test the patch from the other ticket, it would be helpful. If you do, please leave any notes about it there (e.g. "it works" or "it breaks XXX"). TIA!

[jdagresta]

(In #14649) The caller of Rmdir() may not know that the "directory" is actually a symlink to a directory, e.g. in a TreeCtrl and/or ListCtrl widget where the directory symlink matches the pattern of files being filtered out in the Tree/List display and the user hits DELETE on such an entry.

In that case, it would be the symlink that you would want deleted and not the directory being linked to (which could be anywhere, including somewhere in the "OS" that you would never want to implicitly delete).

Should the caller always be responsible for testing for a smylink before calling Rmdir()? Well, right now that's not possible because there's no IsLink() (or corresponding) method and adding that responsibility to the caller now implies a potentially large impact as far as searching all Rmdir() calls in all wxWidgets applications.

In my opinion Rmdir() should just be made to do the "right" thing, as in all other ways the directory symlink is (currently) treated as if it is a directory. As far as I can tell, nothing in the #14542 patch explicitly changes this default behavior.

I've already gone ahead and used my proposed changes in our version of wxWidgets to fix our application and patched it to our customers, since I cannot leave our application with the possibility that it will delete linked to files (that may be part of the "OS") and I need Rmdir(RECURSIVE) to work "correctly" whether it's the top-level Rmdir() path that's a symlink or it's a lower-level entry that's a symlink.

This bug was one that existed in MainWin that we used to build our application with before we moved to wxWidgets (and we never moved to the version of MainWin that had a fix because we switched) and I just happened to test it today with wxWidgets - I was disappointed that it was also broken in wxWidgets and had to rush fix it.

I'm not sure when or if I'll have time to try to apply the #14542 patch instead to our version of wxWidgets and test the Rmdir() calls in our application. In any case as I've noted, without further changes to the Rmdir() code itself I already know that it won't work for us (because our application needs Rmdir of a top-level directory symlink to remove the symlink and not generate an error).

[vadz]

(In #1993) We should return to this once #14542 is applied.

[vadz]

I'm going to apply the DontFollowLink() part of this patch but I'm still not totally happy about the changes to {File,Dir,}Exists() so I won't change the static variants of these functions -- just update the member ones to honour the setting of m_dontFollowLinks.

The main reason I'm unhappy about the static ones is pretty stupid but here it is: naming. I don't think wxFSItem is a good name (meaning not for the enum itself, as nobody cares about it, but as the prefix for its elements) but I can't for the life of me come with anything better. So instead of not doing anything, I've decided to apply the important part of the patch right now and add the other one later, when somebody comes up with a good idea about how to call these flags.

[vadz]

(In #14544) Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

The only change I intentionally did (as opposed to any number of bugs I could have introduced unintentionally...) was to change the filespec check to use the full file name instead of just its extension, I don't see any reason why we shouldn't allow watching for e.g. "syslog*".

Please let me know if I broke anything else and thanks again for all your work on this!

[VZ]

(In [72680]) Add support for symlinks to wxFileName.

Allow to work with the symlinks themselves and not the file they reference by
calling the new wxFileName::DontFollowLink().

Update Unix wxDir implementation to not treat symlinks to directories as
directories, this ensures that we don't recurse into the directories outside
of the original parent accidentally.

Closes #14542.

[VZ]

(In #14544) (In [72681]) Check for filespec when generating events in wxFileSystemWatcher.

Instead of setting watches on individual files when a non-empty filespec is
given, always watch all the files but just ignore the events from the ones not
matching the filespec. This makes the code simpler and fixes several bugs.

See #14544.

[VZ]

(In #14544) (In [72682]) Fix bug in Unix wxFileSystemWatcher implementation when watch is deleted.

Don't assert when removing a watch descriptor from the stale descriptors list.

See #14544.

[VZ]

(In #14544) (In [72683]) Handle deletion of watched directories in wxFileSystemWatcher sample.

Don't assert when trying to stop watching a directory that doesn't exist any
more later.

See #14544.

[dghart]

(In #14544) Replying to vadz:

Thanks, I've applied everything here, I think, but I had to do it by combining some patches to avoid breaking unit tests.

Sorry to reopen this, but unless I'm missing something (which is quite possible; the complex series of patches confuses me too) I don't think you applied CreateDelete.diff

[vadz]

(In #14544) Oops, sorry about this and thanks for noticing it. Hopefully nothing else got forgotten, so re-closing it, thanks again.

[VZ]

(In #14544) (In [72689]) Improve inotify()-based wxFileSystemWatcher to handle creation/deletion.

Handle creation and deletion of directories under the watched path better.

See #14544.

[dghart]

Replying to vadz:

The main reason I'm unhappy about the static ones is pretty stupid but here it is: naming. I don't think wxFSItem is a good name (meaning not for the enum itself, as nobody cares about it, but as the prefix for its elements) but I can't for the life of me come with anything better. So instead of not doing anything, I've decided to apply the important part of the patch right now and add the other one later, when somebody comes up with a good idea about how to call these flags.

How about wxFileSystemFlags (or wxFSFlags if you prefer brevity)? Or 'Mode'?

[vadz]

Both are too generic IMHO, and we also already have wxFileSystemOpenFlags which is for (virtual) wxFileSystem and not (physical) wxFileName so it could be also a bit confusing. BTW, we also already have wxFileKind too.

So I guess it would need to be some new wxFileExistsFlags (or, actually, unnamed, because what do we need its name for?) enum with elements wxFILE_EXISTS_FILE (or REGULAR?), wxFILE_EXISTS_DIR, ... and wxFILE_EXISTS_NO_FOLLOW.

Any better suggestions?

[dghart]

Replying to vadz:

Both are too generic IMHO, and we also already have wxFileSystemOpenFlags which is for (virtual) wxFileSystem and not (physical) wxFileName so it could be also a bit confusing. BTW, we also already have wxFileKind too.

OK

So I guess it would need to be some new wxFileExistsFlags (or, actually, unnamed, because what do we need its name for?) enum with elements wxFILE_EXISTS_FILE (or REGULAR?), wxFILE_EXISTS_DIR, ... and wxFILE_EXISTS_NO_FOLLOW.

I'm not sure I understand. If we don't bother with the static functions, do we need an extra flag in the enum at all? Even if so, is there any need to change the names from the anonymous wxFileSystemObject_Foo?

I'm perfectly happy to skip the statics. I added the flag to them for completeness, and because it self-documents the need to consider symlinks; but it's not that much extra work for the user to create a wxFileName object and call DontFollowLink().

Any better suggestions?

I don't dare make the alternative suggestion of reverting to a bool parameter ;)

[vadz]

Replying to dghart:

I'm not sure I understand. If we don't bother with the static functions, do we need an extra flag in the enum at all?

No, we don't, but I do agree that being able to use this functionality with static functions would be useful/convenient/less surprising.

Any better suggestions?

I don't dare make the alternative suggestion of reverting to a bool parameter ;)

Believe it or not, but I did consider it. However I'd also like to add a test for symlink existence as this can be useful and I'd really prefer to do it by adding flags to Exists() rather than by adding yet another LinkExists() so flags would be better here.

And for now the best I can see is to allow

if ( wxFileName::Exists(wxFILE_EXISTS_FILE | wxFILE_EXISTS_NO_FOLLOW) )
  ...

as I wrote above. OTOH it's not without its problems neither as this would mean that the non-static Exists() shouldn't take the same flags because wxFILE_EXISTS_NO_FOLLOW could conflict with m_dontFollowLinks.

So finally perhaps it's indeed better to do nothing for now and maybe add LinkExists() etc (FIFOExists()? `SocketExists()?) later if anybody asks for it.

[dghart]

Replying to vadz:

Believe it or not, but I did consider it. However I'd also like to add a test for symlink existence as this can be useful and I'd really prefer to do it by adding flags to Exists() rather than by adding yet another LinkExists() so flags would be better here.

And for now the best I can see is to allow
{{{
#!cpp
if ( wxFileName::Exists(wxFILE_EXISTS_FILE | wxFILE_EXISTS_NO_FOLLOW) )
...
}}}

Ah, I understand now.

OTOH it's not without its problems neither as this would mean that the non-static Exists() shouldn't take the same flags because wxFILE_EXISTS_NO_FOLLOW could conflict with m_dontFollowLinks.

Using wxFILE_EXISTS_NO_FOLLOW in non-static Exists() sounds to me like a feature, and certainly not a problem. fn(wxFILE_EXISTS_DIR | wxFILE_EXISTS_NO_FOLLOW) would test if fn holds a real dir, not a symlink, without changing fn's default behaviour.

So finally perhaps it's indeed better to do nothing for now and maybe add LinkExists() etc (FIFOExists()? `SocketExists()?) later if anybody asks for it.

I'm happy to implement this, or similar, if you wish. I wonder though if it's worth adding extra functions (FIFOExists() etc) which will seldom be used, to the already function-heavy wxFileName. wxFilename::Exists(wxFILE_EXISTS_FLAGS flag = wxFILE_EXISTS_ANY) is less readable, but allows testing for multiple types.

[vadz]

I do prefer to have a single Exists(flags = ALL) rather than ThisExists() and ThatExists() and so on. If you think it's not a problem to have both NO_FOLLOW flag and DontFollow() method, this is what should probably be done.

[dghart]

Attached is a new patch, filename.diff, that implements the flags parameter. I've combined block and char devices in wxFILE_EXISTS_DEVICE, but they could easily be separated if you prefer.
I've also left a gap before wxFILE_EXISTS_ANY for future entries.

[VZ]

(In [72707]) Allow testing for existence of specific file types in wxFileName.

Add "flags" parameter to wxFileName::Exists() to allow testing for the
existing of files of specific type: not only regular or directory but also
symlink, device, FIFO or socket.

And also to pass wxFILE_EXISTS_NO_FOLLOW flag inhibiting following the
symlinks without using DontFollowLink().

Closes #14542.

[dghart]

As discussed in #14543, the attached wxdir.diff reverts the previous change, and instead makes following symlinks the default. There is a new flag in the wxDirFlags enum to control this.

This patch doesn't touch wxFileName::Rmdir (I'll do that separately). As a result, its use in the FileNameTestCase:929 causes looping; for about 20 seconds on my machine (I don't know what stops it). You may therefore wish to coordinate applying this and a fix for #14649.

[dghart]

(In #14649) The attached rmdir.diff, which depends on the latest #14542 patch, implements my understanding of the above discussion. In particular:

1) with the wxPATH_RMDIR_RECURSIVE flag, internal symlinks will themselves be deleted, not followed.
2) with the wxPATH_RMDIR_RECURSIVE flag, a passed symlink will be deleted, not followed.

The discussion avoided the following situations:

3) wxPATH_RMDIR_FULL, and not wxPATH_RMDIR_RECURSIVE. I've not-followed any symlinks, so unexpected deletions won't occur.
4) What about the situation where the passed 'dir' is a symlink and 1) no flag, 2) only wxPATH_RMDIR_FULL is passed? The patch ignores this situation; is that correct?

FileNameTestCase now succeeds again without any looping. I added a should-fail test for 4)

[jdagresta]

Note: there are two references in the latest wxdir.diff patch just attached to "Note that wxDIR_NO_FOLLOW is relevant only on Linux".

Shouldn't that be a reference to "... only on UNIX" or "... only on UNIX/Linux"?

It applies to other flavors of UNIX than Linux, such as Solaris, AIX, etc., namely any OS that supports symlinks.

[dghart]

Replying to jdagresta:

Shouldn't that be a reference to "... only on UNIX" or "... only on UNIX/Linux"?

Fair enough.

[jdagresta]

(In #14649) I'm not convinced that the latest rmdir.diff patch will stil work correctly for the case where the dir passed to Rmdir() is itself a symlink and should just be removed without affecting anything in the linked-to directory.

Won't the symlink'd to top-level dir be opened by virue of wxDir d(path) and still iterated through deleting any files and directories (whether the iterated files are symlinks or not) - although not recursively following any symlinks?

Or does adding the wxDirNoFollow flag to d.GetFirst() mean that no entry will be matched on wxDIR_DIRS or wxDIR_FILES if it is a symlink? But what about those entries in the top-level symlinked-to directory that are not themselves symlinks? Won't they still get deleted?

One reason that I'm not convinced is that my original proposal (filenamecpp.patch) had an added test for the top-level symlink right at the test (for UNIX) for flags !=0 so that it would not go into the flags != 0 code at all and just fall through to the code that checks to do wxRemoveFile() instead of wxRmdir().

It's not obvious to me that the latest proposed changes with wxdir.diff on #14542, in addition to earlier changes already applied, mean that no "normal" entry in a top-level symlinked-to directory will be matched on wxDIR_DIRS or wxDIR_FILES. That's by looking at the logic, even if wxdir.diff is applied, in the wxDirData::Read(wxString *filename) routine (in src/unix/dir.cpp).

I'm not sure, but it seems like "normal" directories and files inside the top-level symlinked-to directory will still get deleted, while either directory or file symlinks also will still get deleted.

Did you try a test of Rmdir() with a top-level symlink to a directory that itself contains both "normal" files and sub-directories as well as symlinks to files and directories and verify that none of the sub-directories or files or symlinks in the linked-to directory were deleted?

I could easily be wrong, but thought it was worth asking.

[vadz]

(In #14649) Replying to dghart:

The discussion avoided the following situations:
3) wxPATH_RMDIR_FULL, and not wxPATH_RMDIR_RECURSIVE. I've not-followed any symlinks, so unexpected deletions won't occur.

I think this is correct, FULL means to delete all the empty subdirectories (recursively), but do nothing with the files.

4) What about the situation where the passed 'dir' is a symlink and 1) no flag, 2) only wxPATH_RMDIR_FULL is passed? The patch ignores this situation; is that correct?

I think it's correct too because ::wxRmdir() should already fail with ENOTDIR error in this case, which is the expected behaviour.

FileNameTestCase now succeeds again without any looping.

I'm rather confused by this looping problem, I'll need to look into it because I really have no idea where would this be coming from. Do you have any more information about it by chance?

[vadz]

(In #14649) Replying to jdagresta:

I'm not convinced that the latest rmdir.diff patch will stil work correctly for the case where the dir passed to Rmdir() is itself a symlink and should just be removed without affecting anything in the linked-to directory.

The best way to answer this would be to add a test for this for the unit test, if it doesn't already check for it.

[vadz]

Replying to dghart:

This patch doesn't touch wxFileName::Rmdir (I'll do that separately). As a result, its use in the FileNameTestCase:929 causes looping; for about 20 seconds on my machine (I don't know what stops it). You may therefore wish to coordinate applying this and a fix for #14649.

FWIW I don't see any problems like this here...

[VZ]

(In [72739]) Change the way directory iteration flags are constructed.

Instead of explicitly constructing the flags from the flags that should be
included, construct them by excluding the flags that shouldn't be used. This
makes the code more stable in the sense that it will continue to work when new
flags, such as the upcoming wxDIR_NO_FOLLOW, are added.

See #14542.

[VZ]

(In [72740]) Add wxDIR_NO_FOLLOW flag for wxDir iteration.

This flag allows to avoid following the symbolic links during the directory
traversal. In particular, this means that links to the directories
(potentially outside the directory being traversed) are not considered as
directories at all when it is used, potentially avoiding surprises.

Closes #14542.

[VZ]

(In [72741]) Mention wxFILE_EXISTS_NO_FOLLOW in wxFILE_EXISTS_SYMLINK description.

Using wxFILE_EXISTS_SYMLINK without wxFILE_EXISTS_NO_FOLLOW can only be
fruitless, so mention that they should normally be used together in the
documentation.

An alternative solution would be to always add wxFILE_EXISTS_NO_FOLLOW
automatically if wxFILE_EXISTS_SYMLINK is used, perhaps we should do this
instead.

See #14542.

[VZ]

(In #14649) (In [72742]) Don't follow symlinks in wxFileName::Rmdir(wxPATH_RMDIR_RECURSIVE).

Following symlinks, possibly leading outside of the directory being removed,
is at best surprising and at worst dangerous, so don't do it and just mimic
the behaviour of "rm -rf", i.e. remove everything inside this directory,
including the symlinks themselves, but don't follow them.

Closes #14649.

[dghart]

(In #14649) Replying to jdagresta:

I'm not convinced that the latest rmdir.diff patch will stil work correctly for the case where the dir passed to Rmdir() is itself a symlink and should just be removed without affecting anything in the linked-to directory.
// snip
I could easily be wrong, but thought it was worth asking.

No, you were right. However Vadim has corrected the mistake, and I confirm that deleting symlinks is now 'safe'.

Replying to vadz:

I'm rather confused by this looping problem, I'll need to look into it because I really have no idea where would this be coming from. Do you have any more information about it by chance?

It happened (now cured) because wxFileName::Rmdir was following symlinks; in this case the symlink pointed to its parent dir, so Rmdir was called on the link multiple times. Presumably the loop was not infinite because the ELOOP limit was eventually reached.

[dghart]

Replying to VZ:

Using wxFILE_EXISTS_SYMLINK without wxFILE_EXISTS_NO_FOLLOW can only be
fruitless, so mention that they should normally be used together in the
documentation.

An alternative solution would be to always add wxFILE_EXISTS_NO_FOLLOW
automatically if wxFILE_EXISTS_SYMLINK is used, perhaps we should do this
instead.

Yes, I noticed this yesterday and was going to ask about it once the dust had settled.

I can't think of a reason to want wxFILE_EXISTS_SYMLINK without wxFILE_EXISTS_NO_FOLLOW. I'll create a patch.

[oneeyeman]

(In #1993) David,
did you finish work on this ticket by chance?

Thank you.

[dghart]

(In #1993) Replying to oneeyeman:

did you finish work on this ticket by chance?

There aren't any outstanding issues in #14542 afaik. If you're looking at its last comment, that was fixed by #14777.

[oneeyeman]

(In #1993) David, Vadim,

So can this ticket be closed?