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

[mateusz834]

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.

[mateusz834]

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]

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

[mknyszek]

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]

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

[zigo101]

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]

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.

[zigo101]

Should this be backported to 1.19.4?

[ianlancetaylor]

@gopherbot Please backport to 1.19.

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

[gopherbot]

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]

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

[gopherbot]

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

[mateusz834]

We should also backport the CL 450655 for go 1.19.4.

[mknyszek]

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

[gopherbot]

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

[mateusz834]

@mknyszek we should get this submitted before go 1.19.4