Crash using gccheckmark=1, possible GC bug?

[Tibor Halter]

Hi!

I get this crash when using the GODEBUG option gccheckmark=1:

runtime: marking free object 0xc0016c5668 found at *(0xc0046ca8e0+0x8)
base=0xc0046ca8e0 s.base()=0xc0046ca000 s.limit=0xc0046cc000 s.spanclass=8 s.elemsize=32 s.state=mSpanInUse
 *(base+0) = 0x15e11f0
 *(base+8) = 0xc0016c5668 <==
 *(base+16) = 0x28
 *(base+24) = 0xc004642030
obj=0xc0016c5668 s.base()=0xc0016c4000 s.limit=0xc0016c5ff8 s.spanclass=6 s.elemsize=24 s.state=mSpanInUse
 *(obj+0) = 0x15e0e80
 *(obj+8) = 0xc00039c080
 *(obj+16) = 0xc0016c5650
fatal error: marking free object

runtime stack:
runtime.throw({0x132362a?, 0x3?})
/usr/local/go-1.21.0/src/runtime/panic.go:1077 +0x5c fp=0x7f9636366d48 sp=0x7f9636366d18 pc=0x43aa9c
runtime.greyobject(0xc0016c5668, 0x1?, 0x7f9636366df8?, 0x6590?, 0x7f9636366df8?, 0x7f9636366de0?)
/usr/local/go-1.21.0/src/runtime/mgcmark.go:1476 +0x285 fp=0x7f9636366d98 sp=0x7f9636366d48 pc=0x424d85
runtime.scanobject(0xc0046ca8e0, 0xc00004d240)
/usr/local/go-1.21.0/src/runtime/mgcmark.go:1336 +0x171 fp=0x7f9636366e20 sp=0x7f9636366d98 pc=0x424711
runtime.gcDrain(0xc00004d240, 0x7)
/usr/local/go-1.21.0/src/runtime/mgcmark.go:1103 +0x1ba fp=0x7f9636366e80 sp=0x7f9636366e20 pc=0x423fba
runtime.gcBgMarkWorker.func2()
/usr/local/go-1.21.0/src/runtime/mgc.go:1385 +0x6f fp=0x7f9636366ed0 sp=0x7f9636366e80 pc=0x4208af
runtime.systemstack()
/usr/local/go-1.21.0/src/runtime/asm_amd64.s:509 +0x4a fp=0x7f9636366ee0 sp=0x7f9636366ed0 pc=0x46b1aa


From the docs, I understand this 2nd pass stop-the-world verification
guarantees the concurrent mark step did not follow the pointer, aka this is a GC bug?

A confirmation would be greatly appreciated to help me follow the right trail.

Context
I'm building a type-system on top of Go for user-defined data structures. For efficient implementation, I'm using unsafe.Pointer-s heavily. I believe I understand pointer conversion rules, span classes, that there is metadata on the side to differentiate ordinary values from pointers and how it is used to follow pointer chains.
Go vet detects no issues. I've been debugging this for 4 days.

Many thanks!

[Ian Lance Taylor]

That error looks like the GC found a pointer to an object that was
already freed. This can happen, for example, if you convert a uintptr
to a pointer type, but the object to which the pointer points has been
freed. While it could be a GC bug, that would not be my first guess.
The contents of the block shown above are intended to help you
identify where the pointer came from.

Ian

[Tibor Halter]

Thanks Ian!

I have managed to strip it down to its essence, see code below.

Summary of what I'm doing
- reserve memory with make([]byte)
- get pointer to underlying array
- cast it to type Num
- write pointer #2 into it
- force GC()
- GC does not follow pointer #2, memory is freed

Notes
- If I run it with `go test` it passes.
- If I run it with `GODEBUG=clobberfree=1 go test` it fails with:
  --- FAIL: Test (0.00s)
      debug_test.go:26: v[DEADBEEFDEADBEEF]
- If I inline the variable `numMemSize` it passes.


As per the rules in unsafe.Pointer docs I would understand the conversions I'm doing are valid.
What do I miss?

Thanks a lot,
Tibor


--


package debug

import (
"testing"
"runtime"
"unsafe"
)

type Num struct {
numData *uint64
}

func Test(t *testing.T) {
n := uint64(123456789)
numMemSize := 8
b := make([]byte, numMemSize, numMemSize)
sliceDataPtr := unsafe.Pointer(unsafe.SliceData(b))
(*Num)(sliceDataPtr).numData = &n


runtime.GC()


act := (*Num)(sliceDataPtr).numData
if *act != 123456789 {
t.Fatalf("v[%X]", *act)
}
}

[Tibor Halter]

I am testing on Go 1.21.0, Linux, amd64

[ma...@eliasnaur.com]

On Friday, 18 August 2023 at 05:57:38 UTC-6 Tibor Halter wrote:

Thanks Ian!

I have managed to strip it down to its essence, see code below.

Summary of what I'm doing
- reserve memory with make([]byte)
- get pointer to underlying array
- cast it to type Num
- write pointer #2 into it
- force GC()
- GC does not follow pointer #2, memory is freed

The Go garbage collector is precise in that it only considers pointers when determining memory that can be freed. In your scenario and demonstration program you explicitly store a pointer in a byte slice, effectively making it invisible to the collector.

 

--


package debug

import (
"testing"
"runtime"
"unsafe"
)

type Num struct {
numData *uint64
}

func Test(t *testing.T) {
n := uint64(123456789)
numMemSize := 8
b := make([]byte, numMemSize, numMemSize)
sliceDataPtr := unsafe.Pointer(unsafe.SliceData(b))
(*Num)(sliceDataPtr).numData = &n

The pointer to &n stored in b here is effectively hidden from the collector.

 

runtime.GC()

With no more visible references to (the supposedly heap-allocated) n, it can be freed.

 

act := (*Num)(sliceDataPtr).numData
if *act != 123456789 {
t.Fatalf("v[%X]", *act)
}
}

Elias

[Tibor Halter]

Hi Elias, thanks for chiming in!

> The Go garbage collector is precise in that it only considers pointers when determining memory that can be freed.

Agreed, but it is not as precise on _what_ is a pointer. Specifically, which matters?
- the type used upon allocating the memory, or
- the type used upon writing to it.

Because *T or unsafe.Pointer is required upon writing to a memory location for it to be marked as a pointer, I came to the conclusion that the type upon writing is what matters.

So then both types need to be pointers?

Does that mean there is no way to define a struct-like memory layout dynamically, that contains a mix of pointer and non-pointer types, in a way that is supported by the GC?

(This is what I'm trying to do.)

[Tibor Halter]

For the record, allocating the memory as []unsafe.Pointer fixes the issues. (Working example below.)

That said, I'm not sure this is a future proof idea. It seems the GC handles it correctly now, but may change in the future.

I'd greatly prefer to find a spec-compliant solution...

package debug

import (
"testing"
"runtime"
"unsafe"
)

type Num struct {

val uint64
ptr *uint64
}
func Test(t *testing.T) {
var sliceDataPtr unsafe.Pointer
func() {
n := uint64(123456789)
numMemSize := 2
b := make([]unsafe.Pointer, numMemSize, numMemSize)

sliceDataPtr = unsafe.Pointer(unsafe.SliceData(b))
numVal := (*Num)(sliceDataPtr)
numVal.val = 123
numVal.ptr = &n

escape(sliceDataPtr) // force on heap
}()


runtime.GC()


numVal := (*Num)(sliceDataPtr)
if numVal.val != 123 {
t.Fatalf("v[%X]", numVal.val)
}
if *numVal.ptr != 123456789 {
t.Fatalf("v[%X]", *numVal.ptr)
}
}

var sink any
func escape(x any) {
sink = x
sink = nil
}

[Ian Lance Taylor]

On Fri, Aug 18, 2023 at 8:12 AM Tibor Halter <tha...@zupa.hu> wrote:
>
> Hi Elias, thanks for chiming in!
>
> > The Go garbage collector is precise in that it only considers pointers when determining memory that can be freed.
>
> Agreed, but it is not as precise on _what_ is a pointer. Specifically, which matters?
> - the type used upon allocating the memory, or
> - the type used upon writing to it.
>
> Because *T or unsafe.Pointer is required upon writing to a memory location for it to be marked as a pointer, I came to the conclusion that the type upon writing is what matters.
>
> So then both types need to be pointers?
>
> Does that mean there is no way to define a struct-like memory layout dynamically, that contains a mix of pointer and non-pointer types, in a way that is supported by the GC?
> (This is what I'm trying to do.)

The rule is pretty simple: a pointer must always have a pointer type.
That includes when it is in a variable and when it is stored in
memory. In your terms, what matters is the type used upon allocating
the memory.

Your code is not following any of the patterns permitted for
unsafe.Pointer (any patterns that are not explicitly permitted are
forbidden). The code is effectively converting from *byte to *Num.
As it says at https://pkg.go.dev/unsafe#Pointer that kind of
conversion is only permitted if the types in question "share an
equivalent memory layout". byte and Num have different memory
layouts.

You can define a struct-like memory layout dynamically by using
reflect.StructOf.

Ian

> --
> You received this message because you are subscribed to the Google Groups "golang-nuts" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/5051da1c-36de-4699-906c-ff8f50754404n%40googlegroups.com.

[Tibor Halter]

Ah reflect.StructOf! I totally missed that, that was the missing piece.

Thanks for the detailed response Ian, much appreciated.

Tibor