stack/loader: Ignore cmd.exe special env variables #4081

vvoland · 2023-03-09T15:11:33Z

Fixes: docker stack deploy Fails When Run From cmd.exe on Windows with CLI v23+ #4078

On Windows, ignore all variables that start with "=" when building an environment variables map for stack.

For MS-DOS compatibility cmd.exe can set some special environment variables that start with a "=" characters, which breaks the general assumption that the first encountered "=" separates a variable name from variable value and causes trouble when parsing.

These variables don't seem to be documented anywhere, but they are described by some third-party sources and confirmed empirically on my Windows installation.

Useful sources:
https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133
https://ss64.com/nt/syntax-variables.html

Known variables:

=ExitCode stores the exit code returned by external command (in hex format)
=ExitCodeAscii - same as above, except the value is the ASCII representation of the code (so exit code 65 (0x41) becomes 'A').
=::=::\ and friends - store drive specific working directory. There is one env variable for each separate drive letter that was accessed in the shell session and stores the working directory for that specific drive. The general format for these is: =<DRIVE_LETTER>:=<CWD> (key==<DRIVE_LETTER>:, value=<CWD>) where is a working directory for the drive that is assigned to the letter <DRIVE_LETTER>

A couple of examples: =C:=C:\some\dir (key: =C:, value: C:\some\dir) =D:=D:\some\other\dir (key: =C:, value: C:\some\dir) =Z:=Z:\ (key: =Z:, value: Z:\)

=::=::\ is the one that seems to be always set and I'm not exactly sure what this one is for (what's drive ::?). Others are set as soon as you CD to a path on some drive. Considering that you start a cmd.exe also has some working directory, there are 2 of these on start.

All these variables can be safely ignored because they can't be deliberately set by the user, their meaning is only relevant to the cmd.exe session and they're all are related to the MS-DOS/Batch feature that are irrelevant for us.

- What I did

- How I did it

- How to verify it
TestBuildEnvironment test

- Description for the changelog
Fix docker stack deploy failing when running from cmd.exe on Windows

- A picture of a cute animal (not mandatory but encouraged)

vvoland · 2023-03-09T15:23:23Z

cli/command/stack/loader/loader.go

