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.

Sign up for GitHub

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

weak: weak.Pointer[T] can be misused with type conversions #71583

Closed
mateusz834 opened this issue Feb 6, 2025 · 11 comments
Closed

weak: weak.Pointer[T] can be misused with type conversions #71583

mateusz834 opened this issue Feb 6, 2025 · 11 comments
Labels
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
Go1.24

Comments

@mateusz834
Copy link
Member

This is the second time something like this happens in the std, see #56603.

func main() {
        val := (weak.Pointer[[1024 * 1024]int])(weak.Make(new(int))).Value()
        val[len(val)-1] = 999
        fmt.Println(val[len(val)-1])
}
$ go run .
unexpected fault address 0xc000810108
fatal error: fault
[signal SIGSEGV: segmentation violation code=0x2 addr=0xc000810108 pc=0x49203d]

goroutine 1 gp=0xc000002380 m=0 mp=0x560480 [running]:
runtime.throw({0x4b4e20?, 0x7f54c773b108?})
        /home/mateusz/code/go/src/runtime/panic.go:1096 +0x48 fp=0xc000084ea8 sp=0xc000084e78 pc=0x463328
runtime.sigpanic()
        /home/mateusz/code/go/src/runtime/signal_unix.go:939 +0x26c fp=0xc000084f08 sp=0xc000084ea8 pc=0x4646cc
main.main()
        /tmp/testing/main.go:10 +0x3d fp=0xc000084f50 sp=0xc000084f08 pc=0x49203d
runtime.main()
        /home/mateusz/code/go/src/runtime/proc.go:283 +0x28b fp=0xc000084fe0 sp=0xc000084f50 pc=0x43434b
runtime.goexit({})
        /home/mateusz/code/go/src/runtime/asm_amd64.s:1700 +0x1 fp=0xc000084fe8 sp=0xc

(........)

Previously reported at: #67552 (comment)

CC @mknyszek
@mateusz834 mateusz834 added this to the Go1.24 milestone Feb 6, 2025
@mateusz834 mateusz834 mentioned this issue Feb 6, 2025
Closed
@mateusz834
Copy link
Member Author

Marking this as a release-blocker since doing the same thing like in #56603 is technically a backwards incompatible change.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/647195 mentions this issue: weak: prevent unsafe conversions using weak pointers

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/647215 mentions this issue: weak: disallow type conversions of Pointer[T]

@thepudds
Copy link
Contributor

thepudds commented Feb 6, 2025

Hi @mknyszek, I sent https://go.dev/cl/647195, which seems to pass the trybots. @qiulaidongfeng also sent https://go.dev/cl/647215.

Do those seem like reasonable approaches?

@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 6, 2025
@mknyszek
Copy link
Contributor

mknyszek commented Feb 6, 2025

Shoot. Maybe we should have a more generic (ha) test for this on the standard library, or perhaps it should be part of the per-release API checker.

Thank you both @qiulaidongfeng and @thepudds for sending CLs! I'll probably go with @thepudds' CL just because it adds a couple of tests, for the next person who trips over the same issue.

@mateusz834
Copy link
Member Author

Just want to note that this needs also to be fixed on the release-branch.go1.24 branch, as this is a new API.

@mknyszek
Copy link
Contributor

mknyszek commented Feb 6, 2025 •
edited
Loading

Yep, we'll have to cherry-pick the change.

@dmitshur
Copy link
Contributor

dmitshur commented Feb 6, 2025

CC @cherrymui.

@dmitshur dmitshur added the FixPending Issues that have a fix which has not yet been reviewed or submitted. label Feb 6, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/647435 mentions this issue: [release-branch.go1.24] weak: prevent unsafe conversions using weak pointers

@dmitshur
Copy link
Contributor

dmitshur commented Feb 6, 2025 •
edited
Loading

Since this release-blocker is in the Go 1.24 milestone, re-opening to track cherry-picking to release-branch.go1.24.

@dmitshur dmitshur reopened this Feb 6, 2025
gopherbot pushed a commit that referenced this issue Feb 6, 2025
@thepudds @cherrymui
[release-branch.go1.24] weak: prevent unsafe conversions using weak p…
b7b4c60 
…ointers

Prevent conversions between Pointer types,
like we do for sync/atomic.Pointer.

Fixes #71583

Change-Id: I20e83106d8a27996f221e6cd9d52637b0442cea4
Reviewed-on: https://go-review.googlesource.com/c/go/+/647195
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
(cherry picked from commit 8163ea1)
Reviewed-on: https://go-review.googlesource.com/c/go/+/647435
Reviewed-by: Carlos Amedee <carlos@golang.org>
TryBot-Bypass: Cherry Mui <cherryyz@google.com>
@dmitshur
Copy link
Contributor

dmitshur commented Feb 6, 2025

Closed by merging CL 647435 (commit b7b4c60) to release-branch.go1.24.

@dmitshur dmitshur closed this as completed Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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
Milestone
Go1.24
Development

No branches or pull requests
5 participants
@mknyszek @dmitshur @gopherbot @mateusz834 @thepudds