golang/go

x/crypto/sha3: unsafe pointer conversion

Closed this issue · 19 comments

albrow commented

What version of Go are you using (go version)?

$ go version
go version go1.14 darwin/amd64

Does this issue reproduce with the latest release?

Go 1.14 is the latest release as of opening this issue, so yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/alex/Library/Caches/go-build"
GOENV="/Users/alex/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/alex/programming/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/alex/.go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/alex/.go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/alex/programming/gomod/sha3-issue-repro/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/mf/g899kr2d4y1_mn3qy7pf3b0h0000gn/T/go-build583899136=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

After upgrading to Go 1.14 I noticed some test failing with the error message:

fatal error: checkptr: unsafe pointer conversion

The error only seems to occur when running tests with the -race flag. I was able to isolate the issue and create a somewhat minimal test to reproduce it:

package repro

import (
	"testing"

	"golang.org/x/crypto/sha3"
)

func TestSha3(t *testing.T) {
	k := []byte("this is a secret key; you should generate a strong random key that's at least 32 bytes long")
	buf := []byte{160, 69, 93, 207, 101, 230, 146, 196, 225, 119, 226, 222, 98, 172, 64, 93, 228, 135, 142, 216, 77, 7, 244, 36, 227, 157, 52, 224, 219, 66, 194, 174, 94, 160, 29, 204, 77, 232, 222, 199, 93, 122, 171, 133, 181, 103, 182, 204, 212, 26, 211, 18, 69, 27, 148, 138, 116, 19, 240, 161, 66, 253, 64, 212, 147, 71, 148, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 160, 229, 174, 236, 88, 233, 99, 68, 76, 119, 53, 56, 242, 58, 80, 253, 215, 9, 241, 163, 86, 38, 110, 224, 163, 179, 247, 171, 101, 15, 19, 87, 144, 160, 209, 47, 96, 255, 84, 213, 125, 96, 111, 10, 119, 234, 132, 74, 224, 250, 149, 82, 191, 222, 100, 212, 4, 242, 64, 88, 168, 191, 169, 188, 81, 36, 160, 252, 224, 45, 71, 222, 125, 170, 50, 73, 132, 192, 244, 207, 163, 40, 184, 120, 13, 117, 95, 242, 65, 52, 118, 84, 7, 101, 135, 121, 135, 200, 205, 185, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 128, 53, 131, 137, 84, 64, 131, 26, 145, 223, 132, 94, 40, 147, 106, 128, 160, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 136, 0, 0, 0, 0, 0, 0, 0, 0}
	d := sha3.NewLegacyKeccak256()
	d.Write(k)
	d.Write(buf)
}

The issue does not happen with all possible values for buf. I'm not sure what is special about this specific value or what categories of values cause the bug. It seems that shorter values work in general.

I'm using golang.org/x/crypto@v0.0.0-20200302210943-78000ba7a073 which is the latest release as of opening this issue.

What did you expect to see?

The minimal test I shared above should pass and not trigger any errors.

What did you see instead?

The test does not pass. Here's the full output:

go test -race .
fatal error: checkptr: unsafe pointer conversion

goroutine 19 [running]:
runtime.throw(0x121c484, 0x23)
	/Users/alex/.go/src/runtime/panic.go:1112 +0x72 fp=0xc00004c560 sp=0xc00004c530 pc=0x1070032
runtime.checkptrAlignment(0xc00015402d, 0x11de3e0, 0x11)
	/Users/alex/.go/src/runtime/checkptr.go:13 +0xd0 fp=0xc00004c590 sp=0xc00004c560 pc=0x1044100
golang.org/x/crypto/sha3.xorInUnaligned(0xc000156000, 0xc00015402d, 0x88, 0x1c9)
	/Users/alex/programming/go/pkg/mod/golang.org/x/crypto@v0.0.0-20200302210943-78000ba7a073/sha3/xor_unaligned.go:21 +0x65 fp=0xc00004c5d0 sp=0xc00004c590 pc=0x11c3bd5
golang.org/x/crypto/sha3.(*state).Write(0xc000156000, 0xc00015402d, 0x1f6, 0x1c9, 0x5b, 0x0, 0x0)
	/Users/alex/programming/go/pkg/mod/golang.org/x/crypto@v0.0.0-20200302210943-78000ba7a073/sha3/sha3.go:138 +0x19e fp=0xc00004c670 sp=0xc00004c5d0 pc=0x11c2ece
github.com/albrow/sha3-issue-repro.TestSha3(0xc000150120)
	/Users/alex/programming/gomod/sha3-issue-repro/sha3_test.go:14 +0x1f9 fp=0xc00004c6d0 sp=0xc00004c670 pc=0x11c9689
testing.tRunner(0xc000150120, 0x121f450)
	/Users/alex/.go/src/testing/testing.go:992 +0x1ec fp=0xc00004c7d0 sp=0xc00004c6d0 pc=0x115d1ac
runtime.goexit()
	/Users/alex/.go/src/runtime/asm_amd64.s:1373 +0x1 fp=0xc00004c7d8 sp=0xc00004c7d0 pc=0x10a1701
created by testing.(*T).Run
	/Users/alex/.go/src/testing/testing.go:1043 +0x661

goroutine 1 [chan receive]:
testing.(*T).Run(0xc000150000, 0x12165da, 0x8, 0x121f450, 0x0)
	/Users/alex/.go/src/testing/testing.go:1044 +0x699
testing.runTests.func1(0xc000150000)
	/Users/alex/.go/src/testing/testing.go:1285 +0xa7
testing.tRunner(0xc000150000, 0xc000131d50)
	/Users/alex/.go/src/testing/testing.go:992 +0x1ec
testing.runTests(0xc000134020, 0x1361d20, 0x1, 0x1, 0x0)
	/Users/alex/.go/src/testing/testing.go:1283 +0x528
testing.(*M).Run(0xc00014c000, 0x0)
	/Users/alex/.go/src/testing/testing.go:1200 +0x300
main.main()
	_testmain.go:44 +0x224
FAIL	github.com/albrow/sha3-issue-repro	0.172s
FAIL
albrow commented

Possibly related to #35173

albrow commented

Maybe someone on the Go team could provide some more clarity:

  1. Is a false positive and a bug in checkptr or
  2. Is checkptr is behaving as intended and this a bug in x/crypto?
randall77 commented

This is definitely related to some of those issues, but I suspect it is its own thing.
The code here is basically:

(*[N]byte)(unsafe.Pointer(p))[:x:x]

The code guarantees that p points to an object of size at least x. But very temporarily, this generates a pointer to an object of size N. N may be bigger than x. If that causes the *[N]byte to look like it straddles objects, checkptr will complain.

It may be worth adding the pattern above as a special case. (@mdempsky: did we not already do that?)
Or convert the slice of byte to a slice of word some other way. Unfortunately that would only cover this instance, and we've been recommending this pattern for a while now, so I'm sure there are other cases like this in the wild.

mdempsky commented

The issue here is that it's creating an unaligned pointer: checkptr requires that *uint64 has to point to an unsafe.Alignof(uint64(0))-aligned address. (More generally: a *T pointer has to be unsafe.Alignof(*new(T))-aligned.)

It looks like that code is only used on x86 and ppc64le, so it's probably fine to annotate xorInUnaligned with //go:nocheckptr.

  • 😕1
randall77 commented

Indeed, you're right, it's alignment not length.

bcmills commented

@mdempsky, if pointers to unaligned data really are benign on x86 and ppc64le, then arguably checkptr should not enforce alignment on those platforms.

A //go:nocheckptr annotation should not be necessary for behaviors that are both not incorrect and not disallowed by the language (or unsafe package) spec.

  • 👍2
OneOfOne commented

Not a dup but #19367 would make everyone's life much easier.

mdempsky commented

@bcmills My rationale is that:

  1. I don't think the Go spec / unsafe.Pointer safety rules unambiguously explain whether misaligned pointers are allowed. For example, the spec asserts "uintptr(unsafe.Pointer(&x)) % unsafe.Alignof(x) == 0". Arguably, this extends to "uintptr(unsafe.Pointer(&*p)) % unsafe.Alignof(*p) == 0", which isn't satisfied if p is a misaligned pointer.

  2. Even if misaligned pointers are safe on some architectures, I think it's good practice for users to be warned that their code won't be safe on other architectures. In the case of x/crypto/sha3, the code is already explicitly annotated to only build on x86 and ppc64le, so it seems reasonable to require a //go:nocheckptr annotation too.

    Theoretically even better would be cmd/compile could recognize the build tags and know the code will never run on a strict alignment CPU, but currently cmd/compile doesn't pay attention to build tags at all.

I'm open to being convinced otherwise though.

@OneOfOne The issue here is alignment, not size, of the slice. unsafe.Slice wouldn't help with that.

  • 👍1
bcmills commented

If unaligned pointers to integer types are not unambiguously allowed, then x/crypto should be changed not to use them: x/crypto is not coupled to the gc compiler and should be correct with any compiler that supports the language.

On the other hand, if unaligned pointers are safe on this architecture, they should not result in an error on this architecture. If we allow false-positives in race-mode checks, people will start ignoring (or annotating away) actual race-mode failures on the (mistaken) theory that they're false-positives.

  • 👍2
mvdan commented

This is way out of my league, but just my two cents - some large modules like go-ethereum are affected by this, so their downstreams (like me) are unable to move to Go 1.14 if they want to keep using -race. Even if we don't agree on a long-term solution right now, we should have a workaround available as soon as possible :)

mdempsky commented

I think the immediate fix/workaround is to add //go:nocheckptr.

We can continue discussing whether that's the most appropriate fix afterwards.

  • 👍2
bcmills commented

See also #37298.

rsc commented

100% agree with last @mdempsky comment - please fix x/crypto/sha3 with //go:nocheckptr.
Let's have the discussion about whether x/crypto/sha3 is broken separately (in #37298).

gopherbot commented

Change https://golang.org/cl/222855 mentions this issue: sha3: mark xorInUnaligned with go:nocheckptr

bcmills commented

The //go:nocheckptr workaround is committed. I'd like to leave this issue open to either update the implementation or update the comment based on the outcome of #37298.

  • 👍1
mdempsky commented

Thanks @bcmills . That sounds reasonable to me.

mdempsky commented

#37298 has been closed with the decision that checkptr will not warn about misaligned pointers-to-non-pointer types. So I think x/crypto/sha3 can remove //go:nocheckptr and this issue closed?

  • 👍1
gopherbot commented

Change https://golang.org/cl/249378 mentions this issue: sha3: remove go:nocheckptr annotation