Fix slight cmdline parsing issue for job container/misc cleanup by dcantah · Pull Request #1113 · microsoft/hcsshim

dcantah · 2021-08-14T01:33:18Z

I was using filepath.Clean to remove any initial '.' paths in the commandline.
However it changes all forward slashes to backslashes, which hinders commandlines like
the following:

"cmd /c ping 127.0.0.1"

as the /c argument will be altered to \c.

Misc. cleanup involves cleaning up some comments, changing to FreeLibrary instead
of CloseHandle after loading kernel32 for signals, and checking if the stdio pipes
are nil before closing them.

Signed-off-by: Daniel Canter dcanter@microsoft.com

kevpar · 2021-08-14T02:11:36Z

-	// Clean the path, to get rid of any . elements
-	commandLine = filepath.Clean(commandLine)
+	// Handle a relative path with .
+	if len(commandLine) >= 3 && commandLine[0:2] == "./" || commandLine[0:2] == ".\\" {


Note that this will not work in a case like .\.\foo.exe. Is there a more robust way to handle this that doesn't require us writing path parsing code?

Was wondering the same, I don't remember cexecsvc doing anything with this so I'm wondering how it worked there (if it even does)

Well, I guess we could call filepath.Clean on every search path we're currently trying. The issues only really for arguments that we don't want to be converted to forward slashes, but if we got as far as the arguments and haven't found an exe, then we're doomed to fail anyways.

Ok after testing this, at least just through the crictl -> ctrd -> shim flow (not sure if docker used to do any cmdline/arg touchups), I don't think relative paths (with a dot) work for Windows Server containers either. I'm setting my working directory to C:\ and have a binary named test.exe at the root that's failing to be found with a command line of "./test.exe" or ".\test.exe". It's successfully found with just "test.exe".

Putting test.exe in a subdirectory foo, "foo\test.exe" and "foo/test.exe" work, but ".\foo\test.exe" and "./foo/test.exe" don't.

If our cmdline parsing is expected to stay fairly similar to Windows Server containers, we might be a bit ahead in this regard.. unless I'm missing something which is always possible 😆

I see. Is it required that we support a relative path for job containers?

I don't know if it's "required" really, but would be nice. A user can supply a relative path without dot and get by just like for server containers. Mainly looked into this as @claudiubelu had ran into an issue supplying a dotted relative path and informed me, @jsturtevant as well.

I was using filepath.Clean to remove any initial '.' paths in the commandline. However it changes all forward slashes to backslashes, which hinders commandlines like the following: "cmd /c ping 127.0.0.1" as the /c argument will be altered. Misc. cleanup involves cleaning up some comments, changing to FreeLibrary instead of CloseHandle after loading kernel32 for signals, and checking if the stdio pipes are nil before closing them. Signed-off-by: Daniel Canter <dcanter@microsoft.com>

kevpar · 2021-08-14T20:08:37Z

 			return "", "", errors.New("no ending quotation mark found in command")
 		}
-		path, err := searchPathForExe(commandLine[1:index+1], searchPath)
+		path, err := searchPathForExe(filepath.Clean(commandLine[1:index+1]), searchPath)


We can't rely on filepath.Clean here as it will simply replace ./ping -> ping, since it assumes the path will be evaluated relative to the working directory, rather than searched for in a list. See this comment as well.

Probably just need to check for ./ and replace it with workingDirectory.

For a relative path without a dot (e.g. ping or foo/ping), we will need to determine the correct behavior. We should understand what the default Windows Server container behavior and CreateProcess behavior is in this case.

We can't rely on filepath.Clean here as it will simply replace ./ping -> ping, since it assumes the path will be evaluated relative to the working directory, rather than searched for in a list. See this comment as well.

Good point

Probably just need to check for ./ and replace it with workingDirectory.

Check for only a first occurrence or to also handle your mention of "././" or ".." that you made in another comment? I feel like if Windows Server containers don't handle even the first case that might be fine to get away with. "././mybin.exe" seems like an odd command line to provide anyways. "../mybin.exe", "../../mybin.exe" however seems like it actually has its uses, but now we're back to your comment of writing custom parsing code

For a relative path without a dot (e.g. ping or foo/ping), we will need to determine the correct behavior. We should understand what the default Windows Server container behavior and CreateProcess behavior is in this case.

I think we should be pretty inline with the Windows Server container behavior. It seemed like everything was behaving the same before the relative path with dot discussion was brought up and the filepath.Clean change.. afaik cexecsvc tries to mirror the CreateProcess behavior and we tried to mirror the cexecsvc behavior, so they should be essentially the same before this work in regards to how they handle relative paths with no dot. Granted I'd have to go check CreateProcess's logic to see just how similar cexec and it actually are.

I think this all comes back to whether we want to support relative paths or not. The answer to that question will inform our approach here. Does the host process KEP say anything about relative paths?

Not that I could find. There's a mention that to access container mounts you could use a relative path (as they're symlinked to the container volume), but nothing regarding the cmdline. https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/1981-windows-privileged-container-support/README.md#container-mounts

Ok after syncing with James, we didn't call out anywhere that relative paths with a dot would work for these cmdline wise, and they don't work already for ws contains so I'm just going to omit this. I'll change this PR to updating the tests, and a small fix for the environment variable replacement logic

kevpar · 2021-08-14T21:57:28Z

+
+	// See if we can successfully find "exe with space.exe" with no quoting, it should first try "exe.exe", then "exe with.exe" and then finally
+	// "exe with space.exe"
+	path, _, err := getApplicationName("exe with space.exe", filepath.Dir(testExe), os.Getenv("PATH"))


Can we turn all these test cases into a loop over structs with t.Run?

Sure, they're getting a bit unwieldy..

kevpar · 2021-08-14T21:57:55Z

 	p.stdioLock.Lock()
 	defer p.stdioLock.Unlock()
-	return p.stdin.Close()
+	if p.stdin != nil {


Can we move this to a separate PR?

dcantah · 2021-08-17T00:48:41Z

@kevpar Going to close this as the work now is just cleaning up the path tests

dcantah · 2021-08-17T00:51:28Z

Oh I'm out of it, I still need to remove the filepath.Clean call also, but that's small.

dcantah requested a review from a team as a code owner August 14, 2021 01:33

dcantah force-pushed the stop-filepath-clean branch from a8b9a0a to 96c9c9e Compare August 14, 2021 01:38