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.

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

x/crypto/sha3: unsafe pointer conversion #37644

Closed
albrow opened this issue Mar 4, 2020 · 19 comments
Closed

x/crypto/sha3: unsafe pointer conversion #37644

albrow opened this issue Mar 4, 2020 · 19 comments

Comments

@albrow
Copy link
Contributor

@albrow albrow commented Mar 4, 2020

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
@gopherbot gopherbot added this to the Unreleased milestone Mar 4, 2020
@albrow
Copy link
Contributor Author

@albrow albrow commented Mar 4, 2020

Possibly related to #35173

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 4, 2020

@albrow
Copy link
Contributor Author

@albrow albrow commented Mar 5, 2020

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
Copy link
Contributor

@randall77 randall77 commented Mar 6, 2020

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
Copy link
Member

@mdempsky mdempsky commented Mar 6, 2020

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
Copy link
Contributor

@randall77 randall77 commented Mar 6, 2020

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

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 6, 2020

@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
Copy link
Contributor

@OneOfOne OneOfOne commented Mar 6, 2020

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

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 7, 2020

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

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 7, 2020

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
Copy link
Member

@mvdan mvdan commented Mar 7, 2020

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
Copy link
Member

@mdempsky mdempsky commented Mar 7, 2020

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

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

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 9, 2020

See also #37298.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2020

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
Copy link

@gopherbot gopherbot commented Mar 11, 2020

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

gopherbot pushed a commit to golang/crypto that referenced this issue Mar 11, 2020
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
Copy link
Member

@bcmills bcmills commented Mar 11, 2020

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
Copy link
Member

@mdempsky mdempsky commented Mar 11, 2020

Thanks @bcmills . That sounds reasonable to me.

bwesterb added a commit to bwesterb/circl that referenced this issue Mar 12, 2020
sc0Vu added a commit to diodechain/diode_go_client that referenced this issue Jun 28, 2020
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.
sc0Vu added a commit to diodechain/diode_go_client that referenced this issue Jun 29, 2020
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.
sc0Vu added a commit to diodechain/diode_go_client that referenced this issue Jun 29, 2020
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
Copy link
Member

@mdempsky mdempsky commented Aug 11, 2020

#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
Copy link

@gopherbot gopherbot commented Aug 19, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.