Flaky "unsafe pointer conversion"-panics with -race and go1.14rc1 about murmur3 HOT 15 OPEN

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

from murmur3.

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.

from murmur3.

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

from murmur3.

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)

from murmur3.

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

from murmur3.

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.

from murmur3.

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

from murmur3.

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

from murmur3.

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

from murmur3.

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

from murmur3.

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"

from murmur3.

I am facing similar issue when migrating to go1.14.x

fatal error: checkptr: unsafe pointer arithmetic

goroutine 26 [running]:
runtime.throw(0xecd19c, 0x23)
/usr/local/go/src/runtime/panic.go:1116 +0x72 fp=0xc000054d68 sp=0xc000054d38 pc=0x467402
runtime.checkptrArithmetic(0xc000054e38, 0x0, 0x0, 0x0)
/usr/local/go/src/runtime/checkptr.go:43 +0xb5 fp=0xc000054d98 sp=0xc000054d68 pc=0x438a95
github.com/spaolacci/murmur3.Sum32WithSeed(0xc000054e38, 0x1b, 0x20, 0xc000000000, 0x1b)
/go/pkg/mod/github.com/spaolacci/[email protected]/murmur32.go:129 +0x8a fp=0xc000054e00 sp=0xc000054d98 pc=0xcf070a
github.com/spaolacci/murmur3.Sum32(...)
/go/pkg/mod/github.com/spaolacci/[email protected]/murmur32.go:111

from murmur3.

This is still broken in go1.15.5 darwin/amd64 for me. In particular, murmur32 is tripping checkptr even when the pointer is actually aligned. Seems like a bug in Go to me.

from murmur3.