Go pointer passing restriction violated in out_multiinstance?

[l2dy]

A Go function called by C code may take C pointers as arguments, and it may store non-pointer or C pointer data through those pointers, but it may not store a Go pointer in memory pointed to by a C pointer. - https://golang.org/cmd/cgo/#hdr-Passing_pointers (emphasis mine)

In the out_multiinstance example, pointer of the Go string id is stored in memory pointed to by a C pointer (plugin argument of FLBPluginInit) in FLBPluginSetContext, which I think violates the restrictions on passing pointers between Go and C.

[jasonkeene]

Hey @l2dy, thanks for pointing this out. I have been reading through the cgo docs and I have to admit my understanding of the restrictions of passing memory between C and Go is not 100%. I have asked around in the Gophers slack about these rules and the documentation to better clarify my understanding.

From what I understand, what you described here might not be an issue because the call to C.GoString creates a copy of the data from C and does not reference that information. If this is not the case please let me know.

Additionally, the other problem we have ran into with sharing context in this way is if you use a fully managed Go value as the context Go's GC will reuse this memory after the Init function completes since it think's there is no references remaining. This is easily fixed via running runtime.KeepAlive(&myGoValue). We are going to update the docs for this use case.

If you are able to reproduce an issue with the example I would love to investigate further. Please let us know if you have problems with it!

[l2dy]

Hey @l2dy, thanks for pointing this out. I have been reading through the cgo docs and I have to admit my understanding of the restrictions of passing memory between C and Go is not 100%. I have asked around in the Gophers slack about these rules and the documentation to better clarify my understanding.

From what I understand, what you described here might not be an issue because the call to C.GoString creates a copy of the data from C and does not reference that information. If this is not the case please let me know.

FLBPluginConfigKey creates a copy (in memory) managed by Go, so the pointer to it (unsafe.Pointer(&id)) is a Go pointer.

plugin unsafe.Pointer is a C pointer as an argument of the FLBPluginInit function called by C code, so p.context.remote_context = ctx in FLBPluginSetContext puts a Go pointer into memory managed by C, violating the restriction I highlighted.

Referencing the original C string in memory pointed to by a C pointer is OK from the Go side, but putting a Go pointer there is not.

Additionally, the other problem we have ran into with sharing context in this way is if you use a fully managed Go value as the context Go's GC will reuse this memory after the Init function completes since it think's there is no references remaining. This is easily fixed via running runtime.KeepAlive(&myGoValue). We are going to update the docs for this use case.

If you are able to reproduce an issue with the example I would love to investigate further. Please let us know if you have problems with it!

[l2dy]

Simply put,

Go code may not store a Go pointer in C memory.

[jasonkeene]

@l2dy Thanks! We were able to reproduce the issue by creating a lot of memory allocations and GC events in a separate goroutine. I think the solution we have not only violates the rule you highlighted but, having read the pointer section again, it probably violates a few other statements there.

Do you have any proposed solutions? What I was thinking is perhaps have Set/Get Context helper functions that store the context in package global slice and then just pass index into that slice around. We experimented with that just now and it seems to work.

[jasonkeene]

@wfernandes and I pushed some changes to avoid passing Go pointers into C but keep the same ABI.

fluent-bit-go/examples/out_multiinstance/out.go

Lines 21 to 22 in fc386d2

	// Set the context to point to any Go variable
	output.FLBPluginSetContext(plugin, id)

fluent-bit-go/examples/out_multiinstance/out.go

Lines 35 to 36 in fc386d2

	// Type assert context back into the original type for the Go variable
	id := output.FLBPluginGetContext(ctx).(string)

fluent-bit-go/output/output.go

Lines 70 to 81 in fc386d2

	var contexts []interface{}
	func FLBPluginSetContext(plugin unsafe.Pointer, ctx interface{}) {
	i := len(contexts)
	contexts = append(contexts, ctx)
	p := (*FLBOutPlugin)(plugin)
	p.context.remote_context = unsafe.Pointer(uintptr(i))
	}
	func FLBPluginGetContext(i unsafe.Pointer) interface{} {
	return contexts[int(uintptr(i))]
	}

Let us know what you think.

[l2dy]

fluent-bit-go/examples/out_multiinstance/out.go

Line 34 in fc386d2

func FLBPluginFlushCtx(ctx, data unsafe.Pointer, length C.int, tag *C.char) int {

[...] These types are uintptr on the Go side because they would otherwise confuse the Go garbage collector; they are sometimes not really pointers but data structures encoded in a pointer type. All operations on these types must happen in C. The proper constant to initialize an empty such reference is 0, not nil.

Should ctx be ctx uintptr? It's not really a pointer but integer encoded in a pointer type. I haven't tested the change, so if it doesn't work, please ignore this.

I'm not sure if "All operations on these types must happen in C" applies to your implementation.

---

I would use C.CString to create a copy of the Go string in C memory and put its pointer into remote_context, but your solution works too.

[l2dy]

@l2dy Thanks! We were able to reproduce the issue by creating a lot of memory allocations and GC events in a separate goroutine. I think the solution we have not only violates the rule you highlighted but, having read the pointer section again, it probably violates a few other statements there.

If your recent changes survived GC stress testing, I think we can close this issue now.

[l2dy]

Are plugins initialized in parallel? FLBPluginSetContext modifies a global variable without locks, so multiple instances of a Go plugin must be initialized sequentially.

Maintaining global state in a library is tricky, so I prefer using the C.CString method I've mentioned above and limit the context to a string.

[l2dy]

After reading the unsafe.Pointer documentation, I think the conversion between unsafe.Pointer and uintptr used in your implementation does not match any valid patterns of using unsafe.Pointer.

The following patterns involving Pointer are valid. Code not using these patterns is likely to be invalid today or to become invalid in the future. Even the valid patterns below come with important caveats.

[...]

[jasonkeene]

Are plugins initialized in parallel?

From what I remember they were initialized serially. fluent-bit is almost entirely single threaded. It will call into Go and block until the Go func has returned.

We mainly kept the unsafe.Pointer to avoid changing the function signature. I'm ok with changing it to an integer type because, as you mentioned, it is no longer a pointer.

cc/ @wfernandes

[l2dy]

I'd change it to an integer type and let FLBPluginSetContext take an integer ctx, but it's a breaking change on the C interface and the Go library, so it's up to you.

[l2dy]

I have discovered a non-breaking fix for the miuse of unsafe.Pointer, see #28.