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

runtime: AddCleanup does not panic if arg equals ptr #71316

Closed
cherrymui opened this issue Jan 17, 2025 · 3 comments
Closed

runtime: AddCleanup does not panic if arg equals ptr #71316

cherrymui opened this issue Jan 17, 2025 · 3 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@cherrymui
Copy link
Member

Go version

tip (go1.24-40b3c0e58a Fri Jan 17 08:40:47 2025 -0800 darwin/arm64)

Output of go env in your module/workspace:

any

What did you do?

https://go.dev/play/p/2SwFBThx4xJ?v=gotip

In particular,runtime.AddCleanup(t, func(a *T) { println("cleanup", a.x); ch <- 1 }, t) where the first and third arguments are equal.

What did you see happen?

It does not panic at the AddCleanup call. The cleanup never runs.

What did you expect to see?

runtime.AddCleanup is documented that it panics if arg is equal to ptr. E.g.
the code above should panic at the AddCleanup call.

The source code of AddCleanup includes

	// Check that arg is not equal to ptr.
	// TODO(67535) this does not cover the case where T and *S are the same
	// type and ptr and arg are equal.
	if unsafe.Pointer(&arg) == unsafe.Pointer(ptr) {
		throw("runtime.AddCleanup: ptr is equal to arg, cleanup will never run")
	}

But it is checking the address of arg not equal to ptr, not arg itself. And &arg cannot equal to ptr, because it's the address of an argument of a new frame. So this condition can never trigger. Also, it is a throw, not a panic as documented.

Is this what the TODO is for?

cc @cagedmantis @mknyszek

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 17, 2025
@cherrymui cherrymui added this to the Go1.24 milestone Jan 17, 2025
@mknyszek
Copy link
Contributor

I think this is just an oversight and a bug. What we should do here is use interface-typed locals to grab the types, check if they're equal, and if they are, do an unsafe conversion and a comparison. (The types can only be equal if they're both pointer types, so the comparison is trivial.)

@cherrymui
Copy link
Member Author

cherrymui commented Jan 17, 2025

Sounds good, thanks!
I guess we don't have to limit it to the types being equal? As long as arg is a pointer type, and the pointer value is equal to ptr, it will never clean up, right? E.g. ptr is a pointer to a struct, arg is a pointer to the struct's first field.

@cherrymui cherrymui added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 17, 2025
@cagedmantis cagedmantis self-assigned this Jan 17, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/643655 mentions this issue: runtime: fix the equality check in AddCleanup

@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants