wxDirDialog doesn't display symbolic links to directories correctly.

[wxtrac]

Issue migrated from trac ticket # 1993

component: GUI-generic | priority: low | keywords: wxDirDialog symlinks

2004-10-22 20:35:54: @dghart created the issue

---

There is an internal function called wxStat, which for *nix setups is a #define for stat. As a result, functions that rely on it cope badly with symlinks, since wxStat reports on the target file rather than on the symlink itself.

wxDirExists() and wxPathExists() treat a 'symlink pointing to a dir' as though it itself is a dir. wxFileExists() returns false if a symlink exists but is broken. wxCopyFile() ditto (amoungst other problems) wxFileModificationTime() will report the mod-time of the target, not the link.

wxFileName uses wxDirExists() & wxFileExists(), and so behaves similarly.

All this won't matter to most programs, but wxFileDialog and wxGenericDirCtrl display 'symlinks pointing to dirs' as dirs.

Possible fixes: a) Redefine wxStat as lstat. This will solve these problems without side-effects. However the name becomes misleading; and it is just possible that the change might break somebody's code that uses wxStat. b) Invent a new wxLstat, and change the functions to use this. c) Make larger changes, especially to wxCopyFile, so that symlinks are properly dealt with.

I should be able to create patches for a) or b), and I have code that may be helpful for c)

David Hart

[wxtrac]

2005-02-21 03:19:10: @vadz commented

---

I think we need (b) and use wxLstat() in wxDirDialog, wxCopyFile &c. Any patches in this direction would be appreciated!

[wxtrac]

2005-02-25 18:51:38: @dghart commented

---

Hi Vadim

Thank you for your response. I've attached 4 patches, for src/common/filefn.cpp and its header, src/common/filename.cpp and src/unix/snglinst.cpp. I don't know if you will feel that snglinst.cpp needs changing: is there any chance that a symlink could be confused with a valid lock-file? Please ignore this patch if you think it unnecessary.

I smothered all the changes in the other 3 patches with #if defined(UNIX)'s as they are common to all os's, but please make sure I did it right. I've checked that everything builds and works, but only for linux, and not for unicode.

You will see that I couldn't resist the temptation to extend wxCopyFile to copy symlinks correctly (currently it copies the target file, unless the symlink is broken whereupon it fails). As wxRenameFile uses wxCopyFile, this now renames symlinks properly. Again I've checked that it works, but I won't be offended if you don't want to use it.

Regards

David Hart

[wxtrac]

2005-02-25 18:52:57: @dghart uploaded file filefnh.patch (0.6 KiB)

[wxtrac]

2005-02-25 19:04:48: @dghart uploaded file filenamecpp.patch (0.4 KiB)

[wxtrac]

2005-02-25 19:05:35: @dghart uploaded file snglinstcpp.patch (0.3 KiB)

[wxtrac]

2005-08-02 14:50:42: @dghart commented

---

Hi Vadim

It's been 6 months now, and the patch for filefn.cpp no longer works (the others are still OK). I've deleted it and substituted a new one.

I don't think the patches reached the Patch Manager, but if I'm wrong please could you update it there too (or even apply the patches ;-).

Thanks,

David Hart

2005-08-02 14:50:42: @dghart uploaded file filefncpp.patch (3.7 KiB)

Updated patch for filefn.cpp

[wxtrac]

2006-10-21 16:51:31: @vadz commented

---

I'm sorry about (very) late reply, unfortunately I didn't get email notifications for your updates for some reason and so had no idea that you submitted the patches. For the future, it's really better to submit patches to the patch tracker, not the bug tracker, as the former is dealt with much more rapidly.

Anyhow, looking at the patches themselves right now I am not sure if I really agree with them. I did add wxLstat() function (which I also defined as wxStat for non-Unix platforms) as it can be generally useful but I don't think we want to use it unconditionally in functions such as wxFileExists() and wxFileName::GetTimes() because it makes it impossible to check for the existence/times of the file pointed to by the symlink, i.e. we switch from one extremity (always use the link target) to another one (always use the link itself). To make this reasonably acceptable we at the very least need to have a readlink() wrapper too. And wxFileExists() probably shouldn't change at all as it's explicitly used to check for S_ISREG files. For wxCopyFile() I'm simply not sure what should it do when one or both of the arguments are symlinks. IMHO it should work as cp(1) and I don't think your patch does this, does it? In any case, we absolutely need to document this behaviour and so I can't apply the patch without the patch to the docs.

I think we should have wxFileName::IsLink() and ReadLink() functions. But, of course, the main problem remains in wxDirDialog and should be addressed there (and this is why I leave this bug open).

And sorry once again for not replying earlier...

[wxtrac]

2008-05-20 22:22:16: @wojdyr changed component from * to GUI-generic*

[wxtrac]

2008-05-23 03:44:43: @wxtrac changed status from assigned to confirmed

