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

go/types, types2: stack overflow in hasVarSize for invalid type #48819

Closed
findleyr opened this issue Oct 6, 2021 · 9 comments
Closed

go/types, types2: stack overflow in hasVarSize for invalid type #48819

findleyr opened this issue Oct 6, 2021 · 9 comments
Assignees
Milestone

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Oct 6, 2021

With type parameters, the results of unsafe.Sizeof may be variable. The logic to check this needs to be defensive against invalid types. Type checking the following code currently results in a stack overflow:

package p

import "unsafe"

type T struct {
	T
}

func _() {
	var t T
	_ = unsafe.Sizeof(t)
}

CC @griesemer

@findleyr findleyr added this to the Go1.18 milestone Oct 6, 2021
@findleyr findleyr self-assigned this Oct 6, 2021
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Oct 6, 2021

This is actually a regression in 1.17: crashes in 1.17.1, but not in 1.16.8. I will bisect.

Loading

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Oct 6, 2021

Bisected to https://golang.org/cl/284254, which makes sense.

We can fix this case, but reconsidering the change in https://golang.org/cl/284254, I wonder if it resulted in any other similar crashers (or broke any uses of go/types).

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 6, 2021

hasVarSize could use a cycle-detection (seen) map in case it encounters invalid types. That said, the change the cycle detection mechanism was setting underlying to Typ[invalid] exactly to kill such cycles. The "fix" in go2go may have been wrong.

Loading

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Oct 6, 2021

@griesemer do you think this is worthy of a back-port to 1.17? I'm not sure. It seems rare, but the behavior change with respect to Named.Underlying is a regression that could cause downstream problems that we can't see.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 6, 2021

Change https://golang.org/cl/354349 mentions this issue: go/types: break cycles in invalid types

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 6, 2021

Change https://golang.org/cl/354329 mentions this issue: cmd/compile/internal/types2: break cycles in invalid types

Loading

@gopherbot gopherbot closed this in 9062a52 Oct 6, 2021
gopherbot pushed a commit that referenced this issue Oct 6, 2021
This is a clean port of CL 354329 from types2 to go/types.

For #48819.

Change-Id: I9efdcdbfa6432f3cee64d924a4c67ecc6793cf86
Reviewed-on: https://go-review.googlesource.com/c/go/+/354349
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Oct 6, 2021

@gopherbot please consider this for backport to 1.17, it's a regression.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 6, 2021

Backport issue(s) opened: #48825 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 1, 2021

Change https://golang.org/cl/368456 mentions this issue: [release-branch.go1.17] go/types: break cycles in invalid types

Loading

gopherbot pushed a commit that referenced this issue Dec 1, 2021
This is a partial port of CL 354329 from types2 to go/types.
It contains an adjustment to type.go to deal with possibly
invalid type bounds.

Fixes #48825.
For #48819.

Change-Id: I9efdcdbfa6432f3cee64d924a4c67ecc6793cf86
Reviewed-on: https://go-review.googlesource.com/c/go/+/354349
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/368456
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants