Don't allow bad directory entry to break entire directory listing

[johntyner]

Under POSIX, the readdir() and getattr() functions are separate, and programs like ls can convey to the user that an entry was found in the first but caused an error in the second. Under WinFSP, these two are combined into a single ReadDirectory/QueryDirectory function, and an error returned by (a single call to) getattr() causes the entire operation to fail.

This change modifies the ReadDirectory/FixDirInfo functions in the FUSE interface to handle an error returned from getattr() by zeroing the file information and marking the file as hidden. By doing this, other entries in the directory may still be returned correctly for display by the calling program.

---

Before submitting this PR please review this checklist. Ideally all checkmarks should be checked upon submitting. (Use an x inside square brackets like so: [x])

• Contributing: You MUST read and be willing to accept the CONTRIBUTOR AGREEMENT. The agreement gives joint copyright interests in your contributions to you and the original WinFsp author. If you have already accepted the CONTRIBUTOR AGREEMENT you do not need to do so again.
• Topic branch: Avoid creating the PR off the master branch of your fork. Consider creating a topic branch and request a pull from that. This allows you to add commits to the master branch of your fork without affecting this PR.
• No tabs: Consistently use SPACES everywhere. NO TABS, unless the file format requires it (e.g. Makefile).
• Style: Follow the same code style as the rest of the project.
• Tests: Include tests to the extent that it is possible, especially if you add a new feature.
• Quality: Your design and code should be of high quality and something that you are proud of.

[billziss-gh]

@johntyner I have seen your PR and will try to act on it in the weekend.

[johntyner]

@billziss-gh, have you had a chance to come back and look this over?

[billziss-gh]

@johntyner sorry for the delay. I am very busy with a private project and have not had time to look into this more closely.

I will have a look at it now.

[billziss-gh]

Ok, I looked at this and I understand what you are trying to do:

• I think the overall idea of removing the faulty directory entry from the listing is a good one.

• A good way to do this would be to reach into the directory buffer and completely remove the directory entry. However this is non-trivial in the current design.

• Perhaps a better way would be to merge AddDirInfo and FixDirInfo so that a faulty entry is never added to the listing. However I am not certain why the split happened in the first place; perhaps it was historical, perhaps there were good reasons for it.

As it stands your simple approach is a good one. However I would like for us to choose a sensible default FileInfo. I am also not certain about the idea of marking it with FILE_ATTRIBUTE_HIDDEN as I think users may find this surprising.

Along the lines of this I propose a default FileInfo as follows:

FILETIME FileTime;
GetSystemTimeAsFileTime(&FileTime);

FileAttributes = 0;
ReparseTag = 0;
AllocationSize = 0;
FileSize = 0;
CreationTime = *(PUINT64)&FileTime;
LastAccessTime = *(PUINT64)&FileTime;
LastWriteTime = *(PUINT64)&FileTime;
ChangeTime = *(PUINT64)&FileTime;
IndexNumber = 0;
HardLinks = 0;
EaSize = 0;

Instead of FILE_ATTRIBUTE_HIDDEN we could consider some "exotic" attributes such as FILE_ATTRIBUTE_OFFLINE although I suspect that the best approach might be to just set the attributes to 0.

I also considered changing the file name to indicate an error, but it feels very heavy-handed and prone to problems.

Thoughts?

[johntyner]

If the difference is between not seeing any files and seeing some files correctly and others marked as hidden, I don't think adding the "hidden" (or other) attribute would be detrimental. I don't have strong feelings on this point; my thought was to hide the file in Explorer rather than surprise the user with a file with a timestamp in the 1600's. If the user has already enabled the option to view hidden files, they're probably the kind of user that understands that they're going to "see some things" with that option enabled.

For the timestamp, I think we should choose some fixed point in time rather than using the current system time. My goal here is to convey some set of attributes that a higher level application could use to determine that the underlying file system is having issues with a file. Again, I don't have strong feelings on what those attributes should be, but using the current time would present a moving target.

--

I considered some of the same alternatives you suggested. For background, the issue that caused this was that we created a file in a directory which appeared for a second and then disappeared when Explorer was refreshed. When we went to recreate the file, we got an error that the file existed. This was caused by the fact that another file in the directory was causing issues with the getattr() call (for reasons beyond the scope of this PR). All of this is to say, I opted not to change the names of the files or attempt to remove the entries from the directory listing because the files are, in fact, there and saying that a file exists when Explorer is clearly showing that it's not is likely to be even more confusing.

[billziss-gh]

@johntyner I have prepared commit b4c39f6, which should resolve this issue.

The commit adds the capability to invalidate directory entries and uses it in FixDirInfo to invalidate entries for which getattr has failed.

Let me know if it looks like this commit would satisfy your original request (albeit not exactly in the manner that you wanted).

---

A few days ago I wrote:

Perhaps a better way would be to merge AddDirInfo and FixDirInfo so that a faulty entry is never added to the listing. However I am not certain why the split happened in the first place; perhaps it was historical, perhaps there were good reasons for it.

Actually this would have been a bad idea. The reason for the split, as I recently recalled, is to avoid reentry into the file system. Consider that AddDirInfo is called from within the file system's readdir, and therefore should not call any other file system operations (including getattr) to avoid reentering the file system.

[johntyner]

Your commit solves our issue, but I'm not fond of hiding the error at the application level. My original PR was targeted at conveying to the application/user that something was amiss if not via error code, then at least in the content.

I can filter out the specific issues causing this problem in our use case, and we can leave WinFSP proper as it is. I'd much rather tackle the issue that way than to hide the error code for your other users who may encounter this condition.

Feel free to close this PR or leave it open if you want to let other ideas percolate.

[billziss-gh]

Your commit solves our issue, but I'm not fond of hiding the error at the application level. My original PR was targeted at conveying to the application/user that something was amiss if not via error code, then at least in the content.

I understand your argument, but let me present some counter arguments:

• The implementation already culls incorrect looking directory entries. See for example here.

• Returning a default directory entry has its own issues:

  • If the getattr failure is only temporary then we may run into an issue where a directory is presented as a regular file (because of the default file attributes that we use when getattr fails), which can cause confusion for users. (Potential workaround for this is to use the directory bit from the filldir stat buffer, when that is available.)
  • If the getattr failure is also (semi-)permanent then we run into the issue of a file that is available via a directory listing, but cannot be opened.

• I find that the PR behavior might be useful for file system developers, but confusing for regular users.

Perhaps a good compromise would be to implement the behavior in the new commit, but also log an error in the event log and/or output a debug message. File system developers could use this message for debugging purposes.

Thoughts?

[johntyner]

Perhaps a good compromise would be to implement the behavior in the new commit, but also log an error in the event log and/or output a debug message. File system developers could use this message for debugging purposes.

This seems reasonable. I didn't realize the existing implementation already removed certain kinds of problematic entries, and I didn't want to be responsible for introducing that kind of functionality in a way that might hide errors/issues. Knowing that it already exists, I think your solution is a good one, especially with the log/debug output.

Thanks for your work on this one.

[billziss-gh]

Commit 9066338 adds debug logging for invalid/ignored directory entries. Only the first 5 such entries will be logged.

Debug logs go to debug output (which can be viewed with e.g. Visual Studio or dbgview) by default. If you specify the -d option then debug logs go to stderr or a file.

[billziss-gh]

I have merged the aforementioned changes into master. Thank you for the PR and your thoughts on this.