2008-05-23 03:44:43: @wxtrac commented

transitioning old 'assigned' status to new 'confirmed' status

[wxtrac]

2012-02-12 09:11:53: @oneeyeman1 commented

---

See also #910 and #953 for a related problems. Hopefully when those 2 are fixed this one will be easier or even fixed as well. ;-)

[wxtrac]

2012-02-12 17:45:06: @vadz changed title from File-functions and symbolic links to wxDirDialog doesn't display symbolic links to directories correctly.

2012-02-12 17:45:06: @vadz commented

This is related to the other bugs but different as it mainly affects wxDirDialog.

[wxtrac]

2012-03-02 10:22:08: @oneeyeman1 commented

---

From the OP: "wxFileDialog and wxGenericDirCtrl display 'symlinks pointing to dirs' as dirs."

I don't know if this is a problem or not. As a test a followed this procedure.

1. Create a symlink to any directory.
2. Open gedit.
3. Hit Ctrl+O to open the file.
4. Change the directory to the one where the symlink you made in step 1 is.

You will see that it is displayed as a directory.

I didn't test the copying and modification time.

[wxtrac]

2012-03-02 10:22:39: @oneeyeman1 commented

---

Forgot to say: dialogs sample behaves the same way.

[wxtrac]

2012-03-03 00:16:12: @vadz commented

---

I'd really expect the sym links to appear differently from the directories, e.g. have arrow overlay in their icon as junctions do under Windows. But if Gnome really shows them the same perhaps we should do the same thing too...

[wxtrac]

2012-03-03 03:04:34: @oneeyeman1 commented

---

Vadim, Replying to [comment:11 vadz]:

I'd really expect the sym links to appear differently from the directories, e.g. have arrow overlay in their icon as junctions do under Windows. But if Gnome really shows them the same perhaps we should do the same thing too...

Yes, the dialogs sample and the gedit "file open" dialog behaves the same in this respect. However, I don't know if the GTK+ 3 behavior will be different.

So it is time to test the other 2 problems...

[wxtrac]

2012-03-03 22:48:49: @oneeyeman1 commented

---

Vadim, Replying to [comment:11 vadz]:

I'd really expect the sym links to appear differently from the directories, e.g. have arrow overlay in their icon as junctions do under Windows. But if Gnome really shows them the same perhaps we should do the same thing too...

I should also note that if I go to "Places->Home" where I made the link to the directory (i.e. open "Windows Explorer") the link is displayed with the arrow. But in the case of File/Dir Open dialog it is plain folder.

[wxtrac]

2012-03-11 10:21:42: @oneeyeman1 commented

---

wxFileModificationTime() is still uses wxStat. In order to fix it:

src/common/filename.cpp:

2664: #elif defined(__UNIX_LIKE__) || defined(__WXMAC__) || defined(__OS2__) || (defined(__DOS__) && defined(__WATCOMC__))
    // no need to test for IsDir() here
    wxStructStat stBuf;
