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

Const method isn't thread safe on Windows #90

Closed
phprus opened this issue Jan 22, 2021 · 6 comments
Closed

Const method isn't thread safe on Windows #90

phprus opened this issue Jan 22, 2021 · 6 comments
Assignees
Labels
available on master Fix is done on master branch, issue closed on next release bug Something isn't working Windows Windows platform is affected
Milestone

Comments

@phprus
Copy link
Contributor

phprus commented Jan 22, 2021

Race condition in all function, who call GHC_INLINE const path::string_type& path::native() const, because this method write to mutable variable _native_cache.

Proposal:
Switch to native wchar_t storage on Windows.
I think this it shouldn't hurt the "UTF-8 Everywhere" philosophy if all other std::string in API are UTF-8 encoded.

@gulrak gulrak added bug Something isn't working Windows Windows platform is affected labels Jan 22, 2021
@gulrak
Copy link
Owner

gulrak commented Jan 22, 2021

Actually it has not really to do with the question of using wchar_t or not internally, but with the fact that the library uses the generic form internally, so if you don't set GHC_WIN_WSTRING_STRING_TYPE or use fs_std.hpp or fs_std__fwd.hpp headers, and have the std::string/char interface everywhere, the issue is still there on Windows.

One way would be using a mutex but I really would not like to introduce that into filesystem. I need to think about this.

@gulrak gulrak self-assigned this Jan 22, 2021
@gulrak gulrak added this to the v1.4.2 milestone Jan 22, 2021
@phprus
Copy link
Contributor Author

phprus commented Jan 23, 2021

Change the format of the internal path to native?

This is also a performance issue. The WinAPI *W function expects the wchar_t native format path. All calls WinAPI functions in this library use the encoding and format transformation, for example:

GetFileAttributesExW(p.wstring().c_str(), GetFileExInfoStandard, &attr)
GetFileAttributesExW(detail::fromUtf8<std::wstring>(p.u8string()).c_str(), GetFileExInfoStandard, &attr)

@gulrak
Copy link
Owner

gulrak commented Jan 23, 2021

Yeah, that's of course my favorite solution too, and I started working on that yesterday, I just don't know how fast I can provide that, with the free time I have currently available. So I was thinking about a temporary workaround to mitigate the risk until that work is done.

@phprus
Copy link
Contributor Author

phprus commented Jan 24, 2021

Great news! Thank you!

Possible workaround: Add macro to select fast and non threadsafe implementation (with mutable _native_cache) or slow and thread-safe (without cache).

@gulrak gulrak modified the milestones: v1.4.2, v1.5.0 Jan 30, 2021
gulrak added a commit that referenced this issue Jan 31, 2021
gulrak added a commit that referenced this issue Jan 31, 2021
…d/prefixed long filenames working with char value_type again
gulrak added a commit that referenced this issue Jan 31, 2021
…nd configurable, all tests working on Windows (wchar_t backend will be stage 3)
gulrak added a commit that referenced this issue Jan 31, 2021
gulrak added a commit that referenced this issue Jan 31, 2021
gulrak added a commit that referenced this issue Feb 6, 2021
@gulrak
Copy link
Owner

gulrak commented Feb 6, 2021

Okay, everything is back together, still want to make some additional tests and look into performance, but I chances are, there might be a release v1.5 tomorrow.

gulrak added a commit that referenced this issue Feb 6, 2021
@gulrak gulrak added the available on master Fix is done on master branch, issue closed on next release label Feb 7, 2021
@gulrak
Copy link
Owner

gulrak commented Feb 7, 2021

This is now part of release v1.5.0

@gulrak gulrak closed this as completed Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
available on master Fix is done on master branch, issue closed on next release bug Something isn't working Windows Windows platform is affected
Projects
None yet
Development

No branches or pull requests

2 participants