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

Windows initiative 2018 #4808

Closed
7 of 9 tasks
djdv opened this issue Mar 12, 2018 · 20 comments
Closed
7 of 9 tasks

Windows initiative 2018 #4808

djdv opened this issue Mar 12, 2018 · 20 comments
Labels
topic/windows Windows specific

Comments

@djdv
Copy link
Contributor

djdv commented Mar 12, 2018

This issue serves as an anchor to monitor the broader topic of go-ipfs on Windows.
Here I will be highlighting the current issues being focused on and the PR efforts to resolve them. Feel free to add anything to this topic that relates to go-ipfs on Windows as a whole.

Build problems

interface problems

Windows Tests & CI
We need to pass tests on Windows so new issues can be caught by the CI before becoming a problem.
When Windows is skipped for testing, we suffer from regressions such as:
#4750
#4512
#3676

In addition to fixing existing tests, new tests should be made for testing platform specific behaviors.
Please comment about platform specific edge cases that should be considered.
For example, considering UNC paths (#1933 (comment))
Long prefix paths (https://github.com/ipfs/go-ipfs/pull/3328/files#diff-7ba0a8b1e26b8ed185c4824d4ee84164R503)
How :. behaves (#3328)
File system restrictions (#2013)
etc.

Feature parity with other platforms
The inability to pipe data, get certain data, mount IPFS, have readable output, etc. only pertain to Windows, platform native solutions for these features will have to be thought about, discussed, and implemented. The future issues and PRs will back-reference this issue so that development and discussion can be easily followed.

Everything else
There are many outstanding and outdated issues
https://github.com/ipfs/go-ipfs/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+amd64%2Fwindows+
They need to be reviewed and tagged
https://github.com/ipfs/go-ipfs/issues?q=is%3Aissue+is%3Aopen+label%3Awindows
Ones that are no longer relevant (already fixed) need to be marked as closed.

@djdv
Copy link
Contributor Author

djdv commented Mar 13, 2018

Pipes and stdin seem to be partially working, it's fine if the daemon is running but gets caught up on the lockfile otherwise, needs investigating, tests, etc.

partial pipe

@djdv
Copy link
Contributor Author

djdv commented Mar 13, 2018

Back-referencing other Windows related patches that are helpful to look at.

Building on Windows documentation [wip]
#4691

Building with make now works reliably (this should be in 0.4.14+)
#4682

gx correctly handles subtools on Windows (will be in the next release of gx)
whyrusleeping/gx#154.
For now, if you depend on this, you should build and install gx from the master branch, set IPFS_GX_USE_GLOBAL in your environment, and proceed to build go-ipfs using the latest version of gx and gx-go.

Windows was returning an ambiguous "Access is denied" when trying to fetch gx deps
whyrusleeping/gx#163

@Stebalien
Copy link
Member

Pipes and stdin seem to be partially working, it's fine if the daemon is running but gets caught up on the lockfile otherwise, needs investigating, tests, etc.

So, on Linux, the lockfile is automatically released when the process closes (by the OS). Unfortunately, windows appears to use the fallback which needs to be manually released. We may not be doing that somewhere.

Personally, I'd add some Windows specific logic to the lock package (https://github.com/camlistore/go4/tree/master/lock). Make it "succeed" if it can open the lock file with exclusive access, regardless of whether or not it already existed. Even better, have it mark the lock file for deletion on close.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 13, 2018

As @Stebalien said, it is same on Linux. The root of the issue is that shells start all processes in the pipe at the same time, no matter the pipe order so ipfs commands compete for repo lock.

@Stebalien
Copy link
Member

Personally, I'd like to fix this by starting the daemon in the background in a special mode that:

  1. Disables the DHT server.
  2. Shuts down after some amount of idle time.
  3. Has a really low connection limit?

@djdv
Copy link
Contributor Author

djdv commented Mar 14, 2018

I've modified the go4 lock library, adding Windows calls to flag the lock for deletion by the OS when all handles are closed, and to not fail immediately when a lock file exists, but I'm having trouble blocking on successive lock attempts after that.
I'm using the LockFileEx systemcall with flags LOCKFILE_EXCLUSIVE_LOCK and without LOCKFILE_FAIL_IMMEDIATELY, which should put additional lock calls in a blocked state.
However I'm running into issues with that right now. Currently calls to lock just succeed 100% of the time instead of blocking, I'm unsure why at the moment.

@Stebalien
Copy link
Member

@djdv and I chatted over IRC. We've agreed to:

  1. Match what happens on linux and not block, just fail. However, @djdv's still going to fix the lock library to automatically clean up the lock file on close.
  2. Handle the pipe issue separately.

@djdv
Copy link
Contributor Author

djdv commented Mar 14, 2018

Lock reference: go4org/go4#31

@djdv
Copy link
Contributor Author

djdv commented Mar 15, 2018

colour
ipfs/go-log#21

@djdv
Copy link
Contributor Author

djdv commented Mar 21, 2018

Making an effort to deal with file system restrictions:
#4850

@Stebalien
Copy link
Member

@djdv we should also look into #3971. Users hit this all the time. We may need to set some kind of "no index" option on the datastore.

@djdv
Copy link
Contributor Author

djdv commented Mar 23, 2018

I'm going to collect information on #3971 here.
While this doesn't work 100% of the time this is a good way to instigate the problem: #3971 (comment)
I've been running repo gc and doing that to instigate this. I'm using a near full 5900RPM HDD for testing, people seem to not have issues on faster drives.

In addition sometimes this happens: #4527
I set my root to G:\tmp\.ipfs and got a lot of garbage in G:\tmp\ which is my working directory. This is bad because something is feeding some call with garbage path names. The garbage content looks like valid block data just with mangled names.
Memory corruption is suspected to be responsible for this, but it's not certain as the filepaths reported via errors are correct, and not mangled.

14:52:13.053 ERROR flatfs: Err in Get() PathError err:&os.PathError{Op:"open", Path:"G:\tmp\.ipfs\blocks\AN\CIQFG5PESHPZCNK2T6BEKAT4AXP7TFIHDHQ36NKX6YXKCHYM5J6VANY.data", Err:0x20} -> open G:\tmp.ipfs\blocks\AN\CIQFG5PESHPZCNK2T6BEKAT4AXP7TFIHDHQ36NKX6YXKCHYM5J6VANY.data: The process cannot access the file because it is being used by another process.

Instigating this while running ProcMon is causing ProcMon to crash for me which isn't helping.
Running a separate tool I noticed several instances of buffer overflows being reported but I'm unsure as to where they originate in the Go source. In addition to this there are over-reads(STATUS_END_OF_FILE) for NtReadFile and the expected STATUS_ACCESS_DENIED for NtSetInformationFile which looks like it's trying to rename a file.
untitled

Finding out why and where filenames are getting corrupted seems like the current goal as it may be the root cause of this.

@Stebalien
Copy link
Member

I believe this was fixed (accidentally) by ipfs/go-ds-flatfs#34. go-os-rename wasn't properly keeping a temporary UTF16 buffers alive here: https://github.com/jbenet/go-os-rename/blob/3ac97f61ef67a6b87b95c1282f6c317ed0e693c2/rename_windows.go#L21-L40

Basically, it was taking the src and dst paths, converting them to UTF16 paths, and then converting those to uintptrs. However, this is only safe in two cases:

  1. If the the uintptr is constructed directly as an argument to the call to Syscall due to super-special go magic.
  2. If the buffers were marked as live (using runtime.KeepAlive) after it's last use.

Othewise, go will consider these buffers "free" after these lines:

https://github.com/jbenet/go-os-rename/blob/3ac97f61ef67a6b87b95c1282f6c317ed0e693c2/rename_windows.go#L31-L32

Congrats @djdv, you managed to fix a bug before looking into it (probably)!

@djdv
Copy link
Contributor Author

djdv commented Mar 23, 2018

Woohoo. I knew that lib was bad news. I was testing go-ipfs with updated gx deps and not encountering the issue on that branch master...djdv:big-deps but trying to find out which lib (out of at least 10) and which revision (some were multiple major revisions ahead) out of the bunch solved the issue, eluded me.
Thanks for investigating @Stebalien that issue has been a big problem for a while.

@Stebalien Stebalien mentioned this issue Mar 24, 2018
13 tasks
@djdv
Copy link
Contributor Author

djdv commented Mar 25, 2018

An inconsistency in Windows specific behaviour in/on Go is being discussed here: golang/go#24482
This is related to an inconsistency where ipfs get *somehash* -o NUL works but ipfs get *somehash* -o nul will fail with a path error.
This will influence whyrusleeping/tar-utils#2

@djdv
Copy link
Contributor Author

djdv commented Mar 27, 2018

The previous Go patch has resolved the case-sensitive NUL issue for files, I've created a separate issue here: golang/go#24556
which prevents us from directing get output to NUL when the hash is a directory (not a file)
i.e.
ipfs get QmQine7gvHncNevKtG9QXxf3nXcwSj6aDDmMm52mHofEEp -o NUL
Should succeed, but currently(Go 1.10) does not.

@djdv
Copy link
Contributor Author

djdv commented Apr 6, 2018

Creation of symlinks is proving to be a problem in go-ipfs and gx
#4906
https://discuss.ipfs.io/t/issus-build-from-source/2445

Decisions are going to have to be made in whyrusleeping/tar-utils#2 on how best to handle this.

On Windows, users can't create symlinks without special privileges (see section below if interested in details), but we don't want to require users to have these privileges for normal operations.

Since symlinks are restricted for normal users on Windows, the current plan is to simply warn the user and skip symlinks if we don't have permission to create them, but still extract everything else as normal.
If we do have permission, there's no warning and we create symlinks as normal.

Requiring symlink privileges instead of opting to skip symlinks, prevents unprivileged users from being able to get any hash with a symlink in it, regardless of if it's desired or not. This currently means we won't be able to build go-ipfs without special rights on Windows as some of our dependencies utilize symlinks in their tests.


Starting with Windows 10-1703(14972), users can enable "Developer Mode" in their system settings, this allows normal users to create symlinks in Windows 10-aware applications, without elevated process rights.
There's currently a pull request in Go to enable support for this golang/go#24307 so it will likely be enabled for all Go programs in Go 1.11.
So after Go 1.11, go-ipfs Windows developers can simply enable Dev-mode and not have to worry about anything.

Prior to Go 1.11 and for users below Windows version 10-1703, users will need to hold the SeCreateSymbolicLinkPrivilege right in their access token.
The easiest way to attain this is by simply running the process as admin, which should hold this right.
To create them as a normal user in an unprivileged process, the process is more complex than simply adding the privilege to your account because of the way UAC works (see below).
Depending on how things go, a user friendly method of enabling unprivileged symlink creation may have to be written up for Windows <10 users who want to create symlinks without requiring full admin rights.

More information about symlinks on Windows can be found here
Unprivileged creation in W10:
https://blogs.windows.com/buildingapps/2016/12/02/symlinks-windows-10/
About SeCreateSymbolicLinkPrivilege:
https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/create-symbolic-links
Why can't I just add SeCreateSymbolicLinkPrivilege to my account?
https://msdn.microsoft.com/en-us/library/bb530410.aspx#vistauac_topic3 [see subsection: Access Token Changes]

@djdv
Copy link
Contributor Author

djdv commented Apr 19, 2018

Symlink creation update
symlinks

On Vista+, as long as the user has the privilege to create links, they should now be able to.
If they don't have that privilege, as a compromise, links will be skipped and a warning will be logged instead of failing out.

How the warning will look isn't finalized yet, it may be shorter or contain a link to documentation telling users how to enable symlinks if they need them.

Unprivileged link creation via Windows 10's Developer Mode, still relies on golang/go#24307, if you have this patch, link creation should work as expected.

Edit: PR is up
#4956

@djdv
Copy link
Contributor Author

djdv commented May 7, 2018

ipfs mount discussion will take place here: #5003

@djdv
Copy link
Contributor Author

djdv commented Dec 31, 2018

With the closing of the year I will also close this issue.
I'm considering this a successful initiative, and am pleased at the lack of platform specific issues today compared to when this was started.
Effort was put into generic fixes typically covering more platforms than just Windows alone, resulting in more portability overall for go-ipfs and its users.


There are only 2 outstanding issues mentioned above:

  • File system character restrictions + special creation semantics

This will require some discussion that is more relevant to spec and reference implementations, rather than Windows specifically.
We have multiple methods to deal with this issue, it's just a matter of deciding which to employ.
Relevant: #4956 (comment)

  • ipfs mount support for Windows

Originally, the goal was to have an independent (Windows specific) read-only implementation of mount for IPFS and IPNS on Windows.
This was changed into a generic read-only implementation (covering all existing platforms).
And then later changed again to support the "Files API" with write support as well.
The current status of this (as well as Windows binaries) can be found here: #5003

Ultimately, I'm glad this was the direction that was decided on.
While it's taking a bit longer, the end result should make everyone a bit happy. ;^)

We're nearing a state where this can be put into a PR, with only 1 critical problem related to writes today.
Admittedly, there are a handful of UX things that still need to be handled but will not likely delay the implementation.
(These include thins like graceful dismount/shutdown, config file variables, and to make sure things like ipfs daemon --init --mount work)


In addition, effort on the CI front will continue. Finding platform specific edge cases and implimenting tests for them, no only to prevent unnecessary regressions, but also to strengthen our test workflow in general across repos.

I look forward to collaborating more with everyone in the coming year and appreciate all the assistance and reports from everyone.

@djdv djdv closed this as completed Dec 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic/windows Windows specific
Projects
None yet
Development

No branches or pull requests

4 participants