ericwj / GoWindows

This repository contains infrastructure to test parts of go on Windows against .NET and Win32.
0 stars 1 forks source link

Discuss what go API's returning invalid results now should actually do #3

Open ericwj opened 4 years ago

ericwj commented 4 years ago

Perhaps it is wise to discuss the expected values against which to test go API's.

For example [System.IO.Path]::GetFullPath("\\?\.") returns "\\?\." which would in my opinion be suitable for Abs, too, but I don't want to get shot down making such claims without at least discussing that.

Another example would be to compare what EvalSymLinks does on Linux before changing the issue title and text of 40104. Can you show that it for example never returns anything 'unstable' like a volume GUID as it does on Windows? Does it return eventually at best say /dev/sda or similar? Sure I have that website which claims it is a good idea to get a stable path from EvalSymLinks, but that is just one corner of the Internet.

ericwj commented 4 years ago

Did you see this @networkimprov

networkimprov commented 4 years ago

Thanks for the ping; new issues don't appear in the inbox for repo collaborators.

For each input you throw at the api app, I imagine you'd want a corresponding call with that input to the winapi, via PowerShell or the Command shell. The latter would be considered canonical.

Re EvalSymlinks, are certain symlink targets (e.g. volumes) skipped by any winapi call to resolve symlinks? That could justify a change. Possibly related discussion in https://github.com/golang/go/issues/20506

On Unix, I'm pretty sure EvalSymlinks returns a path for the current mounted tree, which would be wrong only if a mount-point was deliberately changed between un/remount. Mount points are not symlinks to devices.

Again, use of EvalSymlinks on Windows as if it was Unix is an application bug. Go apps are expected to deal with OS differences. The Go team probably won't agree to change it.

ericwj commented 4 years ago

use of EvalSymlinks on Windows as if it was Unix is an application bug

It is breaking a primary software engineering principle to undo indirections that were there to solve a problem without even being aware of the problem.

It all looks the same from a distance but symlinks just aren't reparse points - technically, operationally and in perception of both developers and users. Trap is just quite the right word for it. If they don't take that PR anytime reasonably in this age, will just be hoping that there will not be an endless stream of successful go software besides Docker with whose crap Windows users have no option than to deal with.

are certain symlink targets ... skipped

The whole API surface looks different on Windows. There are several calls involved in resolving links. The 40095 issue is that go returns the error from the correct API that was called with the wrong flag for that link.

via PowerShell or the Command shell

Install x86 Windows 10 and you can still run Turbo Pascal today. One Microsoft foak claimed that was its sole purpose, I don't agree, but still. The Command Prompt is not canonical and is severely limited in what it can do compared with any API for any language on Windows.

If they consider Command Prompt canonical they are tards. Oh yes, Command Prompt always has drive letters. Explains a lot.

PowerShell is .NET: (dir C:\pagefile.sys).GetType().FullName will tell you System.IO.FileInfo. You can even Add-Type -NotSure @" C# "@> and it will happily compile and run the C#.

So for example for Abs, you have [FileSystemInfo]::FullName which will give you proper cased paths if at all possible and for example [System.IO.Path]::GetFullPath which will retain whatever casing you gave them. Some API's do check the filesystem, others don't. So there are some opinions and alternatives involved.

networkimprov commented 4 years ago

I was just guessing re the canonical source. Any winapi result could be considered canonical I guess. That's your dept.

Please stop seeming so upset. It makes you look somewhere between unprofessional and a troll. Go isn't great at Windows, and the Go community isn't very invested in it. You're not going to change that.

You wanted an app to probe the filepath API. I gave it to you. So stop complaining and probe.

ericwj commented 4 years ago

Yes I am increasingly unhappy even spending this time while I write PowerShell instead of doing what I was doing. I mean identifying issues is fairly simple, but it is all just a lot of work that I shouldn't have to do in the first place. And we're not even starting at the stages you have identified, like writing API calls and porting those, getting shot at over code style, etc.

And look at what I get for example, what do you think they will argue? image

