fix(go/adbc/pkg): cgo handle pattern#4287
Open
zeroshade wants to merge 2 commits intoapache:mainfrom
Open
Conversation
…e access pattern Two bugs fixed in the CGO driver template (and all generated drivers): 1. exportStringOption wrote the null terminator to sink[lenWithTerminator] (= sink[len(val)+1]) instead of sink[len(val)], causing a one-byte buffer overwrite when the caller supplied exactly the minimum buffer size. 2. The Release functions recovered the cgo.Handle from private_data using (*(*cgo.Handle)(ptr)), which reinterprets the C-allocated uintptr_t as a *cgo.Handle. This worked by coincidence (both are uintptr-sized) but was fragile and inconsistent with getFromHandle. Introduce handleFromPtr so all read-back paths go through the same explicit uintptr cast, and replace the misleading "GC will corrupt the handle" comment with an accurate explanation of the CGO pointer rule.
…e ordering cgo.Handle is a uintptr integer, not a Go pointer, so the CGO checker does not object to storing it directly in a void* field. No C allocation is needed; createHandle now packs the handle value into the pointer-sized private_data field directly and handleFromPtr casts it back. This removes the calloc/free pair from every New/Release path and eliminates the risk of a leak if a Release function were to exit early before reaching the C.free call. Also fix the ordering in all four Release functions: private_data is cleared first (so a concurrent caller sees nil and exits early), then h.Value() extracts the Go object while the handle is still valid, then h.Delete() removes it from the global map. The previous code called h.Delete() before h.Value() in some paths, which would panic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Looking into adbc-drivers/mysql#99 (comment) resulted in finding some of these issues, along with apache/arrow-go#793
Summary
Three bugs found and fixed in the CGO driver template (
_tmpl/driver.go.tmpl) and all generated drivers(
flightsql,snowflake,panicdummy).Bug 1 — off-by-one in
exportStringOption(buffer overwrite)exportStringOptionwrote the null terminator tosink[lenWithTerminator](
= sink[len(val)+1]) instead ofsink[len(val)]. When the caller suppliedexactly the minimum buffer size (
len(val)+1), this wrote one byte past theend of the allocated buffer.
Bug 2 — fragile
cgo.Handlerecovery in Release functionsThe four Release functions (
ArrayStreamRelease,DatabaseRelease,ConnectionRelease,StatementRelease) recovered the handle fromprivate_datausing(*(*cgo.Handle)(ptr))— reinterpreting theC-allocated
uintptr_twrapper as a*cgo.Handle. This worked bycoincidence (both are
uintptr-sized) but was inconsistent withgetFromHandleand relied on an undocumented type-size coincidence.Introduces
handleFromPtr(ptr unsafe.Pointer) cgo.Handleas the singlecanonical read-back path, used by both
getFromHandleand all Releasefunctions. The misleading comment claiming the GC would corrupt the handle
is replaced with an accurate explanation of the actual CGO rule being
satisfied.
Bug 3 — unnecessary C allocation for handle storage; wrong
DeleteorderingcreateHandleallocated auintptr_tviaC.callocto hold the handle'snumeric value, then stored a pointer to that allocation in
private_data.This was not necessary:
cgo.Handleistype Handle uintptr— an integer,not a Go heap pointer — so the CGO checker does not object to storing it
directly in a pointer-sized
void*field.The C allocation is eliminated entirely.
createHandlenow stores the handlevalue directly via
unsafe.Pointer(uintptr(hndl)), andhandleFromPtrcastsit back with
cgo.Handle(uintptr(ptr)). This removes acalloc/freepairfrom every New/Release call path and eliminates any possibility of a leak from
an early return between allocation and free.
Additionally, the Release functions were calling
h.Delete()beforeh.Value()in some paths, which would panic —Deleteremoves the entry fromthe handle map, invalidating any subsequent
Valuecall. The correct sequenceis now applied consistently in all four Release functions:
private_data(idempotence guard for double-release)h.Value()— extract the Go object while the handle is still liveh.Delete()— remove from the map