sync/atomic: atomic.Pointer[T] can be misused with type conversions. #56603

mateusz834 · 2022-11-06T09:46:31Z

package main

import (
	"math"
	"sync/atomic"
)

type small struct {
	small [64]byte
}

type big struct {
	big [math.MaxUint16 * 10]byte
}

func main() {
	a := atomic.Pointer[small]{}
	a.Store(&small{})

	b := atomic.Pointer[big](a) // type conversion
	big := b.Load()

	for i := range big.big {
		big.big[i] = 1
	}
}

This sample (on my system) produces a segfault:

[mateusz@arch aa ]$ go run main.go
signal: segmentation fault (core dumped)

So we basically done a unsafe type conversion without using the unsafe package.

The text was updated successfully, but these errors were encountered:

mateusz834 · 2022-11-06T09:52:06Z

To disallow this we would need something like that:

type Pointer[T any] struct {
	// We need this field to disallow type conversion.
	// See issue golang/go#56603 for more details.
	_ T

	_ noCopy
	v unsafe.Pointer
}

But it will probably increase the size of the struct.

Edit:
This will not increase the size of the struct:

type noTypeConversion[T any] struct{}
type Pointer[T any] struct {
	// We need this field to disallow type conversion.
	// See issue golang/go#56603 for more details.
	_ noTypeConversion[T]
       (...)
}

Note: This is obviously a breaking change, but I don't think that we want to allow that kind of misuse.

gopherbot · 2022-11-06T10:00:59Z

Change https://go.dev/cl/448275 mentions this issue: sync/atomic: disallow type conversions of atomic.Pointer[T]

mknyszek · 2022-11-07T06:18:22Z

CC @rsc

I agree any such type conversion should at least be forced to go through something that says "unsafe." The one concern I would have with this is breaking backwards compatibility. Programs will fail to compile that previously compiled successfully.

mateusz834 · 2022-11-07T07:28:36Z

@mknyszek I think that most people using generics don't even know that this kind of conversions is allowed (I discovered them recently by a mistake).

go101 · 2022-11-07T14:33:57Z

It looks the code only runs well with Go 1.19.1, but panics with 1.19, 1.19.2 and 1.19.3.

[edit], Ah, the program panics randomly, with any Go 1.19 versions.

mknyszek · 2022-11-07T14:49:43Z

I think that most people using generics don't even know that this kind of conversions is allowed

That may be true, but it only takes one upstream dependency breaking to break a whole bunch of code. That being said, this seems serious enough that it's worth breaking programs over. @aclements also pointed out to me that it's a compile error not a runtime error so the issue will be much more immediately apparent to whoever runs into it.

go101 · 2022-11-08T04:13:17Z

Should this be backported to 1.19.4?

ianlancetaylor · 2022-11-08T05:03:55Z

@gopherbot Please backport to 1.19.

This will make it less likely for people to write invalid type conversions with generic atomic types.

gopherbot · 2022-11-08T05:04:20Z

Backport issue(s) opened: #56638 (for 1.19).

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

gopherbot · 2022-11-08T05:10:10Z

Change https://go.dev/cl/448518 mentions this issue: [release-branch.go1.19] sync/atomic: disallow type conversions of atomic.Pointer[T]

…mic.Pointer[T] For #56603. Fixes #56638. Change-Id: I6af9d80201025ae4028bfaa4a62e5de9ac0c501d GitHub-Last-Rev: e6ed5e1 GitHub-Pull-Request: #56604 Reviewed-on: https://go-review.googlesource.com/c/go/+/448275 Reviewed-by: Michael Knyszek <mknyszek@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> (cherry picked from commit 6bead8f) Reviewed-on: https://go-review.googlesource.com/c/go/+/448518 Run-TryBot: Michael Knyszek <mknyszek@google.com>

gopherbot · 2022-11-15T14:58:59Z

Change https://go.dev/cl/450655 mentions this issue: sync/atomic: allow linked list of atomic pointers again

For #56603, CL 448275 added a _ [0]T field to atomic.Pointer, so that different kinds of atomic.Pointer are not convertible. Unfortunately, that breaks code like: type List struct { Next atomic.Pointer[List] } which should be valid, just as using Next *List is valid. Instead, we get: ./atomic_test.go:2533:6: invalid recursive type List ./atomic_test.go:2533:6: List refers to ./atomic_test.go:2534:13: "sync/atomic".Pointer refers to ./atomic_test.go:2533:6: List Fix by using _[0]*T instead. Change-Id: Icc4c83c691d35961d20cb14b824223d6c779ac5e Reviewed-on: https://go-review.googlesource.com/c/go/+/450655 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com>

mateusz834 · 2022-11-21T17:19:13Z

We should also backport the CL 450655 for go 1.19.4.

mknyszek · 2022-11-21T17:22:26Z

Sent https://go.dev/cl/452438.

gopherbot · 2022-11-21T17:22:33Z

Change https://go.dev/cl/452438 mentions this issue: [release-branch.go1.19] sync/atomic: allow linked list of atomic pointers again