I say it's dead wrong, but well yeah, PowerShell makes it look correct.

ericwj commented 4 years ago

To clarify, Set-GoFilePath dumps the PWD from the OS and from PowerShell, or sets it if it is not $null.

The above was preceded by image

networkimprov commented 4 years ago

I don't follow...

That volume is mounted on ...\GoWindows\src ? Or C:\ ?

And you think the filepath.Abs result should be the volume? Or that filepath.EvalSymlinks result should NOT be the volume?

What gave the "Cannot find path..." error?

ericwj commented 4 years ago

Well I said PowerShell is .NET, but PowerShell is in fact a large code base on top of it with to an extend its own ways of doing things.

PowerShell can't go there, that's why it errors. No idea why since I haven't used Win32 calls to change the path to the volume path, I used System.IO.Directory.SetCurrentDirectory and at face value I would have expected PowerShell to use those API's. But who knows they might have their own compatibility reasons or design philosophy. Maybe it is sort of a security feature that you can't just walk up to a computer, start PowerShell and create directories that Windows Explorer can't show or for which you need to use prefixes to delete, etc. I know some viruses did things like that.

It might be reasonable to compare PowerShell with Command Prompt. PowerShell basically replaces the latter. Neither should have in any way comparable capability to any (serious/system) programming language API (like C/C#/go) and access to this kinds of paths is one of the differences.

One long standing issue which bites people who use anything next to PowerShells own CmdLets is that it has its own concept of current directory. Well it has providers, such that you can for example do dir Cert:\LocalMachine\WebHosting and get IIS certificates back. Or do dir HKLM: -Recurse and pretty much see the whole registry scroll before you, which is in itself pretty neat but with the caveat that you need to be aware that $PWD is not GetCurrentDirectory.

That volume path is one that has Y: assigned; it is a virtual disk. There is not one relationship or link at any level between the volume id and C:\tools. And I'm sure Win32 GetCurrentDirectory will give me the volume path although I haven't tried it.

ericwj commented 4 years ago

Well I did use SetCurrentDirectory, albeit through a few different C# classes all else System.IO.Directory.SetCurrentDirectory does is error checking.

networkimprov commented 4 years ago

So your current directory is Y:\ but PS disagrees? And filepath.Abs should return Y:\ ? And filepath.EvalSymlinks should also return Y:\ ?

Seems like you should use Go API for cd and pwd instead of PS?

EDIT: I added os.Getwd & os.Chdir to the api app.

ericwj commented 4 years ago

I did not use EvalSymLinks.

I did this (pseudocode, tagged with how):

Win32 cd \\?\Volume{607932d3-78af-41c0-9786-dd0177e78a39}\
@{ OS = Win32 $PWD; PS = PowerShell $PWD }
PowerShell "json Abs ." | api.exe | Write-Host
@{ OS = Win32 $PWD; PS = PowerShell $PWD }

So my assessment is that there is PowerShell voodoo in between me asking what the directory is and PowerShell starting the api.exe process for me.

Both the @{} blocks give me a Win32 current directory of \\?\Volume{607932d3-78af-41c0-9786-dd0177e78a39} and a PowerShell PWD on C:

ericwj commented 4 years ago

Running go API which I should test rather than use only makes this even less obvious.

The solution might be to port all that tedious error prone PowerShell to a proper PowerShell DLL such that I can call Process.Start myself without voodoo. And if that produces any discussions, I can even just [DllImport] from the WinAPI and use that to short circuit any discussions.

ericwj commented 4 years ago

You would still be able to just Import-Module GoFilePath and Get-GoFilePath from PowerShell.

networkimprov commented 4 years ago

Wow that's truly annoying. We could let api.exe make certain winapi syscalls before & after each Go API test.

ericwj commented 4 years ago

No, seeing the amount of iteration and manual hacking writing PowerShell gives me, I am much better off writing C#.

It just does mean that I can pretty much throw away what I already have.

networkimprov commented 4 years ago

Don't ask me how to include .cs files in a Go build :-)

ericwj commented 4 years ago

No, it will be a PowerShell module that they might pull from this repositories releases or the PowerShell gallery in binary form, or include wholesale as a .dll.

Although even if they want the source included, that wouldn't be hard to do. There is a PowerShell script available via dot.net with which building the module would become a two-liner, although it requires a download of over 100MB and if they then want PowerShell 7 an additional one-liner to install that which pulls another few tens of megabytes.

And we started with the idea that it would be temporary until someone ports the tests to go.

networkimprov commented 4 years ago

Try to do as little work as possible; that's what I did for api.exe

You just need: a winapi version of api.exe, to give canonical results for comparison with api.exe. a text file of json inputs.

You don't need to submit those in a CL, unless running on a Windows VM is essential; but why would it be?

ericwj commented 4 years ago

That sounds good, but it will still have PowerShell voodoo or Command Line limitations, requiring a drive letter.

ericwj commented 4 years ago

How hard is it to make api.exe a DLL such that I can [DllImport]?

networkimprov commented 4 years ago

We could use the syscall package to run winapi calls for cd and pwd within api.exe. Then run api.exe < input.json

Building a DLL requires a C compiler (maybe only mingw). I've never tried it, and don't want to. This might help: https://stackoverflow.com/questions/49078510/trouble-compiling-windows-dll-using-golang-1-10

ericwj commented 4 years ago

I will first try to see what happens when I call Process.Start myself - whether the voodoo is indeed coming from PowerShell.

ericwj commented 4 years ago

It doesn't work. So either

ericwj commented 4 years ago

OK, that just requires an additional \. Chdir is pretty much equal to [Environment]::SetCurrentDirectory in .NET, but it took me a while to figure out how and what. image

a text file of json inputs.

maybe its my way of thinking, but to do that and check it against code I ran that is either a lot of typing or hard to validate after the fact and a static file won't reproduce the same volume id's, machine names or UNC paths unless I ship the vhdx with the json file and start hacking etc\hosts on the test box

But with this working via [Process]::Start and depending on the discussion in golang, it is just so much easier to write it in C#

networkimprov commented 4 years ago

Let's make api.exe get local ids from env vars or arguments, and insert them into json inputs from a static file. That won't be much typing. Mirroring api.exe with your C# code is best for comparison.

Re syscall package, I was referring to this, which calls winapi directly https://golang.org/pkg/syscall/?GOOS=windows

ericwj commented 4 years ago

It is telling that there is no equivalent of EvalSymLinks anywhere.

And there must be something very similar to Match and Glob because i can type <Compile Include="<glob>" Exclude="<glob>" /> in the C# project file, but I have no idea where that is hidden.

Should I use IsPathFullQualified or IsPathRooted? See if you have drive letters /Documents is absolute but not fully qualified like C:/Documents but *nix developers might overlook the difference any day and everything will look fine until the day they call Chdir to a different drive and get bitten by not found errors.

networkimprov commented 4 years ago

Assuming you're asking re filepath.IsAbs, I'd use IsPathFullyQualified.

ericwj commented 4 years ago

Does go have something even in basic form or remotely similar to Analyzers or Obsolete attribute?

Analyzers are able to give suggestions that are not even warnings and which, depending on setup, don't even show up in builds outside an editor that is aware of them. People that have the habit of always cleaning all warnings might be broken by [Obsolete] only temporarily, forcing them to review their use of perhaps one API.

In both cases the diagnostics can be suppressed at the project level in the project file or in .editorconfig (for some standard ones) and the file level up to individual lines of code with #pragma.

networkimprov commented 4 years ago

There is command go vet which checks for common mistakes that aren't errors. There's no language feature to tag an API as deprecated.

You could include a suggestion in the EvalSymlinks doc fix issue re a new go vet rule questioning use of the API on Windows.

ericwj commented 4 years ago

If you expect me to add to 40180 with a concrete proposal for vet- I can only do that if it is very simple to do, basically my skill is limited to proposing the text of such a diagnostic. I can't find any definition files if it works like that in the repo, about only .html, .sh and .go files.

VS Code just updated gopls@4.3.0 and while I remember vaguely it must've been VS Code that made me rename bool isVolumeNameGuid to isVolumeNameGUID, but it doesn't give me any warnings now after changing that back. I guess that is vet too? If it is, it is broken for me now.

networkimprov commented 4 years ago

No need for concrete proposal. You raised the idea in a comment, that's fine.

gopls isn't vet.

networkimprov commented 4 years ago

Still want to test the rest of the filepath API?

ericwj commented 4 years ago

Havent' worked on it a few days. I wrote it and was testing it with null, "" and C:\ but that's where I got stopped. image

Some are just wrong test data that I copy/pasted, others might be more work to make and test.

Maybe I leave EvalSymLinks for now. Glob/Match I might look if I can find something that I can readily use - but the pattern options might not be the same.

I think all others mostly work once I just start shooting down null values which go can't produce, but which I automatically get from missing string properties after deserialization and fix for .NET having 3 path component concepts being root, directory and file name where go has 2.

ericwj commented 4 years ago

Wow, that kubernetes issue you linked actually hinted me to check directory security with EvalSymLinks.

It turns out that no-op'ing EvalSymLinks is required to fix directory security issues.

networkimprov commented 4 years ago

Examples of directory security violations it enables?

Aren't there fixes that aren't no-ops?

ericwj commented 4 years ago

No not violations, but depending on Administrator trustyness very, very bad things can happen.

Suppose you have a go app that is configured to use "." and it also uses ".\Subdir", then it starts using ".\Subdir\.." which might be the root of a volume. If the administrator has configured security properly, the go app may not even be able to list files or directories - not even ".\Subdir\..\Subdir". If the administrator is too trusty, they may be able to change security on a whole volume. If they have at least some access to ".\Subdir\..", they may expect to be able to change directory security and affect ".\Subdir" through ".\Subdir\..\Subdir", but that in fact just doesn't work at all since the directory security should (in proper security configuration) be inherited from "." not ".\Subdir\..".

networkimprov commented 4 years ago

Um, didn't follow that. And it doesn't mention EvalSymlinks.

ericwj commented 4 years ago

.\Subdir\.. is . before EvalSymLinks. After EvalSymLinks it can be anywhere. They just warped into a completely different directory and all they did was call EvalSymLinks and then expect the most normal thing in the world: that .\Subdir\.. is ..

networkimprov commented 4 years ago

Can . be a symlink?? If so, EvalSymlinks should yield . for it.

ericwj commented 4 years ago

Sure, any valid path can be a link. It would not match the spec to return . either if . is a link or junction.

But I didn't even write that with . meaning the string "." literally per se. If they call EvalSymLink on C:\Program Files\goapp\Subdir they may get back about any path and .. on that would not be C:\Program Files\goapp if the link is defined on C:\Program Files\goapp\Subdir and the resolved link path looks like (any directory)\Subdir with Subdir either meaning "Subdir" or any other name.

The link could also point at K:\ or \\?\Volume{guid}\ in which case .. does not exist.

ericwj commented 4 years ago

Worse, it might have the app write to the wrong directory:

PS> [System.IO.Path]::GetFullPath("C:\..")
C:\
networkimprov commented 4 years ago

On Unix, . & .. are reserved names in every directory! If they can be overwritten on Windows, that is a terrible design flaw. Are you sure about that?

Sure, .. is relative to the resolved pathname, and not nec on the path containing a symlink. How is that a worse problem than using .. on a normal pathname? EDIT: If you have permissions to it, that's not the app's fault.

ericwj commented 4 years ago

. and .. mean exactly the same on I think every operating system ever created. It is silly that what I wrote gave you the impression that this is any different on Windows.

How is that a worse problem than using .. on a normal pathname?

I'm not 100% sure but since Linux has a directory structure that is a strict single rooted tree, the issues described may not appear on Linux. They appear on Windows because the directory structure usually has multiple roots, insofar the behavior is any different compared to Linux. Plus I just randomly provide several examples of possible issues, there may be others.

Certainly what I wrote about security is probably completely different due to differences in how security works. Windows does not have chmod or the concepts user, group and other, but ACL's and inheritance separate for files and directories. Like I hinted at, given that there is a link or junction, Object Inherit and Container Inherit will propagate from both the parent of the linked folder and the 'physical' parent - depending on the administrator configuring whatever he likes on either.

If you have permissions to it, that's not the app's fault.

Maybe not, I say it is perhaps not a fault of go either, but go is responsible for having that API which maps absolutely horribly to Windows and seems to be sort of a no-brainer to use on other operating systems in the mind of many developers without checking what OS they are on or being aware of any of this.

networkimprov commented 4 years ago

You just wrote "any valid path can be a link".

I don't see a security issue here that a change to EvalSymlinks can solve. Its result is available to any app by other means. I agree that it should not generally be used, and I don't know why any app would outside of filesystem mgmt.

ericwj commented 4 years ago

Join(EvalSymLinks("C:\Program Files\goapp\Subdir"), "..") is not available in go without EvalSymLinks, .NET does not have an API for it, it is very advanced C++ realm that very, very few people have available to them.

networkimprov commented 4 years ago

So we change the docs. And let it return an error for a volume not mapped to a letter.

.NET has no ResolveSymlink API for a single fs object?

ericwj commented 4 years ago

So we change the docs. And let it return an error for a volume not mapped to a letter.

I haven't mentioned volume guid paths (not much anyway). All of this applies to links or junctions to DOS paths.

Is EvalSymLinks supposed to resolve symlinks to UNC paths or even simple drive mappings to UNC shares? It doesn't do any of that, whatever I give api.exe. The docs aren't specific about it. Or these uses reveal yet more bugs. Either way, that takes away another whole area of use cases for it.

PS> cmd /c mklink /d UncMapping \\localhost\Multimedia$
symbolic link created for UncMapping <<===>> \\localhost\Multimedia$
PS> '{"Api":"filepath.EvalSymlinks", "Path": "UncMapping"}' | .\api.exe
{"Error":"The system cannot find the file specified.","Errno":2,"Result":""}

and

PS> New-SmbMapping M: \\localhost\Multimedia$

Status Local Path Remote Path
------ ---------- -----------
OK     M:         \\localhost\Multimedia$

PS> '{"Api":"filepath.EvalSymlinks", "Path": "M:\\"}' | .\api.exe
{"Errno":0,"Result":"M:\\"}

Also error 2 is wonky, should be 3 - I see that more often.

networkimprov commented 4 years ago

I'll let you decide what the right result is for UNC targets; an error seems like a bug to me. I don't see how a drive letter mapping is a symlink.

The rest of the filepath API is more important.

ericwj commented 4 years ago

I don't see how a drive letter mapping is a symlink.

Maybe they are on Linux. Then go has to decide how portable they want to be (inhowfar as well as exactly how).

ericwj commented 4 years ago

Yeah, as I suspected, Microsoft.Extensions.FileSystemGlobbing can only do *, **, . and .. where ** means at arbitrary depth below, which Glob and Match don't do.

I'm sure it will get messy if I try to parse the pattern and build a regex from half a regex plus DOS patterns and escape characters \ intermingled with directory separators \. Are [ and ] valid characters in directory and/or file names on Linux? They are on Windows.

Glob and Match are solidly case-sensitive where FAT and similar are always and NTFS is practically about always case-insensitive - although an NTFS file system can be case-sensitively formatted (I believe) or can be case-sensitive on a directory-by-directory basis wherever configured to be. Basically that means implementing this properly is not trivial. I cannot do this with a regex alone, I'd have to ask the filesystem to match and then perhaps afterwards shoot down matches returned, but that must be done knowing whether the folder is set to be case-sensitive.

I think we should just create some file, then ask for a pattern with just stars and one letter and have it break on that alone for now. Perhaps also demonstrating creating a directory that is actually case sensistive and one that is not and show that pattern matching breaks to guide implementation complexity.