Flaky "unsafe pointer conversion"-panics with -race and go1.14rc1

[imsodin]

I cannot reproduce the panic below with a minimal example and it occurs very often, but neither all the time nor in the same test - so this report might be entirely unhelpful, but I am reporting it anyway in case it makes any sense to you:

When running the Syncthing database testsuite on go1.14rc1 with race detection enabled on linux/amd64 I get the following panic:

$ go test -v -race -short -count 1 github.com/syncthing/syncthing/lib/db/ 2>&1 | tee test.out
fatal error: checkptr: unsafe pointer conversion

goroutine 148 [running]:
runtime.throw(0xd106c0, 0x23)
	/media/ext4_data/Linux/source/go/src/runtime/panic.go:1112 +0x72 fp=0xc00022ba50 sp=0xc00022ba20 pc=0x4630d2
runtime.checkptrAlignment(0xc00011e3f1, 0xc40880, 0x1)
	/media/ext4_data/Linux/source/go/src/runtime/checkptr.go:13 +0xd0 fp=0xc00022ba80 sp=0xc00022ba50 pc=0x4348a0
github.com/spaolacci/murmur3.(*digest128).bmix(0xc000123c80, 0xc00011e3f1, 0x20, 0x2f, 0x0, 0x48, 0x58)
	/media/ext4_data/Coding/go/pkg/mod/github.com/spaolacci/murmur3@v1.1.0/murmur128.go:67 +0xc0 fp=0xc00022bae8 sp=0xc00022ba80 pc=0xb649f0
github.com/spaolacci/murmur3.(*digest).Write(0xc000123c80, 0xc00011e3f1, 0x20, 0x2f, 0xb64474, 0xc000123c80, 0xc000123c80)
	/media/ext4_data/Coding/go/pkg/mod/github.com/spaolacci/murmur3@v1.1.0/murmur.go:51 +0x127 fp=0xc00022bb98 sp=0xc00022bae8 pc=0xb63dd7
github.com/spaolacci/murmur3.(*digest128).Write(0xc000123c80, 0xc00011e3f1, 0x20, 0x2f, 0x10, 0x10, 0x21)
	<autogenerated>:1 +0x6a fp=0xc00022bbf8 sp=0xc00022bb98 pc=0xb6558a
github.com/willf/bloom.baseHashes(0xc00011e3f1, 0x20, 0x2f, 0x0, 0x0, 0x0, 0x0)
	/media/ext4_data/Coding/go/pkg/mod/github.com/willf/bloom@v2.0.3+incompatible/bloom.go:97 +0xb9 fp=0xc00022bc68 sp=0xc00022bbf8 pc=0xb6d179
github.com/willf/bloom.(*BloomFilter).Test(0xc000134700, 0xc00011e3f1, 0x20, 0x2f, 0xdf6fe0)
	/media/ext4_data/Coding/go/pkg/mod/github.com/willf/bloom@v2.0.3+incompatible/bloom.go:183 +0x6b fp=0xc00022bd20 sp=0xc00022bc68 pc=0xb6deab
github.com/syncthing/syncthing/lib/db.(*Lowlevel).gcBlocks(0xc000098870, 0x0, 0x0)
	/media/ext4_data/Coding/go/src/github.com/syncthing/syncthing/lib/db/lowlevel.go:570 +0x4d4 fp=0xc00022beb8 sp=0xc00022bd20 pc=0xb798b4
github.com/syncthing/syncthing/lib/db.(*Lowlevel).gcRunner(0xc000098870)
	/media/ext4_data/Coding/go/src/github.com/syncthing/syncthing/lib/db/lowlevel.go:483 +0x279 fp=0xc00022bfd8 sp=0xc00022beb8 pc=0xb78fb9
runtime.goexit()
	/media/ext4_data/Linux/source/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0xc00022bfe0 sp=0xc00022bfd8 pc=0x4969c1
created by github.com/syncthing/syncthing/lib/db.NewLowlevel
	/media/ext4_data/Coding/go/src/github.com/syncthing/syncthing/lib/db/lowlevel.go:65 +0x36a

(rest of the backtrace does not include murmur3 nor bloom).

Do you have any ideas what this might be about? Thanks in advance for any help.

[calmh]

I suspect the problem is that there is no guarantee that the alignment is correct after the pointer conversion here:

murmur3/murmur128.go

Line 68 in 539464a

t := (*[2]uint64)(unsafe.Pointer(&p[i*16]))

[calmh]

This test reproduces it reliably:

func TestUnalignedWrite(t *testing.T) {
	b := make([]byte, 128)
	for i := 0; i < 16; i++ {
		Sum128(b[i:])
	}
}

(Also one of the existing string tests)

[geoah]

Sigh. go1.14 is officially out (yay), and checkptr validation is now a real thing :D

@calmh thank you for taking the time to work on this, I stole your test to try and solve this the way go-cmp did, with #31 being the result.

Benchmark results seem a bit better than using encoding/binary but not sure if it's enough to worth keeping unsafe around there.

Sigh. Sorry it's getting late, that was completely wrong, let me try again. :D

Converting a Pointer to a uintptr produces the memory address of the value pointed at, as an integer. The usual use for such a uintptr is to print it.

Conversion of a uintptr back to Pointer is not valid in general.

A uintptr is an integer, not a reference. Converting a Pointer to a uintptr creates an integer value with no pointer semantics. Even if a uintptr holds the address of some object, the garbage collector will not update that uintptr’s value if the object moves, nor will that uintptr keep the object from being reclaimed.

If p points into an allocated object, it can be advanced through the object by conversion to uintptr, addition of an offset, and conversion back to Pointer.

I was going for something like this but checkptr still doesn't like it.

        nblocks := len(p) / 4
        for i := 0; i < nblocks; i++ {
-               k1 := *(*uint32)(unsafe.Pointer(&p[i*4]))
+               k1 := *(*uint32)(unsafe.Pointer(uintptr(unsafe.Pointer(&p[0])) + uintptr(i*4)*unsafe.Sizeof(&p[0])))

[calmh]

The check enforces this:

When converting unsafe.Pointer to *T, the resulting pointer must be aligned appropriately for T.

Any given byte is not going to be properly aligned for uint32 or uint64 so we can’t assume that we can do that conversion unfortunately.

[twmb]

Fundamentally, the unsafe code needs changing. Feel free to take my patches: twmb/murmur3@b0c3ada...HEAD

Note that using binary.LittleEndian (or my path) will resolve this repos tickets around issues across architectures, as well.

[calmh]

That patch looks better than mine, which was just a quick fix. We might end up depending on your repo instead, @twmb. :)

[mangup]

not merged. So, I've switched my code to github.com/DataDog/mmh3

[oliverpool]

The fork of @twmb seems quite nice: https://github.com/twmb/murmur3

[twmb]

FWIW DataDog/mmh3 theoretically same unaligned input problem, but I think it goes through unsafe & reflect.SliceHeader in a roundabout enough way that the new checks don't catch it:

        h := *(*reflect.SliceHeader)(unsafe.Pointer(&key))
        h.Len = nblocks * 2
        b := *(*[]uint64)(unsafe.Pointer(&h))

[kumakichi]

seems commit 229247d33b has already solved this problem, maybe we can close this issue?

[twmb]

Likely, but for what it's worth only the warning was removed, the underlying issue the warning was about still exists. Given that nobody complained about the issue in years, it may not a large issue.

"The main benefit of the check is to catch people making alignment assumptions on x86, where their program would then fail on another architecture. That just doesn't seem that much of a pain point to me. Even if it fails on another architecture, at least it does so loudly, without silently corrupting data"