follow symlinks in the Win32 code for System.Environment.getExecutablePath
ClosedPublic

Authored by alpmestan on Nov 23 2017, 7:44 AM.

Details

Summary

This partially addresses Trac #14483 by fixing the Windows implementation of
System.Environment.getExecutablePath. This is achieved by using
GetFinalPathNameByHandleW to resolve potential symlinks, while making sure
we do not get back a UNC path (see Trac #14460).

Test Plan

Validate

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
alpmestan created this revision.Nov 23 2017, 7:44 AM
alpmestan removed subscribers: rwbarton, thomie.
angerman added inline comments.Nov 23 2017, 7:50 AM
libraries/base/System/Environment/ExecutablePath.hsc
40

We used to import System.Posix.Internals on mingw32_HOST_OS?

alpmestan added inline comments.Nov 23 2017, 7:57 AM
libraries/base/System/Environment/ExecutablePath.hsc
40

Yes, apparently. I removed it to silence a redundant import warning.

bgamari requested changes to this revision.Nov 23 2017, 8:46 AM
bgamari added inline comments.
libraries/base/System/Environment/ExecutablePath.hsc
63

We should document this change in semantics on Windows.

This revision now requires changes to proceed.Nov 23 2017, 8:46 AM
alpmestan updated this revision to Diff 14780.Nov 23 2017, 9:11 AM
  • document the new behaviour of 'getExecutablePath' on Windows
alpmestan added inline comments.Nov 23 2017, 9:12 AM
libraries/base/System/Environment/ExecutablePath.hsc
63

Done. I'll let you tick the 'Done' box if/when you're happy with the description.

Phyx added inline comments.Nov 23 2017, 4:38 PM
libraries/base/System/Environment/ExecutablePath.hsc
168

drop the visual studio version (v=vs.85) to make the link stabler.

176

The other patch with the UNC fix changed this to FILE_NAME_OPENED so 8.

190

This comment is inaccurate, device paths \\?\ paths are exempt from MAX_PATH as MAX_PATH is an API level limitation, \\?\ means pass it directly to the underlying filesystem, as such, API limits don't apply.

alpmestan updated this revision to Diff 14790.Nov 24 2017, 3:42 AM
  • address Phyx's comments
alpmestan added inline comments.Nov 24 2017, 3:47 AM
libraries/base/System/Environment/ExecutablePath.hsc
168

Fixed.

176

Fixed. I'm even directly using FILE_NAME_OPENED to make it crystal clear now.

190

I removed the reference to MAX_PATH and rather tried to turn this into a comment about how we try bigger buffers until we supply one that's big enough to contain the result. Hopefully this is more helpful.

Phyx added a comment.Nov 24 2017, 3:48 AM

WIth the changelog entry this is ok from me. I'll OK it after that.

libraries/base/System/Environment/ExecutablePath.hsc
63

You're the only one who can tick the box, unless we commandeer the revision :), with documentation I believe @bgamari meant in the changelog https://github.com/ghc/ghc/blob/master/libraries/base/changelog.md since this is a user visible function.

alpmestan updated this revision to Diff 14791.Nov 24 2017, 3:55 AM
  • add changelog entry
alpmestan marked 11 inline comments as done.Nov 24 2017, 3:57 AM

Added a changelog entry for base.

libraries/base/System/Environment/ExecutablePath.hsc
63

Ooooh, OK. Well I don't think it hurts to advertise the change of behaviour there as well. I'll add a changelog entry about this and leave this as well, unless asked to remove it.

alpmestan marked 2 inline comments as done.Nov 24 2017, 3:59 AM
bgamari accepted this revision.Nov 27 2017, 8:23 AM

If it's good enough for @Phyx it's good enough for me. Thank you both!

This revision is now accepted and ready to land.Nov 27 2017, 8:23 AM
This revision was automatically updated to reflect the committed changes.