mateusz834 · 2022-12-06T09:57:05Z

@mknyszek we should get this submitted before go 1.19.4

…mic.Pointer[T] For golang#56603. Fixes golang#56638. Change-Id: I6af9d80201025ae4028bfaa4a62e5de9ac0c501d GitHub-Last-Rev: e6ed5e1 GitHub-Pull-Request: golang#56604 Reviewed-on: https://go-review.googlesource.com/c/go/+/448275 Reviewed-by: Michael Knyszek <mknyszek@google.com> Run-TryBot: Ian Lance Taylor <iant@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@google.com> (cherry picked from commit 6bead8f) Reviewed-on: https://go-review.googlesource.com/c/go/+/448518 Run-TryBot: Michael Knyszek <mknyszek@google.com>

…ters again For #56603, CL 448275 added a _ [0]T field to atomic.Pointer, so that different kinds of atomic.Pointer are not convertible. Unfortunately, that breaks code like: type List struct { Next atomic.Pointer[List] } which should be valid, just as using Next *List is valid. Instead, we get: ./atomic_test.go:2533:6: invalid recursive type List ./atomic_test.go:2533:6: List refers to ./atomic_test.go:2534:13: "sync/atomic".Pointer refers to ./atomic_test.go:2533:6: List Fix by using _[0]*T instead. For #56638. Fixes #57124. Change-Id: Icc4c83c691d35961d20cb14b824223d6c779ac5e Reviewed-on: https://go-review.googlesource.com/c/go/+/450655 Run-TryBot: Russ Cox <rsc@golang.org> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Michael Knyszek <mknyszek@google.com> (cherry picked from commit b14cf3d) Reviewed-on: https://go-review.googlesource.com/c/go/+/452438 Reviewed-by: Cherry Mui <cherryyz@google.com> Run-TryBot: Michael Pratt <mpratt@google.com> Reviewed-by: Michael Pratt <mpratt@google.com>

mateusz834 changed the title ~~sync/atomic: atomic.Ponter[T] can be misused with type conversions.~~ sync/atomic: atomic.Pointer[T] can be misused with type conversions. Nov 6, 2022

gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 6, 2022

mateusz834 mentioned this issue Nov 6, 2022

sync/atomic: disallow type conversions of atomic.Pointer[T] #56604

Closed

mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 7, 2022

mknyszek added this to the Backlog milestone Nov 7, 2022

gopherbot closed this as completed in 6bead8f Nov 7, 2022

gopherbot mentioned this issue Nov 8, 2022

sync/atomic: atomic.Pointer[T] can be misused with type conversions. [1.19 backport] #56638

Closed

dmitshur modified the milestones: Backlog, Go1.20 Dec 6, 2022

dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 6, 2022

sivchari mentioned this issue Feb 3, 2023

Add unusedtypeparam linter golangci/golangci-lint#3552

Closed

golang locked and limited conversation to collaborators Dec 6, 2023

gopherbot added the FrozenDueToAge label Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync/atomic: atomic.Pointer[T] can be misused with type conversions. #56603

sync/atomic: atomic.Pointer[T] can be misused with type conversions. #56603

mateusz834 commented Nov 6, 2022 •

edited

mateusz834 commented Nov 6, 2022 •

edited

gopherbot commented Nov 6, 2022

mknyszek commented Nov 7, 2022

mateusz834 commented Nov 7, 2022 •

edited

go101 commented Nov 7, 2022 •

edited

mknyszek commented Nov 7, 2022

go101 commented Nov 8, 2022

ianlancetaylor commented Nov 8, 2022

gopherbot commented Nov 8, 2022

gopherbot commented Nov 8, 2022

gopherbot commented Nov 15, 2022

mateusz834 commented Nov 21, 2022

mknyszek commented Nov 21, 2022

gopherbot commented Nov 21, 2022

mateusz834 commented Dec 6, 2022

sync/atomic: atomic.Pointer[T] can be misused with type conversions. #56603

sync/atomic: atomic.Pointer[T] can be misused with type conversions. #56603

Comments

mateusz834 commented Nov 6, 2022 • edited

mateusz834 commented Nov 6, 2022 • edited

gopherbot commented Nov 6, 2022

mknyszek commented Nov 7, 2022

mateusz834 commented Nov 7, 2022 • edited

go101 commented Nov 7, 2022 • edited

mknyszek commented Nov 7, 2022

go101 commented Nov 8, 2022

ianlancetaylor commented Nov 8, 2022

gopherbot commented Nov 8, 2022

gopherbot commented Nov 8, 2022

gopherbot commented Nov 15, 2022

mateusz834 commented Nov 21, 2022

mknyszek commented Nov 21, 2022

gopherbot commented Nov 21, 2022

mateusz834 commented Dec 6, 2022

mateusz834 commented Nov 6, 2022 •

edited

mateusz834 commented Nov 6, 2022 •

edited

mateusz834 commented Nov 7, 2022 •

edited

go101 commented Nov 7, 2022 •

edited