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

Use GetFinalPathNameByHandleW instead of GetLongPathNameW to get full path #606

Merged
merged 1 commit into from
Jan 21, 2016

Conversation

skvoboo
Copy link

@skvoboo skvoboo commented Jan 15, 2016

Pull request for issue #598

@skvoboo skvoboo changed the title Use GetFinalPathNameByHandleW to get full path instead of GetLongPathNameW Use GetFinalPathNameByHandleW instead of GetLongPathNameW to get full path Jan 15, 2016
@skvoboo
Copy link
Author

skvoboo commented Jan 15, 2016

The string that is returned by GetFinalPathByHandle() function uses the \?\ syntax.
Call to normalize_ntpath() function is required.

@dscho
Copy link
Member

dscho commented Jan 15, 2016

The string that is returned by GetFinalPathByHandle() function uses the ?\ syntax.

I know that, thanks.

Call to normalize_ntpath() function is required.

This is not a good explanation! It states something out of the blue, does not address any of my suggestions, and does not even try to explain why.

@skvoboo
Copy link
Author

skvoboo commented Jan 18, 2016

Commit has been updated.

@skvoboo
Copy link
Author

skvoboo commented Jan 19, 2016

GetLongPathName() is not faster then CreateFile() + GetFinalPathNameByHandle() because of GetLongPathName() internally uses FindFirstFile() for each next short part of path.

GetLongPathName():
getlongpathname

CreateFile() + GetFinalPathNameByHandle():
getfinalpathnamebyhandle

@dscho
Copy link
Member

dscho commented Jan 19, 2016

Hrm. You could have left more work for me to analyze those data, but not much...

@dscho
Copy link
Member

dscho commented Jan 19, 2016

... and I just realized that you also kept the information to yourself that the normalize_nt_path() is necessary because GetFinalPathNameByHandle() returns a path prefixed with \\?\!!! Why do you hold back such important information? Now I have to wonder what other crucial tidbits are missing, and I am worrying about the soundness of the entire approach. Not good. Not good at all.


if (INIT_PROC_ADDR(GetFinalPathNameByHandleW)) {
HANDLE hnd = CreateFileW(cwd, 0,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,

This comment was marked as off-topic.

This comment was marked as off-topic.

@dscho
Copy link
Member

dscho commented Jan 19, 2016

In the meantime I found reports that GetFinalPathNameByHandle() can fail on empty directories (although it does not do that on Windows 10 in my hands), and that it can hang indefinitely (although I could not get it to do that here, either).

In all, I am much more reluctant about this change now. There just have been many unexpected issues with it which makes me expect more to come. And all of that for only an obscure use case where somebody works from within a directory whose parent directory they have no business listing.

I prefer to have substantially less doubts about Pull Requests. Let's see whether you can dispel them.

@skvoboo
Copy link
Author

skvoboo commented Jan 19, 2016

GetFinalPathNameByHandle() does not fall on empty directories on my Windows 2012 R2 too.
GetFinalPathNameByHandle() hangs on pipe handle which opened in blocking mode.

Are your doubts dispelled?

@dscho
Copy link
Member

dscho commented Jan 19, 2016

Are your doubts dispelled?

No. In fact, now I am even more doubtful, because I would have at least expected you to come up with a link to authoritative documentation that states clearly what one can expect on which Windows versions.

Now I am more certain than before that you did not look whether it works on all Windows versions we try to support, you probably just tested this patch in your setup and did not care whether it works elsewhere, too.

I have been bitten too often in the past by accepting patches in too good faith only to find out that I was left with fixing users' broken setups.

There are two ways forward with this Pull Request:

  1. You improve the commit message of the patch so as to be stellarly convincing, reversing the increasing doubts I have right now.
  2. I will eventually come back to this topic when I have time and research whether it is safe, whether the parameters are all indeed correct, decide what should be the fallback and under which circumstances to fall back right away, and document all of that in the commit message. I would be okay to do that, and it would just take a couple of weeks or months to clear my plate enough.

`GetLongPathName()` function may fail when it is unable to query
the parent directory of a path component to determine the long name
for that component. It happens, because of it uses `FindFirstFile()`
function for each next short part of path. The `FindFirstFile()`
requires `List Directory` and `Synchronize` desired access for a calling
process.

In case of lacking such permission for some part of path,
the `GetLongPathName()` returns 0 as result and `GetLastError()`
returns ERROR_ACCESS_DENIED.

`GetFinalPathNameByHandle()` function can help in such cases, because
it requires `Read Attributes` and `Synchronize` desired access to the
target path only.

The `GetFinalPathNameByHandle()` function was introduced on
`Windows Server 2008/Windows Vista`. So we need to load it dynamically.

`CreateFile()` parameters:
    `lpFileName` = path to the current directory
    `dwDesiredAccess` = 0 (it means `Read Attributes` and `Synchronize`)
    `dwShareMode` = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE
                    (it prevents `Sharing Violation`)
    `lpSecurityAttributes` = NULL (default security attributes)
    `dwCreationDisposition` = OPEN_EXISTING
                              (required to obtain a directory handle)
    `dwFlagsAndAttributes` = FILE_FLAG_BACKUP_SEMANTICS
                             (required to obtain a directory handle)
    `hTemplateFile` = NULL (when opening an existing file or directory,
                            `CreateFile` ignores this parameter)

The string that is returned by `GetFinalPathNameByHandle()` function
uses the \\?\ syntax. To skip the prefix and convert backslashes
to slashes, the `normalize_ntpath()` mingw function will be used.

Note: `GetFinalPathNameByHandle()` function returns a final path.
It is the path that is returned when a path is fully resolved.
For example, for a symbolic link named "C:\tmp\mydir" that points to
"D:\yourdir", the final path would be "D:\yourdir".

Signed-off-by: Anton Serbulov <aserbulov@plesk.com>
@skvoboo
Copy link
Author

skvoboo commented Jan 21, 2016

Commit has been updated.

@dscho
Copy link
Member

dscho commented Jan 21, 2016

Thank you, @skvoboo this commit was a real pleasure to read and indeed managed to reinstate my trust in it, in particular because it simply cannot cause regressions due to handling only the case when it would have failed earlier.

dscho added a commit that referenced this pull request Jan 21, 2016
Use GetFinalPathNameByHandleW instead of GetLongPathNameW to get full path
@dscho dscho merged commit a41b148 into git-for-windows:master Jan 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants