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: nil pointer dereference in Alias.Underlying() #65455

Closed
timothy-king opened this issue Feb 2, 2024 · 7 comments
Closed

go/types: nil pointer dereference in Alias.Underlying() #65455

timothy-king opened this issue Feb 2, 2024 · 7 comments
Assignees
Milestone

Comments

@timothy-king
Copy link
Contributor

Alias.Underlying() crashes when called on an Alias created by NewAlias(...).

  1. NewAlias calls newAlias with a (*Checker)(nil).
  2. a.actual by Alias.cleanup() is not set when the (*checker) is nil in newAlias.
  3. Alias.Underlying() calls a.actual.Underlying().

As a mitigation strategy, one can immediately call Unalias(a) for its side-effect after a := NewAlias(...), but we should fix this.

cc @griesemer @findleyr

@adonovan
Copy link
Member

adonovan commented Feb 2, 2024

...and while we're there, also avoid the update of the Alias.actual link in the Unalias function, which turns a getter into a setter (=> data race, as @timothy-king pointed out). Using atomic load/store on Alias.actual is one solution.

@timothy-king
Copy link
Contributor Author

I think the following would also handle the datarace:

func Unalias(t Type) Type {
    if a0, _ := t.(*Alias); a0 != nil {
      return a0.actual
    }
    return t
}

func unalias(a0 *Alias)  {
	if a0.actual != nil {
		return
	}
	var t Type
	for a := a0; ; {
		t = a.fromRHS
		a, _ = t.(*Alias)
		if a == nil {
			break
		}
	}
	if t == nil {
		panic(fmt.Sprintf("non-terminated alias %s", a0.obj.name))
	}
	a0.actual = t
}

func (check *Checker) newAlias(obj *TypeName, rhs Type) *Alias {
        ...
	if check != nil {
		check.needsCleanup(a)
	} else {
	        a.cleanup()
	}

	return a
}

func (a *Alias) cleanup() {
	unalias(a)
}

@findleyr
Copy link
Contributor

findleyr commented Feb 2, 2024

This is a common pattern: we had symmetrical bugs with instances and interfaces. Generally, the fix is to ensure we never return an incomplete object from the API. I'll send a fix.

@mknyszek
Copy link
Contributor

mknyszek commented Feb 2, 2024

Speaking offline, @findleyr said this can wait for the next minor release. Moving this to the Go 1.23 milestone. After the Go 1.22 release goes out, you should be able to ask gopherbot for a backport issue (unfortunately, that will not work before then). The Go1.22.1 milestone exists if you want to create the backport issue manually. Up to you.

@mknyszek mknyszek modified the milestones: Go1.22, Go1.23 Feb 2, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/560915 mentions this issue: go/types, types: ensure that Alias.actual is in NewAlias

@findleyr
Copy link
Contributor

@gopherbot please consider this for backport to 1.22, it's a deterministic panic with a simple fix.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #65728 (for 1.22).

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

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Types returned by the go/types API must be immutable (or at least
concurrency safe), but NewAlias returned an alias without actual set.

Ensure that actual is set by unaliasing. Also make some superficial
simplifications to unalias, and avoid indirection where unnecessary.

Fixes golang#65455

Change-Id: Ic9a020da5accf9032056a924b65c9e9e08cb2e0a
Reviewed-on: https://go-review.googlesource.com/c/go/+/560915
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

5 participants