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

os: ExpandEnv: Missing curly bracket at end of string substitutes var name whether or not env var is set #35752

Open
gfl-chris opened this issue Nov 21, 2019 · 10 comments
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@gfl-chris
Copy link

What version of Go are you using (go version)?

go version go1.12.9 linux/amd64

and

Go Playground (1.13.4)

Does this issue reproduce with the latest release?

Yes. (Go Playground)

What operating system and processor architecture are you using (go env)?

go env Output
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ceineke/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ceineke/.gvm/pkgsets/go1.12.9/global"
GOPROXY=""
GORACE=""
GOROOT="/home/ceineke/.gvm/gos/go1.12.9"
GOTMPDIR=""
GOTOOLDIR="/home/ceineke/.gvm/gos/go1.12.9/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build732428512=/tmp/go-build -gno-record-gcc-switches"
and
Go Playground

What did you do?

https://play.golang.org/p/GSfBKiZqj2L

What did you expect to see?

Either

Hello, ${name
Hello, ${name

or

Hello, ${name
Hello, foo

What did you see instead?

Hello, name
Hello, name
@cuonglm
Copy link
Member

cuonglm commented Nov 21, 2019

{ is an undefined environment variable, so ${ is expanded to empty string. I think it's working as intended. Environment variable is not the same as shell variable.

@gfl-chris
Copy link
Author

gfl-chris commented Nov 21, 2019

It's not and it shouldn't. The code here points out that ${ is bad syntax. I guess what I'm trying to say is that it shouldn't be "eaten."

Concrete example: Running ExpandEnv over a YAML file whose last line looks like this:

  password: abcd${efgh

will, IMHO, incorrectly clobber the password.

@cuonglm
Copy link
Member

cuonglm commented Nov 22, 2019

It's not and it shouldn't. The code here points out that ${ is bad syntax. I guess what I'm trying to say is that it shouldn't be "eaten."

Concrete example: Running ExpandEnv over a YAML file whose last line looks like this:

  password: abcd${efgh

will, IMHO, incorrectly clobber the password.

Hmm, it's also not consistent, since when we can do os.Setenv("{", "foo") then os.Getenv("{") will return foo, but ExpandEnv does not.

I think it's better to just improve the document of ExpandEnv to indicate that the expansion follow the rule of shell expansion, invalid syntax will return empty string.

@toothrot
Copy link
Contributor

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 25, 2019
@toothrot toothrot added this to the Backlog milestone Nov 25, 2019
@toothrot toothrot changed the title os: ExpandEnv: Missing curly bracket at end of string substitutes var name whether env var is set or not. os: ExpandEnv: Missing curly bracket at end of string substitutes var name whether or not env var is set Nov 25, 2019
@ianlancetaylor
Copy link
Contributor

I agree that we should mention this in the documentation and not change the behavior of the function.

@ianlancetaylor ianlancetaylor added Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done. labels Nov 25, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 25, 2019
@ghost
Copy link

ghost commented Nov 26, 2019

In another case Expand was fixed:
#24345
Perhaps in this case ExpandEnv should be fixed too.

@ianlancetaylor
Copy link
Contributor

There is no significant difference between Expand and ExpandEnv.

See also #26135.

That said, I think you are suggesting that for ${ with no } we should keep the ${. That may be OK.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/208857 mentions this issue: os: add documentation for invalid expansion syntax in ExpandEnv

@ghost
Copy link

ghost commented Nov 27, 2019

you are suggesting that for ${ with no } we should keep the ${.

Yep. It's not even like ExpandEnv replaces ${ with a value of the { variable: https://play.golang.org/p/CzLSGzkLAWx

@fitzy101
Copy link

I'm looking at picking up an issue for a first contribution, and it looks as though this is a candidate. Is that actively being addressed by anyone? It looks like there is still a decision to be made on the expected behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants