x/crypto/sha3: unsafe pointer conversion #37644

albrow · 2020-03-04T01:30:59Z

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

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

albrow · 2020-03-04T01:31:09Z

Possibly related to #35173

bcmills · 2020-03-04T01:37:43Z

CC @FiloSottile @katiehockman @mdempsky

See also #37298, #35381.

albrow · 2020-03-05T22:01:02Z

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

Is a false positive and a bug in checkptr or
Is checkptr is behaving as intended and this a bug in x/crypto?

randall77 · 2020-03-06T00:10:15Z

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 · 2020-03-06T00:15:09Z

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.

randall77 · 2020-03-06T00:20:10Z

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

bcmills · 2020-03-06T14:19:28Z

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

OneOfOne · 2020-03-06T23:41:45Z

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

mdempsky · 2020-03-07T00:59:30Z

@bcmills My rationale is that:

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

bcmills · 2020-03-07T02:01:15Z

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.

mvdan · 2020-03-07T11:25:35Z

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 · 2020-03-07T17:27:33Z

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

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

bcmills · 2020-03-09T18:35:12Z

See also #37298.

rsc · 2020-03-10T15:29:28Z

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

Upstream issue: golang/go#37644

gopherbot · 2020-03-11T13:33:04Z

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

It is unclear whether unaligned reads should be allowed, or if they are even actually a good idea here. However, while we figure that out, we should un-break 'go test -race' for users of this package. Updates golang/go#37644 Updates golang/go#37298 Updates golang/go#37715 Updates golang/go#34972 Updates golang/go#35128 Change-Id: I088f5703023e4f05ee274a6753e925973f12ac1b Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222855 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>

bcmills

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.

mdempsky · 2020-03-11T17:21:57Z

Thanks @bcmills . That sounds reasonable to me.

Upstream issue: golang/go#37644

After add -race flag when test, got this error in old sha3 library: x/crypto: fatal error: checkptr: unsafe pointer conversion (golang/go#37644) Since they have already fixed the issue, it's better to switch to official crypto library.

Add cfg.SaveToFile() Add ParseMediaType Update config server, todo: remove yaml config file Update config server Add go-playground/validator Remove diode_go_client/crypto/sha3 After add -race flag when test, got this error in old sha3 library: x/crypto: fatal error: checkptr: unsafe pointer conversion (golang/go#37644) Since they have already fixed the issue, it's better to switch to official crypto library. Update config api server Update .travis.yml Got fatal error: checkptr: unsafe pointer conversion in crypto library, the issue was fixed and released in 1.4.1. see go 1.4.1 release milestone for more information: https://github.com/golang/go/issues?q=milestone%3AGo1.14.1+label%3ACherryPickApproved

mdempsky · 2020-08-11T05:08:38Z

#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?

gopherbot · 2020-08-19T18:50:29Z

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

As of Go 1.14.1, -d=checkptr no longer errors on unaligned reads of non-pointer data. This reverts the code change (but not the test) from CL 222855. Fixes golang/go#37644 Updates golang/go#37298 Change-Id: I935c773a3541ed8dca7eb005d39a082eb5f10eb8 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/249378 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>

gopherbot added this to the Unreleased milestone Mar 4, 2020

bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 4, 2020

bcmills mentioned this issue Mar 6, 2020

x/crypto: fatal error: checkptr: unsafe pointer conversion #37715

Closed

OneOfOne mentioned this issue Mar 6, 2020

Look into checkptr OneOfOne/xxhash#30

Open

bcmills mentioned this issue Mar 9, 2020

cmd/compile: -d=checkptr should not reject unaligned pointers to non-pointer data #37298

Closed

bwesterb mentioned this issue Mar 11, 2020

Shake is crashing when using unsafe pointer casting cloudflare/circl#89

Closed

bwesterb added a commit to bwesterb/circl that referenced this issue Mar 11, 2020

Temporary solution to cloudflare#89 waiting on upstream.

50b6118

Upstream issue: golang/go#37644

bwesterb mentioned this issue Mar 11, 2020

Fix #89 cloudflare/circl#95

Merged

bcmills mentioned this issue Mar 11, 2020

crypto/cipher: move xorBytes function to internal #28465

Closed

bwesterb added a commit to bwesterb/circl that referenced this issue Mar 12, 2020

Temporary solution to cloudflare#89 waiting on upstream.

d1189c2

Upstream issue: golang/go#37644

gopherbot closed this as completed in golang/crypto@5c72a88 Aug 20, 2020

golang locked and limited conversation to collaborators Aug 20, 2021

gopherbot added the FrozenDueToAge label Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/crypto/sha3: unsafe pointer conversion #37644

x/crypto/sha3: unsafe pointer conversion #37644

x/crypto/sha3: unsafe pointer conversion #37644

x/crypto/sha3: unsafe pointer conversion #37644

Comments

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

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

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