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

cmd/compile: incorrectly accepts package with name collision due to dot-imported function (was: panic during dot-import function name collision) [1.16 backport] #47244

Closed
gopherbot opened this issue Jul 16, 2021 · 7 comments
Milestone

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 16, 2021

@cuonglm requested issue #47201 to be considered for backport to the next 1.16 minor release.

@gopherbot please consider this for backport to 1.16

@gopherbot
Copy link
Author

@gopherbot gopherbot commented Jul 16, 2021

Change https://golang.org/cl/334874 mentions this issue: [release-branch.go1.16] cmd/compile: fix wrong Block for dot import symbol

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 21, 2021

Edit: See #47244 (comment) instead; this assessment was based on a mistaken understanding of how the issue manifested in Go 1.16 (which differs from how it affects Go 1.17).

FWIW, I think the CL is trivially safe, but this may not meet the bar of "security issues, serious problems with no workaround, and documentation fixes" specified at https://github.com/golang/go/wiki/MinorReleases. In particular, this is an issue that the compiler crashes on particular invalid input rather than reporting an error. So I think it's not serious, and it also has a reasonable workaround (i.e., fix the problematic code). Dot imports also aren't very common.

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jul 21, 2021

@mdempsky But the problem is that the compiler compile successfully invalid program!

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 21, 2021

@mdempsky But the problem is that the compiler compile successfully invalid program!

Oops, thanks. I forgot the issue affected Go 1.16 differently; I was just going by the issue title.

I think that the erroneous package would still be rejected by go/types, including due to go vet or go test (which runs vet by default). So it's perhaps still not a serious issue, but I think it's a bit more ambiguous.

Loading

@mdempsky mdempsky changed the title cmd/compile: panic during dot-import function name collision [1.16 backport] cmd/compile: incorrectly accepts package with name collision due to dot-imported function (was: panic during dot-import function name collision) [1.16 backport] Jul 21, 2021
@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jul 21, 2021

I think that the erroneous package would still be rejected by go/types, including due to go vet or go test (which runs vet by default). So it's perhaps still not a serious issue, but I think it's a bit more ambiguous.

Fair point, but during development, I think developers often use go run *.go or go build *.go or go build. And it's actually make me surprise at first try when investigating #47201.

I think it's ok to not backport if this doesn't meet the bar.

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 21, 2021

I don't feel strongly about this but I'm inclined to think that this does not meet the bar for a backport. It's not good that the compiler accepts an invalid program, but the fact that it does so doesn't prevent anybody from using Go 1.16. The most one can say is that some people may accidentally run invalid Go programs, but that doesn't seem like a problem serious enough to require a fix in a minor release.

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Jul 22, 2021

Closing this as it does not meet the bar for backporting.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants