golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.74k stars 1.58k forks source link

TestDynamicTypesExtensionNotFound: panic: unaligned 64-bit atomic operation #1555

Closed stapelberg closed 1 year ago

stapelberg commented 1 year ago

What version of protobuf and what language are you using? Version: commit 4396dd4cd0795b750a83f9fafa6747e6c5082526 (latest at the time of writing)

What did you do? ./test.bash

I’m getting the following stack trace:

        --- FAIL: TestDynamicTypesExtensionNotFound (0.00s)
        panic: unaligned 64-bit atomic operation [recovered]
            panic: unaligned 64-bit atomic operation

        goroutine 14 [running]:
        testing.tRunner.func1.2({0x82d7ea0, 0x838b46c})
            /usr/local/google/home/stapelberg/protobuf/.cache/go1.19.6/src/testing/testing.go:1396 +0x2ab
        testing.tRunner.func1()
            /usr/local/google/home/stapelberg/protobuf/.cache/go1.19.6/src/testing/testing.go:1399 +0x41f
        panic({0x82d7ea0, 0x838b46c})
            /usr/local/google/home/stapelberg/protobuf/.cache/go1.19.6/src/runtime/panic.go:884 +0x1c3
        runtime/internal/atomic.panicUnaligned()
            /usr/local/google/home/stapelberg/protobuf/.cache/go1.19.6/src/runtime/internal/atomic/unaligned.go:8 +0x2d
        runtime/internal/atomic.Load64(0x9027b54)
            /usr/local/google/home/stapelberg/protobuf/.cache/go1.19.6/src/runtime/internal/atomic/atomic_386.s:225 +0x10
        google.golang.org/protobuf/types/dynamicpb.(*Types).FindExtensionByNumber(0x9027b48, {0x832e0bc, 0x13}, 0x64)
            /usr/local/google/home/stapelberg/protobuf/.cache/gopath/src/google.golang.org/protobuf/types/dynamicpb/types.go:86 +0x2a
        google.golang.org/protobuf/types/dynamicpb_test.TestDynamicTypesExtensionNotFound(0x8c07590)
            /usr/local/google/home/stapelberg/protobuf/.cache/gopath/src/google.golang.org/protobuf/types/dynamicpb/types_test.go:114 +0x1f4
        testing.tRunner(0x8c07590, 0x833eaf0)
            /usr/local/google/home/stapelberg/protobuf/.cache/go1.19.6/src/testing/testing.go:1446 +0x113
        created by testing.(*T).Run
            /usr/local/google/home/stapelberg/protobuf/.cache/go1.19.6/src/testing/testing.go:1493 +0x374
        FAIL    google.golang.org/protobuf/types/dynamicpb  0.034s

cc @neild

puellanivis commented 1 year ago

Well, this is weird:

type Types struct {
        files *protoregistry.Files

        extMu               sync.Mutex
        atomicExtFiles      uint64
        extensionsByMessage map[extField]protoreflect.ExtensionDescriptor
}

There’s no reason that should end up unaligned… huh, I am able to repo locally with GOARCH=386 go test. It looks like there’s an odd number of pointers ahead of atomicExtFiles, resulting in the misallignment.

I am perplexed though, why the Golang system isn’t aligning a uint64 on a 64-bit boundary. 🤔

puellanivis commented 1 year ago

It looks like according to the language spec's Size and alignment guarantees:

For a variable x of struct type: unsafe.Alignof(x) is the largest of all the values unsafe.Alignof(x.f) for each field f of x, but at least 1.

So, it’s only guaranteeing the alignment of the struct itself, not the individual fields. We could therefore safely move atomicExtFiles to the top of the struct, in order to ensure proper alignment. It’s only ever accessed atomically, so it doesn’t need to be order after the sync.Mutex indicating it is guarded by it.