-    if ( wxStat( GetFullPath(), &stBuf) == 0 )
+    if( wxLstat( GetFullPath(), &stBuf) == 0 )
    {
        // Android defines st_*time fields as unsigned long, but time_t as long,
        // hence the static_casts.
        if ( dtAccess )
            dtAccess->Set(static_cast<time_t>(stBuf.st_atime));
        if ( dtMod )
            dtMod->Set(static_cast<time_t>(stBuf.st_mtime));

And it would be nice to have a unit test for it...

[wxtrac]

2012-03-11 10:43:17: @oneeyeman1 commented

---

Replying to [comment:14 oneeyeman]:

wxFileModificationTime() is still uses wxStat. In order to fix it:

src/common/filename.cpp:

2664: #elif defined(__UNIX_LIKE__) || defined(__WXMAC__) || defined(__OS2__) || (defined(__DOS__) && defined(__WATCOMC__))
    // no need to test for IsDir() here
    wxStructStat stBuf;
-    if ( wxStat( GetFullPath(), &stBuf) == 0 )
+    if( wxLstat( GetFullPath(), &stBuf) == 0 )
    {
        // Android defines st_*time fields as unsigned long, but time_t as long,
        // hence the static_casts.
        if ( dtAccess )
            dtAccess->Set(static_cast<time_t>(stBuf.st_atime));
        if ( dtMod )
            dtMod->Set(static_cast<time_t>(stBuf.st_mtime));

And it would be nice to have a unit test for it... And it probably worth mentioning in the docs somewhere.

[wxtrac]

2012-03-11 10:51:07: @oneeyeman1 commented

---

wxCopyFile() is also still uses wxStat in src/common/filefn.cpp, to check if we can open the file, but I didn't check if what will be copied yet.

[wxtrac]

2012-03-11 11:05:21: @vadz changed status from confirmed to new

2012-03-11 11:05:21: @vadz commented

FWIW I don't think it's a good idea to just change all functions using wxStat() to use wxLstat(), it's not really obvious that the latter is more correct in general.

[wxtrac]

2012-03-11 20:28:20: @oneeyeman1 commented

---

Vadim, Replying to [comment:17 vadz]:

FWIW I don't think it's a good idea to just change all functions using wxStat() to use wxLstat(), it's not really obvious that the latter is more correct in general. Do you agree that some function, presumably wxFileModificationTime() needs to be changed? We will do it one function at a time by testing current behavior.

[wxtrac]

2012-03-12 00:02:58: @vadz commented

---

Mo, I don't see anything special about this one. Why would you need the modification time of the symlink itself instead of the file it points to? In most cases you're clearly interested in the latter. In some specific cases you may need the former but I don't think it should be the default behaviour.

[wxtrac]

2012-04-10 10:09:31: @oneeyeman1 commented

---

Vadim, Out of all functions that is mentioned in the original report which one should be changed? If we can agree that some functions needs to be changed and decide which one it would be great. Of course the default behavior will be the one that is done right now.

[wxtrac]

2012-09-21 02:45:50: @oneeyeman1 commented

---

Vadim, Which functions here needs to be changed?

Mo, I don't see anything special about this one. Why would you need the modification time of the symlink itself instead of the file it points to?

But then why not check the file itself?

[wxtrac]

2012-09-21 11:56:34: @vadz commented

---

We should return to this once #14542 is applied.

[wxtrac]

2012-10-15 02:21:09: @vadz commented

---

(In #14542) 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.

[wxtrac]

2012-10-15 03:09:02: @vadz commented

---

(In #14542) (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.

[wxtrac]

2012-10-16 16:27:27: @dghart commented

---

(In #14542) Replying to [comment:13 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'?

[wxtrac]

2012-10-16 17:24:53: @vadz commented

---

(In #14542) 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?

[wxtrac]

2012-10-16 19:14:36: @dghart commented

---

(In #14542) Replying to [comment:23 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 ;)

[wxtrac]

2012-10-17 00:27:21: @vadz commented

---

(In #14542) Replying to [comment:24 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

#!cpp
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.

[wxtrac]

2012-10-17 12:13:30: @dghart commented

---

(In #14542) Replying to [comment:25 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.

[wxtrac]

2012-10-17 17:42:49: @vadz commented

---

(In #14542) 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.

[wxtrac]

2012-10-19 14:17:53: @dghart commented

---

(In #14542) 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.

[wxtrac]

2012-10-20 00:02:16: @vadz commented

---

(In #14542) (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.

[wxtrac]

2012-10-23 18:15:53: @dghart commented

---

(In #14542) 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.

[wxtrac]

2012-10-23 23:09:18: jdagresta (Jonathan Dagresta) commented

---

(In #14542) 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.

[wxtrac]

2012-10-23 23:25:44: @dghart commented

---

(In #14542) Replying to [comment:32 jdagresta]:

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

Fair enough.

[wxtrac]

2012-10-24 01:22:52: @vadz commented

---

(In #14542) Replying to [comment:30 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...

[wxtrac]

2012-10-24 01:57:08: @vadz commented

---

(In #14542) (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.

[wxtrac]

2012-10-24 01:57:33: @vadz commented

---

(In #14542) (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.

[wxtrac]

2012-10-24 01:57:55: @vadz commented

---

(In #14542) (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.

[wxtrac]

2012-10-24 13:18:15: @dghart commented

---

(In #14542) Replying to [comment:40 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.

[wxtrac]

2012-12-13 00:45:24: @oneeyeman1 commented

---

David, did you finish work on this ticket by chance?

Thank you.

[wxtrac]

2012-12-13 11:06:13: @dghart commented

---

Replying to [comment:41 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.

[wxtrac]

2012-12-20 03:27:17: @oneeyeman1 commented

---

David, Vadim,

So can this ticket be closed?

[wxtrac]

2012-12-22 01:38:40: @vadz changed status from new to confirmed

2012-12-22 01:38:40: @vadz commented

AFAIK it was never fixed, i.e. symlinks to the directories still must appear as directories in the generic wxDirDialog. We do have the possibility to fix it now, but, again, this wasn't done yet unless I missed it somehow.

[wxtrac]

2014-08-25 01:58:32: @oneeyeman1 uploaded file directory bug.png (226.2 KiB)

Screenshot for the bug

[wxtrac]

2014-08-25 02:00:25: @oneeyeman1 commented

---

Vadim, According to the screenshot, the /usr/src/linux, which is just a symlink to the kernel directory showing up as directory.

Tested on Gentoo Linux against GTK+-2.24.23.

I think this can be closed.

[wxtrac]

2014-09-03 18:34:19: neis (Stefan Neis) commented

---

Sorry? Wasn't the point of the original report exactly that the symlink shows up as directory, which it shouldn't? And aren't you just confirming that this is still the case? So why should this be closed?