Improve memory safety of cgo interop code. #366
Conversation
Unpinning must be done after the native handle is freed. Also do not unpin the buffers in `Query.Finalize`.
| } | ||
| runtime.KeepAlive(sa) | ||
| // The start and end pointers are being kept alive by passing them to cgo calls. |
There was a problem hiding this comment.
From documentation:
Go pointers passed as function arguments to C functions have the memory they point to implicitly pinned for the duration of the call.
| } | ||
| cpy := C.GoBytes(cbuffer, C.int(csize)) |
There was a problem hiding this comment.
We can't know whether the memory is owned by Go or C, so we always make a copy. This API is not recommended either way.
| q.bufferMutex.Lock() | ||
| defer q.bufferMutex.Unlock() | ||
| q.buffers = nil |
There was a problem hiding this comment.
We should not be unpinning the buffers in Finalize.
| @@ -118,6 +118,7 @@ func WithStartTime(start time.Time) ArrayOpenOption { | |||
| func WithEndTimestamp(endTimestamp uint64) ArrayOpenOption { | |||
| return func(tdbArray *Array) error { | |||
| ret := C.tiledb_array_set_open_timestamp_end(tdbArray.context.tiledbContext, tdbArray.tiledbArray, C.uint64_t(endTimestamp)) | |||
| runtime.KeepAlive(tdbArray) | |||
There was a problem hiding this comment.
When in the future we support Go 1.24's AddCleanup, we would have to explicitly keep the contexts alive, because the finalizers do not run in order.
Let's do it now, before marking the PR as ready for review.
There was a problem hiding this comment.
On second thought, I have misread documentation. runtime.KeepAlive does apply recursively (obviously), and as long as an object is kept alive, its finalizers will not run. This is what we want, and the order finalizers execute does not matter here.
| @@ -533,6 +555,8 @@ func DeserializeFragmentInfo(fragmentInfo FragmentInfo, buffer *Buffer, arrayURI | |||
| defer C.free(unsafe.Pointer(cArrayURI)) | |||
|
|
|||
| ret := C.tiledb_deserialize_fragment_info(fragmentInfo.context.tiledbContext, buffer.tiledbBuffer, C.tiledb_serialization_type_t(serializationType), cArrayURI, cClientSide, fragmentInfo.tiledbFragmentInfo) | |||
| runtime.KeepAlive(fragmentInfo) | |||
There was a problem hiding this comment.
This FragmentInfo (and the one in line 581) is passed by value, so this call does not do anything. We might want to change it now or in the future to take a pointer, don't know if the break is tolerable.
teo-tsirpanis
force-pushed
the
keepalive
branch
from
e517043 to
3ef4c9b
Compare
SC-58351
SC-62492
runtime.KeepAliveto keep the finalizers of TileDB objects from being called during each cgo call.runtime.Pinnertype to pin memory held long-term by C, in a well-defined way.Free()function, after the C handle is freed.GOEXPERIMENT=cgocheck2option.