Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit the buffer size for compatibility with previous versions of Windows #246

Closed

Conversation

olavobelloc
Copy link

Windows API functions NtQueryDirectoryFile and GetFileInformationByHandleEx can fail with ERROR_INVALID_PARAMETER if the provided buffer is greater than 64k, and the file handle refers to a directory located on a network share, and the shared folder is itself in the local computer. This was reproduced on Windows 7 and 8.1, but not on a early release of Windows 10 (v2004).

I could not find any documentation regarding this issue, apart from a brief comment on another open-source project:
https://github.com/FarGroup/FarManager/blob/master/far/platform.fs.cpp#L620

The argument to have a buffer greater than 64k is based on the claim that ReFS supports filenames up to 32768 UTF-16 characters, but I could not find any documentation supporting this claim. In contrast, I found this Microsoft
documentation that mentions the maximum filename length to be 255 for both NTFS and ReFS.

Therefore, I believe 64k should be enough.

…dows

The reported error was reproduced on Windows 7 and 8.1, but not on an early version of Windows 10.
@Lastique
Copy link
Member

Lastique commented Aug 8, 2022

The filename limit of 32768 characters is mentioned here and also somewhat confusingly here. The last link says both 32K and 255, and 255 seems to be the limit inherited from the implementation bits shared with NTFS. I haven't found the description of the on-disk format to see the actual limit of the format, so I'm guessing the on-disk format allows 32K but the implementation and API allows 255 characters. If this is the case, limiting the buffer might prohibit some very long filenames on Windows versions that do actually support the 32K limit.

@olavobelloc
Copy link
Author

I also cannot tell for sure.

But, the first link you mentioned does not look very "precise" in it's terms. The mentioned paragraph seems to be talking about path limits, not file names. It starts by saying "On an NTFS file system, file paths are limited to 255 characters..." and then draws a comparison with ReFS, and then finishes by providing a link on how to remove the path limit on Windows 10. So, it really seems to me that the term "file name", when referring to ReFS in this context, refers to something like a complete file name, including the path itself. Otherwise, I guess the comparison does not make much sense - but this is just my 2c.

Regarding the second link, it cannot be used as an argument either way. =) To be honest, it's indeed interesting the sentence "for compatibility this was made consistent with NTFS for the RTM product", which seems to indicate that they had deliberately limited the filename to 255 for the ReFS product release.

So, I would honestly consider this documentation link as good of an evidence as the other ones.

I would also say that the current implementation breaks on somewhat common use cases, like accessing network shares on Windows 7/8.1, in exchange of supporting a somewhat obscure use case (which might not even exist). For instance, we already have a reproducible issue on Windows 7/8.1 - we could fix that, and then wait until we are sure there is really an issue with ReFS (like another pull request/bug report), instead of doing it the other way around..

I hope I am making sense, thx.

@Lastique Lastique closed this in 9c9d127 Aug 8, 2022
@Lastique
Copy link
Member

Lastique commented Aug 8, 2022

Thank you for the contribution. I've committed a slightly updated version of the fix (changed comment and added a release note).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants