-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
[BUG] Go Ethereum Does Not Adhere to Unsafe Usage Conventions With Go 1.14 Changes #20731
Comments
This bug is in Go's crypto libs. Was fixed upstream (golang/crypto@c7e5f84), we need to update our dependency. |
Ah okay, thanks. |
@karalabe are you sure? I'm still getting very similar panics, even though I'm using a very recent x/crypto version, after that fix:
Note that I don't know if the bug is in this module, or once again in the crypto module. |
This seems to be a bug in go's crypto lib, which is used by go-ethereum ethereum/go-ethereum#20731 From the Go 1.14 release notes: This release adds -d=checkptr as a compile-time option for adding instrumentation to check that Go code is following unsafe.Pointer safety rules dynamically. This option is enabled by default (except on Windows) with the -race or -msan flags, and can be disabled with -gcflags=all=-d=checkptr=0. Specifically, -d=checkptr checks the following: When converting unsafe.Pointer to *T, the resulting pointer must be aligned appropriately for T. If the result of pointer arithmetic points into a Go heap object, one of the unsafe.Pointer-typed operands must point into the same object. Using -d=checkptr is not currently recommended on Windows because it causes false alerts in the standard library.
Probable cause
It seems this feature not only breaks on windows, but also on GNU+Linux. Fix |
The fix is not to disable the check entirely, unless you're certain you've found a bug in the checkptr feature. If you're going to be disabling checks, you might as well not use |
@mvdan The problem occurs within golang's sha3 implementation. It's not something we as developers have control over. Normally, it would be bad style to do this, but the Go team seems to have released a broken version. |
Hmm, lemme try and repro this locally. If it's indeed a bug in Go, I'm sure they'd fix is quite quickly if reported. Just would be nice to repro it without all our code in the soup. |
@mvdan @RmbRT I'm still kind of confident that it's a bug in Go crypto, and that it's already been fixed upstream: Running the trie tests with 1.14 on current codebase:
Update the crpyto modules:
Rerun the trie tests:
|
I'll open a PR to update the crypto package, will probably solve a lot of issues if you don't have to jump through hoops to replace the package yourself. |
@karalabe just so we are on the same page, note that my panic trace contains an up-to-date version of the module containing the sha3 package. That version contains the fix you're referencing. I'm pretty sure we're talking about different issues here, even though they both surface because of 1.14 and the same package. |
@mvdan Oh, that's super interesting, I can repro that issue with updated crypto too. Funky, lemme dig deeper :) |
Precisely. Even if you update the version, the check can still fail. |
The smallest repro I have currently is package racerepro
import (
"testing"
"github.com/ethereum/go-ethereum/rlp"
"golang.org/x/crypto/sha3"
)
type S struct {
F1 [2048]byte
F2 [32]byte
}
func TestRacePanic(t *testing.T) {
rlp.Encode(sha3.NewLegacyKeccak256(), new(S))
} I still need to nuke out parts of |
💥 Bug's in Go package racerepro
import (
"encoding/hex"
"testing"
"golang.org/x/crypto/sha3"
)
func TestRacePanic(t *testing.T) {
b1, _ := hex.DecodeString("f90824")
b2, _ := hex.DecodeString("b908000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a00000000000000000000000000000000000000000000000000000000000000000")
hasher := sha3.NewLegacyKeccak256()
hasher.Write(b1)
hasher.Write(b2)
} |
Nice find. Good to see that the go team is already aware of the bug. It's unfortunate I didn't find the existing issue when googling just a couple of days ago. |
This seems to be a bug in go's crypto lib, which is used by go-ethereum ethereum/go-ethereum#20731 From the Go 1.14 release notes: This release adds -d=checkptr as a compile-time option for adding instrumentation to check that Go code is following unsafe.Pointer safety rules dynamically. This option is enabled by default (except on Windows) with the -race or -msan flags, and can be disabled with -gcflags=all=-d=checkptr=0. Specifically, -d=checkptr checks the following: When converting unsafe.Pointer to *T, the resulting pointer must be aligned appropriately for T. If the result of pointer arithmetic points into a Go heap object, one of the unsafe.Pointer-typed operands must point into the same object. Using -d=checkptr is not currently recommended on Windows because it causes false alerts in the standard library.
This seems to be a bug in go's crypto lib, which is used by go-ethereum ethereum/go-ethereum#20731 From the Go 1.14 release notes: This release adds -d=checkptr as a compile-time option for adding instrumentation to check that Go code is following unsafe.Pointer safety rules dynamically. This option is enabled by default (except on Windows) with the -race or -msan flags, and can be disabled with -gcflags=all=-d=checkptr=0. Specifically, -d=checkptr checks the following: When converting unsafe.Pointer to *T, the resulting pointer must be aligned appropriately for T. If the result of pointer arithmetic points into a Go heap object, one of the unsafe.Pointer-typed operands must point into the same object. Using -d=checkptr is not currently recommended on Windows because it causes false alerts in the standard library. Signed-off-by: Sebastian Stammler <[email protected]>
System information
Geth version: 1.9.11
OS & Version: Linux
Commit hash : v1.9.11 release
Go Version: 1.14
Expected behaviour
Tests with race detection should run safely and CI passes
Actual behaviour
CI builds with
-race
fail because go1.14 introduces new detection methods to better validate usage ofunsafe.Pointer
Steps to reproduce the behaviour
Write code that uses go-ethereum while using v1.14 of go. Write a test and run test with
-race
enabledBacktrace
The text was updated successfully, but these errors were encountered: