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

Jump to bottom

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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.18

Comments

@findleyr
Copy link
Contributor

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 NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Oct 6, 2021
@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 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.

@findleyr
Copy link
Contributor Author

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).

@griesemer
Copy link
Contributor

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.

@findleyr
Copy link
Contributor Author

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.

@gopherbot
Copy link

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

@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Oct 6, 2021
@griesemer
go/types: break cycles in invalid types
39a4df4 
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 commented Oct 6, 2021

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

@gopherbot
Copy link

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.

@gopherbot gopherbot mentioned this issue Oct 6, 2021
Closed
@gopherbot
Copy link

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

gopherbot pushed a commit that referenced this issue Dec 1, 2021
@griesemer @mknyszek
[release-branch.go1.17] go/types: break cycles in invalid types
66007e5 
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>
@findleyr findleyr mentioned this issue May 15, 2022
Closed
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
3 participants
@griesemer @gopherbot @findleyr