win: make uv_exepath resolve symbolic links by TomCrypto · Pull Request #293 · libuv/libuv

TomCrypto · 2015-03-27T05:33:19Z

This is a draft of a possible fix to #206. It uses GetModuleFileNameW as before to get some kind of path to the executable, and then gets a handle to the file and resolves the full path (without any symlinks) using GetFinalPathNameByHandleW. It's not finished, of course, I have a few questions. With this commit the function will read through symbolic links and return resolved paths like:

\\?\C:\libuv\Debug\run-tests.exe

Which are completely valid (and fully resolved) Windows paths, but the VOLUME_NAME_DOS flag used means that the file actually needs to be on a mounted drive with an assigned drive letter or the function will fail with ERROR_PATH_NOT_FOUND, as MSDN states. The file could be on a mounted drive with no drive letter assigned, and in this case I assume it could be addressed via its GUID (via VOLUME_NAME_GUID) as a long UNC path. But is this realistic?

Basically should the function return "usual" paths like C:\path\to\executable.exe or should it return full-blown UNC paths (which, by the way, apparently can be slightly longer than 32768 characters)? Or whichever is shorter? Does anyone have an opinion on this?

I mean, logically the function should never fail (besides out of resource errors) since the executable always exists somewhere, but this could be a faulty assumption?

EDIT: actually I noticed that the exepath test expects the passed buffer to be filled and truncated if it is too short, so since uv_exepath is a two-step process now it might be necessary to allocate 32KB of memory upfront to reliably get the initial path and then have the GetFinalPathNameByHandleW function handle the truncation for us, assuming it does (since the expanded full path may be longer than the symlink). I'll need to look into that.

saghul · 2015-03-27T07:42:00Z

Maybe we should be more deffensive here? Looks like this function could return ERROR_INSUFFICIENT_BUFFER. Also, these is some remark for Windows XP, which we (sadly) still support.

Well, the function returns a DWORD, which is unsigned, so I'm not sure what the less than zero part was meant to signify? As I understand it the function returns zero on error and puts an explicit error code, such as ERROR_INSUFFICIENT_BUFFER, to be retrieved by GetLastError().

We should definitely make sure that the code works well for Windows XP, especially since these functions tend to behave strangely on XP with respect to null termination (unfortunately).

Oh, reading the documentation again I see what you mean. Yes, I agree there should be a check for ERROR_INSUFFICIENT_BUFFER error case here. Taking note.

saghul · 2015-03-27T07:44:09Z

/cc @piscisaureus

TomCrypto · 2015-04-02T03:18:53Z

Okay, I improved it a bit, and now it works reliably and also adheres to the semantics inferred from the last few asserts of the test-get-currentexe test when the input buffer is too short to hold the executable path (unless I messed up).

However the current test has some code that (I quote) "verify that the path returned from uv_exepath is a subdirectory of executable_path". This seems incredibly brittle since not only is executable_path equal to argv[0], the irregular nature of fully qualified windows paths (with the \\?\ prefix for instance) makes that part of the test fail anyway.

Do you think it is it worth trying to work around the problem by e.g. stripping \\?\ (if possible) or having a special case for Windows and whatnot, or should the test perhaps be rewritten to use a more "natural" validation measure, perhaps a three-step process that:

checks the returned path is not a relative path [uv_exepath should return an absolute path]
stats the returned path to check it contains no symlinks [uv_exepath should resolve symlinks]
tries to execute the program at the returned path with some command-line argument that the test runner understands, and checks the output/exit code to validate the path is in fact the path to the executable [uv_exepath should return a path to the executable]

Together with the invalid parameter asserts I believe this should fully verify all the postconditions that the function should meet and would be more robust, though a little bit more work, than the current version.

Opinions?

saghul · 2015-04-02T06:54:09Z

@TomCrypto your suggestions sound great to me! Bert (@piscisaureus) is our Chief Windows Officer, maybe he has some extra thoughts.

piscisaureus · 2015-04-02T17:12:53Z

Why didn't you just set all share flags (FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE) ?

Good point, I actually just lifted the parameters to CreateFile straight from the MSDN page for GetFinalPathNameByHandle thinking they would be appropriate, but I agree now they are not very permissive in the context of opening one's own executable file. Changing them.

piscisaureus · 2015-04-02T17:32:03Z

When possible, try to return a DOS path. If there is no valid DOS path you could fall back to providing a GUID path, or just returning the GetModuleFileName result without further processing - I don't have a strong opinion about it.
My preference would be to chip off the \\?\ or \\?\UNC\ prefix if the file resides on device that has a DOS path or a mapped network drive path.

Another thing that worries me a little bit is that opening the .exe file may not succeed, so I'd like to be as defensive as possible. I spent some time checking whether there is a method to obtain the HANDLE to the primary section that was used to create the process, but it seems that there's no easy method for that. So I guess this strategy will have to do.

In general the patch looks well-written and clean to me. Good work!

TomCrypto · 2015-04-03T00:36:05Z

Thanks! For the fallback, I think falling back to GUID is best. After all if the executable isn't on a mapped drive it's already a weird situation IMHO. If everything fails I guess the result of GetModuleFileName could be used, but this kind of best-effort behaviour should be documented for users.

About stripping \\?\, do we do that in every case or only if the DOS path is less than MAX_PATH characters in length? The documentation is a bit thick but suggests that paths longer than that actually need the prefix to be further used in Windows (unicode) API's?

Added some helper functions.

TomCrypto · 2015-04-03T06:20:17Z

Rewrote the function to be able to handle fallback, now it does the following:

Gets the path to the module using GetModuleFileNameW. If that actually fails, returns an error.
Attempts to open the .exe file at that path and get a handle to it. If that fails, returns the path to the module (and returns success).
Attempts to get a full DOS path to said handle using GetFinalPathByHandleW. If that fails because there is no such path, attempts to get a GUID path instead. If that fails, or if getting a DOS path fails for any other reason, returns the path to the module (and returns "success").
If it is returning the path to the module or a DOS path (not a GUID path), strips the \\?\UNC\ or \\?\ prefix from the start of the path, whichever one is longer.

That means that as long as GetModuleFileNameW succeeds, the function will at least return something sensible, but I don't know if that behaviour is wise. If that doesn't seem reasonable after some more thought, it's easy to tweak or revert :-)

piscisaureus · 2015-07-08T21:41:36Z

GetFinalPathNameByHandleW isn't supported in Windows XP/2k3. I'm fine with landing this after we officially drop support for them.

saghul · 2015-12-14T20:40:53Z

We now use GetFinalPathNameByHandleW in uv_fs_realpath, I guess we could conditionally use it now, and just don't do it in XP.

saghul · 2016-08-26T10:16:54Z

I'm going to close this one. We use GetFinalPathNameByHandleW in uv_fs_realpath, but there are many corner cases, I do not want to open pandora's box again... For reference see nodejs/node#7726

win: make uv_exepath resolve symbolic links

cbf596b

saghul reviewed Mar 27, 2015
View reviewed changes

saghul added the windows label Mar 27, 2015

win: improve uv_exepath to be more defensive, and add comments

242ca75

piscisaureus reviewed Apr 2, 2015
View reviewed changes

TomCrypto added 2 commits April 3, 2015 15:30

Better flags to CreateFileW

a99201e

Refactored uv_exepath to handle fallback.

f546019

Added some helper functions.

saghul added this to the next milestone Jun 15, 2015

saghul removed this from the 1.7.0 milestone Jul 8, 2015

saghul mentioned this pull request Dec 14, 2015

Windows: uv_exepath does not return resolved symbolic link #206

Closed

saper mentioned this pull request Jan 5, 2016

Accept process.execPath as-is sass/node-sass#1325

Merged

saghul closed this Aug 26, 2016

joaocgreis mentioned this pull request Dec 6, 2016

test: check that process.execPath is a realpath nodejs/node#9229

Closed

3 tasks done

Conversation

TomCrypto commented Mar 27, 2015

Uh oh!

saghul Mar 27, 2015

Choose a reason for hiding this comment

Uh oh!

TomCrypto Mar 27, 2015

Choose a reason for hiding this comment

Uh oh!

TomCrypto Mar 27, 2015

Choose a reason for hiding this comment

Uh oh!

saghul Mar 27, 2015

Choose a reason for hiding this comment

Uh oh!

saghul commented Mar 27, 2015

Uh oh!

TomCrypto commented Apr 2, 2015

Uh oh!

saghul commented Apr 2, 2015

Uh oh!

piscisaureus Apr 2, 2015

Choose a reason for hiding this comment

Uh oh!

TomCrypto Apr 3, 2015

Choose a reason for hiding this comment

Uh oh!

piscisaureus commented Apr 2, 2015

Uh oh!

TomCrypto commented Apr 3, 2015

Uh oh!

TomCrypto commented Apr 3, 2015

Uh oh!

piscisaureus commented Jul 8, 2015

Uh oh!

saghul commented Dec 14, 2015

Uh oh!

saghul commented Aug 26, 2016

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants