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.

Sign up for GitHub

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

Jump to bottom

Add function to query windows known folders #31331

Closed
wants to merge 3 commits into from
Closed

Add function to query windows known folders #31331

wants to merge 3 commits into from

Conversation

musm
Copy link
Contributor

@musm musm commented Mar 13, 2019

As suggested and remarked by @stevengj relying on environment variables for system paths can be dangerous, especially since users can mess with them.

For example, an easy way to break julia's download function is as follows

ENV["SYSTEMROOT"] = "blah"

julia> download("https://github.com")                                                               
ERROR: IOError: could not spawn `'blah\System32\WindowsPowerShell\v1.0\powershell.exe'

In this PR we wrap windows api to access known folders from julia, to prevent such problems.

@ararslan ararslan added system:windows Affects only Windows domain:filesystem Underlying file system and functions that use it labels Mar 13, 2019
@ararslan ararslan requested a review from vtjnash March 13, 2019 17:11
c42f
c42f reviewed Mar 26, 2019
View reviewed changes
Copy link
Member

@c42f c42f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that environment variables aren't the most robust things to depend on. Was the motivation real or theoretical here? (Ie, did a user actually break their julia install by messing with %SYSTEMROOT%?)

Regarding API, this looks reasonable if it's limited to internal use by Base. If we wanted a general "standard paths" API, we should consider looking for some API inspiration elsewhere, for example:

This needs some kind of tests before merging. Obviously the output of this function will depend on the test machine configuration. That's annoying but you could do some pattern matching I suppose.

base/path.jl
pathptr = Ref{Ptr{Cwchar_t}}()
result = ccall((:SHGetKnownFolderPath,:shell32), stdcall, Cint,
(GUID, UInt32, Ptr{Cvoid}, Ref{Ptr{Cwchar_t}}), folderid, KF_FLAG_DEFAULT, C_NULL, pathptr)
result != 0 && error()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing SystemError("SHGetKnownFolderPath", result) would be more helpful here if this ever failed.

base/path.jl
result = ccall((:SHGetKnownFolderPath,:shell32), stdcall, Cint,
(GUID, UInt32, Ptr{Cvoid}, Ref{Ptr{Cwchar_t}}), folderid, KF_FLAG_DEFAULT, C_NULL, pathptr)
result != 0 && error()
pathbuf = unsafe_wrap(Vector{Cwchar_t}, pathptr[], ccall(:wcslen, UInt, (Ptr{Cwchar_t},), pathptr[]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wcslen returns size_t; best use Csize_t for clarity.

base/path.jl
result != 0 && error()
pathbuf = unsafe_wrap(Vector{Cwchar_t}, pathptr[], ccall(:wcslen, UInt, (Ptr{Cwchar_t},), pathptr[]))
path = transcode(String, pathbuf)
ccall((:CoTaskMemFree, :ole32), stdcall, Cvoid, (Ptr{Cwchar_t},), pathptr[])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this takes void* as input.

@c42f c42f added the needs tests Unit tests are required for this change label Mar 26, 2019
@musm
Copy link
Contributor Author

musm commented Mar 26, 2019

@c42f thank you for the review and comments.

Agreed that environment variables aren't the most robust things to depend on. Was the motivation real or theoretical here? (Ie, did a user actually break their julia install by messing with %SYSTEMROOT%?)

The concern here was more theoretical.

My initial motivation was just to limit this to internal use in Base, although I see the benefit of a better API for standardpaths.

@c42f
Copy link
Member

c42f commented Mar 27, 2019

Hum. If the concern is theoretical it does make me wonder how useful it is to have all this code to kill off a couple of environment variable uses which may be removed anyway (#27043). Do you have a link to the original conversation containing a bit more motivation for doing this?

@musm
Copy link
Contributor Author

musm commented Mar 27, 2019

Well other packages that rely on these system paths, specifically PyCall, directly call SHGetKnownFolderPath in order to avoid incorrect environment variables since it can be brittle. An internal API that can be used by such packages is desirable.

These directories can be different from when run from msys or cygwin terminal, and environment variables may be overridden. SHGetKnownFolderPath() always gives the correct path to known system folders.

There probably needs to be some more discussion on an API and how this should all look like, but I wanted to get the ball rolling with a PR.

@c42f
Copy link
Member

c42f commented Mar 27, 2019

Oh excellent: that's good motivation to have this code if it's being duplicated across the ecosystem. (I was just a little worried that it didn't seem entirely worthwhile to clean up a couple of uses in Base.)

However, I think that also means designing a public API which can be portable (to the extent possible) between systems. At a quick glance Qt's QStandardPath is a good place to look for design inspiration.

Another place to look for use cases (in addition to PyCall) would be the handling of standard paths rooted in the user home directories as used to find julia configuration, julia Pkg data, etc. How are they handled and can it be unified in a neat way under this API?

@musm
Copy link
Contributor Author

musm commented Jul 15, 2020

Stale

@musm musm closed this Jul 15, 2020
@musm
Copy link
Contributor Author

musm commented Jul 27, 2020

I put this in a package https://github.com/musm/WinKnownPaths.jl
where all the paths are available

@musm
Copy link
Contributor Author

musm commented Jul 27, 2020

@c42f should we think about more general platform folder abstractions? I registered the julia package, but it might be beneficial to try to make a cross-platform version with the same idea.

@musm
Copy link
Contributor Author

musm commented Jul 27, 2020

I'm just not sure if it's as a big an issue on macos and linux as windows ?

@musm musm deleted the wpaths branch July 27, 2020 19:44
musm
musm commented Jul 27, 2020
View reviewed changes
base/path.jl
"""
function get_known_folder_path(folderid::GUID)
pathptr = Ref{Ptr{Cwchar_t}}()
result = ccall((:SHGetKnownFolderPath,:shell32), stdcall, Cint,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason I am unable to get this line to run on win-32bit. It keeps spitting ERROR: ReadOnlyMemoryError()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@c42f c42f c42f left review comments

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees
No one assigned
Labels
domain:filesystem Underlying file system and functions that use it needs tests Unit tests are required for this change system:windows Affects only Windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@musm @c42f @ararslan