+			// https://ss64.com/nt/syntax-variables.html
+			// https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133
+			// https://github.com/docker/cli/issues/4078
+			if s[0] == '=' {


I didn't make this one an explicit blacklist of variables, because:

MS documentation explicitly states that:

The name of an environment variable cannot include an equal sign (=).
https://learn.microsoft.com/en-us/windows/win32/procthread/environment-variables

so user can't deliberately set such variable nor anyone other than cmd.exe should actually set such variables.

I don't think we want to implement the parser for the drive-specific CWD format (or hardcode every possible drive letter).

We don't want to play whack-a-mole if there are some other undocumented variables that can start with =.

MS documentation explicitly states that:

The name of an environment variable cannot include an equal sign (=).
https://learn.microsoft.com/en-us/windows/win32/procthread/environment-variables

Interesting, thanks!

So;

Perhaps we should include that link (and description that a user cannot set it) in the comment

From the above, I wonder indeed if this is something that os.Environ() should take into account (if that would make sense) and possibly filter out those "env-vars" (might be worth opening a ticket for in Golang, and see if there's opinions on that)

There look to be existing Golang issues related to this:

os: Environ doc comment does not describe keys containing "=" golang/go#49886

x/build: spurious "=C:=C:\golang" in environment golang/go#49882

vvoland · 2023-03-09T15:40:12Z

#17 [lint 4/4] RUN --mount=type=bind,target=.     --mount=type=cache,target=/root/.cache         golangci-lint run
#17 75.52 cli/command/stack/loader/loader_test.go:65:20: `ther` is a misspelling of `there` (misspell)
#17 75.52 			"=D:=D:\\some\\other\\dir",
#17 75.52 			                ^
#17 ERROR: executor failed running [/bin/sh -c golangci-lint run]: exit code: 1

#14 [internal] load build context
------
 > [lint 4/4] RUN --mount=type=bind,target=.     --mount=type=cache,target=/root/.cache         golangci-lint run:
#17 75.52 cli/command/stack/loader/loader_test.go:65:20: `ther` is a misspelling of `there` (misspell)
#17 75.52 			"=D:=D:\\some\\other\\dir",
#17 75.52 			                ^
------

Hmm, even the linter barks at this blasphemous environment variable 😄 .

Not sure why, though:

fmt.Println("=D:=D:\\some\\other\\dir")

is: =D:=D:\some\other\dir

Will try with raw literals.

codecov-commenter · 2023-03-09T15:42:33Z

Codecov Report

Merging #4081 (a47058b) into master (881c353) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4081      +/-   ##
==========================================
- Coverage   59.16%   59.14%   -0.02%     
==========================================
  Files         287      287              
  Lines       24728    24741      +13     
==========================================
+ Hits        14631    14634       +3     
- Misses       9212     9221       +9     
- Partials      885      886       +1

vvoland · 2023-03-09T15:48:51Z

#17 [lint 4/4] RUN --mount=type=bind,target=.     --mount=type=cache,target=/root/.cache         golangci-lint run
#17 73.57 cli/command/stack/loader/loader_test.go:65:18: `ther` is a misspelling of `there` (misspell)
#17 73.57 			`=D:=D:\some\other\dir`,
#17 73.57 			              ^
#17 ERROR: executor failed running [/bin/sh -c golangci-lint run]: exit code: 1

#14 [internal] load build context
------
 > [lint 4/4] RUN --mount=type=bind,target=.     --mount=type=cache,target=/root/.cache         golangci-lint run:
#17 73.57 cli/command/stack/loader/loader_test.go:65:18: `ther` is a misspelling of `there` (misspell)
#17 73.57 			`=D:=D:\some\other\dir`,
#17 73.57 			              ^

Interesting, looks like golangci-lint bug?

On Windows, ignore all variables that start with "=" when building an environment variables map for stack. For MS-DOS compatibility cmd.exe can set some special environment variables that start with a "=" characters, which breaks the general assumption that the first encountered "=" separates a variable name from variable value and causes trouble when parsing. These variables don't seem to be documented anywhere, but they are described by some third-party sources and confirmed empirically on my Windows installation. Useful sources: https://devblogs.microsoft.com/oldnewthing/20100506-00/?p=14133 https://ss64.com/nt/syntax-variables.html Known variables: - `=ExitCode` stores the exit code returned by external command (in hex format) - `=ExitCodeAscii` - same as above, except the value is the ASCII representation of the code (so exit code 65 (0x41) becomes 'A'). - `=::=::\` and friends - store drive specific working directory. There is one env variable for each separate drive letter that was accessed in the shell session and stores the working directory for that specific drive. The general format for these is: `=<DRIVE_LETTER>:=<CWD>` (key=`=<DRIVE_LETTER>:`, value=`<CWD>`) where <CWD> is a working directory for the drive that is assigned to the letter <DRIVE_LETTER> A couple of examples: `=C:=C:\some\dir` (key: `=C:`, value: `C:\some\dir`) `=D:=D:\some\other\dir` (key: `=C:`, value: `C:\some\dir`) `=Z:=Z:\` (key: `=Z:`, value: `Z:\`) `=::=::\` is the one that seems to be always set and I'm not exactly sure what this one is for (what's drive `::`?). Others are set as soon as you CD to a path on some drive. Considering that you start a cmd.exe also has some working directory, there are 2 of these on start. All these variables can be safely ignored because they can't be deliberately set by the user, their meaning is only relevant to the cmd.exe session and they're all are related to the MS-DOS/Batch feature that are irrelevant for us. Signed-off-by: Paweł Gronowski <[email protected]>

twinter-amosfivesix · 2023-03-09T15:51:38Z

cli/command/stack/loader/loader.go

+				continue
+			}
+		}
+
 		k, v, ok := strings.Cut(s, "=")
 		if !ok || k == "" {
 			return result, errors.Errorf("unexpected environment %q", s)


I would love to see this message changed to "unexpected environment variable %q". See my comment on the bug report.

Possibly we could use '%s' as well; the %q is nice as it escapes output, but in some cases that makes the output harder to read (error something "\"\\\\foo\\\\bar\" etc), so having it print "as-is" could make that more readable.

Done. Not entirely sure about using %s instead of %q though (because if the var starts with = which isn't explicitly not allowed, can we really assume it's safe to print without escaping?)

I think we should be fine? Or were you thinking of these containing control characters (\033[1;42m) that would mess up the terminal?

thaJeztah · 2023-03-09T15:52:16Z

Oh! I guess it sees \o as "escaped o", and then checks for ther (as misspell of there) 😂

Guess either a nolint comment, or maybe some other "fake" words for the directories would work (maybe better, as it would avoid needing a nolint comment)

vvoland · 2023-03-09T15:52:59Z

Changing =D:=D:\some\other\dir to =D:=D:\some\different\dir does the trick! 😄

Make the error more specific by stating that it's caused by a specific environment variable and not an environment as a whole. Also don't escape the variable to make it more readable. Signed-off-by: Paweł Gronowski <[email protected]>

thaJeztah · 2023-03-09T16:06:08Z

cli/command/stack/loader/loader.go

 		k, v, ok := strings.Cut(s, "=")
 		if !ok || k == "" {


I was wondering if the case above could also be checked as;

if ok && k == "" && runtime.GOOS == "windows" { // windows handling here

But not sure if that would make it actually more readable than your implementation, and won't give us a performance benefice, so .. bad suggestion I guess 😂

I think the version as written is better as it makes the intention more obvious.

thaJeztah · 2023-03-09T16:31:40Z

CI failure is unrelated;

#14 324.4 	github.com/prometheus/[email protected]: invalid version: git fetch --unshallow -f origin in /go/pkg/mod/cache/vcs/63ab0745b8f8592be04b11f20c3394b541f0fee31096db359ab45cff934d72a3: exit status 128:
#14 324.4 	fatal: unable to access 'https://github.com/prometheus/common/': OpenSSL SSL_connect: Connection reset by peer in connection to github.com:443

thaJeztah

code LGTM

left one comment earlier about possibly referring to the MS Docs (not a show-stopper, but let me know if you think it makes sense to include) #4081 (comment)

vvoland added area/stack platform/windows kind/bugfix PR's that fix bugs labels Mar 9, 2023

vvoland added this to the 23.0.2 milestone Mar 9, 2023

vvoland requested a review from silvin-lubecki as a code owner March 9, 2023 15:11

vvoland commented Mar 9, 2023

View reviewed changes

vvoland force-pushed the windows-drive-cwd-env branch from 363817b to cb78f35 Compare March 9, 2023 15:41

vvoland force-pushed the windows-drive-cwd-env branch from cb78f35 to a47058b Compare March 9, 2023 15:49

twinter-amosfivesix reviewed Mar 9, 2023

View reviewed changes

thaJeztah reviewed Mar 9, 2023

View reviewed changes

thaJeztah added the process/cherry-pick label Mar 9, 2023

thaJeztah modified the milestones: 23.0.2, v-next Mar 9, 2023

thaJeztah approved these changes Mar 9, 2023

View reviewed changes

neersighted approved these changes Mar 9, 2023

View reviewed changes

thaJeztah merged commit 677aac9 into docker:master Mar 9, 2023

thaJeztah added status/4-merge process/cherry-picked labels Mar 9, 2023

thaJeztah mentioned this pull request Mar 9, 2023

[23.0 backport] stack/loader: Ignore cmd.exe special env variables #4083

Merged

neersighted added impact/changelog and removed process/cherry-pick labels Mar 16, 2023

stack/loader: Ignore cmd.exe special env variables #4081

stack/loader: Ignore cmd.exe special env variables #4081

Uh oh!

Conversation

vvoland commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vvoland Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland commented Mar 9, 2023

Uh oh!

codecov-commenter commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vvoland commented Mar 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 9, 2023

Uh oh!

vvoland commented Mar 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 9, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vvoland commented Mar 9, 2023 •

edited

Loading

vvoland Mar 9, 2023 •

edited

Loading

codecov-commenter commented Mar 9, 2023 •

edited

Loading