Bug in the way that data is passed down to callback functions

[plusvic]

While investigating some other issue I accidentally hit a bug that seems related to the mechanism for passing data to callback functions. By adding the following test case to rules_test.go you should be able to trigger the bug:

func TestBug(t *testing.T) {
	ch := make(chan bool, 0)
	go func() {
		for i := 0; i < 10000; i++ {
			makeRules(t, "rule test { strings: $a = /a.*a/ condition: $a }")
			time.Sleep(1 * time.Millisecond)
		}
		ch <- true
	}()

Loop:
	for {
		select {
		case <-ch:
			break Loop
		default:
			runtime.GC()
			fmt.Println("GC")
		}
	}
}

The bug can manifest itself in two ways, by panicing in line 44 of callback-util.go or by raising the following error:

runtime: nelems=512 nfree=501 nalloc=11 previous allocCount=10 nfreed=65535
fatal error: sweep increased allocation count

runtime stack:
runtime.throw(0x4174515, 0x20)
	/usr/local/go/src/runtime/panic.go:596 +0x95 fp=0x70000e1b3cd8 sp=0x70000e1b3cb8
runtime.(*mspan).sweep(0x44042f8, 0x4404200, 0x70000e1b3d00)
	/usr/local/go/src/runtime/mgcsweep.go:288 +0x786 fp=0x70000e1b3da8 sp=0x70000e1b3cd8
runtime.sweepone(0x0)
	/usr/local/go/src/runtime/mgcsweep.go:110 +0x1ad fp=0x70000e1b3e10 sp=0x70000e1b3da8
runtime.gcSweep(0x2)
	/usr/local/go/src/runtime/mgc.go:1786 +0xd4 fp=0x70000e1b3e40 sp=0x70000e1b3e10
runtime.gcMarkTermination.func2()
	/usr/local/go/src/runtime/mgc.go:1261 +0x62 fp=0x70000e1b3e60 sp=0x70000e1b3e40
runtime.systemstack(0xc42001e600)
	/usr/local/go/src/runtime/asm_amd64.s:327 +0x79 fp=0x70000e1b3e68 sp=0x70000e1b3e60
runtime.mstart()
	/usr/local/go/src/runtime/proc.go:1132 fp=0x70000e1b3e70 sp=0x70000e1b3e68

In my case commenting out the line fmt.Println("GC") switch from one error to the other, but your mileage may vary. It was the panic in callback-util.go what lead me find a possible explanation for what's happening.

Take a look at this:

C.yr_compiler_set_callback(c.cptr, C.YR_COMPILER_CALLBACK_FUNC(C.compilerCallback), unsafe.Pointer(&id))

This passes to the callback the address of id, as an unsafe.Pointer. My theory is that the garbage collector is moving id around before compilerCallback is called. By the time compilerCallback is called the address where id was previously stored contains different data.

Thoughts?

[plusvic]

This fix seems to work, but I'm not familiarized yet withcgo.

With that fixgo vet says: possible misuse of unsafe.Pointer because of the conversion of anuintptr back tounsafe.Pointer, but in this case I would say that's perfectly safe. The conversion is necessary only because yr_compiler_set_callback expects a void* for user_data and cgo translates it into unsafe.Pointer.

[hillu]

I have been able to reproduce the issue with Go 1.8.5 but not with Go 1.9.2 (Debian unstable/amd64). What version are you using?

My belief is that the garbage collector collects id right after it has been passed to C.compiler_set_callback.. Adding keepalive(id) at the end of the function seems to fix the issue here.

[plusvic]

I have Go 1.8 too.

Your belief makes sense. At first I thought that too, but then read that the GC doesn't free the object if you hold an unsafe.Pointer to it, and discarded the idea. What I didn't noticed is that the pointer is not hold by the function neither, so nothing prevents the GC from freeing id.

[plusvic]

BTW, what do you think about plusvic@7275017, I believe it's safe even if go vet complains, but I would like to hear a second opinion. I like it because you don't need to pass the address of id to the callback, you can pass id itself.

[hillu]

I have gone through a few iterations of trying to use integers / uintptr values where the Go compiler and/or runtime thought it was passing pointers around. It would work with the then-current compiler/runtime version but then the CGO rules (which are underdocumented anyway) would get changed and things would break.

At this point I have pretty much given up on that idea. Let's see if explicitly keeping the id values alive will do the trick...

[hillu]

Let's see what the Travis CI setup comes up with...

[hillu]

Woohoo, it crashed with every Go version but tip.

[hillu]

Ah well, it seems like the keepAlive function for go 1.6 also has a beneficial effect in this case. I am getting too old for this nonsense.