From 42264802227c29d9bb3bcea7b3c5f689dc490b82 Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Thu, 30 Apr 2026 13:03:23 -0400 Subject: [PATCH 1/2] fix(go/pkg): fix off-by-one in exportStringOption and unify cgo.Handle 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. --- go/adbc/pkg/_tmpl/driver.go.tmpl | 30 ++++++++++++++++-------------- go/adbc/pkg/flightsql/driver.go | 30 ++++++++++++++++-------------- go/adbc/pkg/panicdummy/driver.go | 30 ++++++++++++++++-------------- go/adbc/pkg/snowflake/driver.go | 30 ++++++++++++++++-------------- 4 files changed, 64 insertions(+), 56 deletions(-) diff --git a/go/adbc/pkg/_tmpl/driver.go.tmpl b/go/adbc/pkg/_tmpl/driver.go.tmpl index d68170f2db..2d62215d0f 100644 --- a/go/adbc/pkg/_tmpl/driver.go.tmpl +++ b/go/adbc/pkg/_tmpl/driver.go.tmpl @@ -230,22 +230,24 @@ func printLoggingHelp() { } -// Allocate a new cgo.Handle and store its address in a heap-allocated -// uintptr_t. Experimentally, this was found to be necessary, else -// something (the Go runtime?) would corrupt (garbage-collect?) the -// handle. +// Store the cgo.Handle's numeric value in C-allocated memory so that +// private_data (a C pointer field) never holds a Go pointer — CGO forbids +// C from retaining a copy of a Go pointer after a call returns. +// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the +// GC does not trace or move it; the handle keeps the pointed-to Go object +// alive via an internal global map until h.Delete() is called. func createHandle(hndl cgo.Handle) unsafe.Pointer { - // uintptr_t* hptr = malloc(sizeof(uintptr_t)); hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - // *hptr = (uintptr)hndl; *hptr = C.uintptr_t(uintptr(hndl)) return unsafe.Pointer(hptr) } +func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { + return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) +} + func getFromHandle[T any](ptr unsafe.Pointer) *T { - // uintptr_t* hptr = (uintptr_t*)ptr; - hptr := (*C.uintptr_t)(ptr) - return cgo.Handle((uintptr)(*hptr)).Value().(*T) + return handleFromPtr(ptr).Value().(*T) } func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusCode { @@ -253,7 +255,7 @@ func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusC if lenWithTerminator <= *length { sink := fromCArr[byte]((*byte)(unsafe.Pointer(out)), int(*length)) copy(sink, val) - sink[lenWithTerminator] = 0 + sink[len(val)] = 0 } *length = lenWithTerminator return C.ADBC_STATUS_OK @@ -417,7 +419,7 @@ func {{.Prefix}}ArrayStreamRelease(stream *C.struct_ArrowArrayStream) { if stream == nil || stream.release != (*[0]byte)(C.{{.Prefix}}ArrayStreamRelease) || stream.private_data == nil { return } - h := (*(*cgo.Handle)(stream.private_data)) + h := handleFromPtr(stream.private_data) cStream := h.Value().(*cArrayStream) cStream.rdr.Release() @@ -614,7 +616,7 @@ func {{.Prefix}}DatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcErr if !checkDBAlloc(db, err, "AdbcDatabaseRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(db.private_data)) + h := handleFromPtr(db.private_data) cdb := h.Value().(*cDatabase) if cdb.db != nil { @@ -1029,7 +1031,7 @@ func {{.Prefix}}ConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_A if !checkConnAlloc(cnxn, err, "AdbcConnectionRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(cnxn.private_data)) + h := handleFromPtr(cnxn.private_data) conn := h.Value().(*cConn) defer func() { @@ -1479,7 +1481,7 @@ func {{.Prefix}}StatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_Adb if !checkStmtAlloc(stmt, err, "AdbcStatementRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(stmt.private_data)) + h := handleFromPtr(stmt.private_data) st := h.Value().(*cStmt) defer func() { diff --git a/go/adbc/pkg/flightsql/driver.go b/go/adbc/pkg/flightsql/driver.go index defe870ffa..e3420e7363 100644 --- a/go/adbc/pkg/flightsql/driver.go +++ b/go/adbc/pkg/flightsql/driver.go @@ -233,22 +233,24 @@ func printLoggingHelp() { fmt.Fprintf(os.Stderr, "FlightSQL: to enable logging, set %s to 'debug', 'info', 'warn', or 'error'", logLevelEnvVar) } -// Allocate a new cgo.Handle and store its address in a heap-allocated -// uintptr_t. Experimentally, this was found to be necessary, else -// something (the Go runtime?) would corrupt (garbage-collect?) the -// handle. +// Store the cgo.Handle's numeric value in C-allocated memory so that +// private_data (a C pointer field) never holds a Go pointer — CGO forbids +// C from retaining a copy of a Go pointer after a call returns. +// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the +// GC does not trace or move it; the handle keeps the pointed-to Go object +// alive via an internal global map until h.Delete() is called. func createHandle(hndl cgo.Handle) unsafe.Pointer { - // uintptr_t* hptr = malloc(sizeof(uintptr_t)); hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - // *hptr = (uintptr)hndl; *hptr = C.uintptr_t(uintptr(hndl)) return unsafe.Pointer(hptr) } +func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { + return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) +} + func getFromHandle[T any](ptr unsafe.Pointer) *T { - // uintptr_t* hptr = (uintptr_t*)ptr; - hptr := (*C.uintptr_t)(ptr) - return cgo.Handle((uintptr)(*hptr)).Value().(*T) + return handleFromPtr(ptr).Value().(*T) } func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusCode { @@ -256,7 +258,7 @@ func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusC if lenWithTerminator <= *length { sink := fromCArr[byte]((*byte)(unsafe.Pointer(out)), int(*length)) copy(sink, val) - sink[lenWithTerminator] = 0 + sink[len(val)] = 0 } *length = lenWithTerminator return C.ADBC_STATUS_OK @@ -420,7 +422,7 @@ func FlightSQLArrayStreamRelease(stream *C.struct_ArrowArrayStream) { if stream == nil || stream.release != (*[0]byte)(C.FlightSQLArrayStreamRelease) || stream.private_data == nil { return } - h := (*(*cgo.Handle)(stream.private_data)) + h := handleFromPtr(stream.private_data) cStream := h.Value().(*cArrayStream) cStream.rdr.Release() @@ -617,7 +619,7 @@ func FlightSQLDatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcError if !checkDBAlloc(db, err, "AdbcDatabaseRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(db.private_data)) + h := handleFromPtr(db.private_data) cdb := h.Value().(*cDatabase) if cdb.db != nil { @@ -1032,7 +1034,7 @@ func FlightSQLConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_Adb if !checkConnAlloc(cnxn, err, "AdbcConnectionRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(cnxn.private_data)) + h := handleFromPtr(cnxn.private_data) conn := h.Value().(*cConn) defer func() { @@ -1482,7 +1484,7 @@ func FlightSQLStatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_AdbcE if !checkStmtAlloc(stmt, err, "AdbcStatementRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(stmt.private_data)) + h := handleFromPtr(stmt.private_data) st := h.Value().(*cStmt) defer func() { diff --git a/go/adbc/pkg/panicdummy/driver.go b/go/adbc/pkg/panicdummy/driver.go index f7b55eda1e..6126539f2b 100644 --- a/go/adbc/pkg/panicdummy/driver.go +++ b/go/adbc/pkg/panicdummy/driver.go @@ -233,22 +233,24 @@ func printLoggingHelp() { fmt.Fprintf(os.Stderr, "PanicDummy: to enable logging, set %s to 'debug', 'info', 'warn', or 'error'", logLevelEnvVar) } -// Allocate a new cgo.Handle and store its address in a heap-allocated -// uintptr_t. Experimentally, this was found to be necessary, else -// something (the Go runtime?) would corrupt (garbage-collect?) the -// handle. +// Store the cgo.Handle's numeric value in C-allocated memory so that +// private_data (a C pointer field) never holds a Go pointer — CGO forbids +// C from retaining a copy of a Go pointer after a call returns. +// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the +// GC does not trace or move it; the handle keeps the pointed-to Go object +// alive via an internal global map until h.Delete() is called. func createHandle(hndl cgo.Handle) unsafe.Pointer { - // uintptr_t* hptr = malloc(sizeof(uintptr_t)); hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - // *hptr = (uintptr)hndl; *hptr = C.uintptr_t(uintptr(hndl)) return unsafe.Pointer(hptr) } +func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { + return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) +} + func getFromHandle[T any](ptr unsafe.Pointer) *T { - // uintptr_t* hptr = (uintptr_t*)ptr; - hptr := (*C.uintptr_t)(ptr) - return cgo.Handle((uintptr)(*hptr)).Value().(*T) + return handleFromPtr(ptr).Value().(*T) } func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusCode { @@ -256,7 +258,7 @@ func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusC if lenWithTerminator <= *length { sink := fromCArr[byte]((*byte)(unsafe.Pointer(out)), int(*length)) copy(sink, val) - sink[lenWithTerminator] = 0 + sink[len(val)] = 0 } *length = lenWithTerminator return C.ADBC_STATUS_OK @@ -420,7 +422,7 @@ func PanicDummyArrayStreamRelease(stream *C.struct_ArrowArrayStream) { if stream == nil || stream.release != (*[0]byte)(C.PanicDummyArrayStreamRelease) || stream.private_data == nil { return } - h := (*(*cgo.Handle)(stream.private_data)) + h := handleFromPtr(stream.private_data) cStream := h.Value().(*cArrayStream) cStream.rdr.Release() @@ -617,7 +619,7 @@ func PanicDummyDatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcErro if !checkDBAlloc(db, err, "AdbcDatabaseRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(db.private_data)) + h := handleFromPtr(db.private_data) cdb := h.Value().(*cDatabase) if cdb.db != nil { @@ -1032,7 +1034,7 @@ func PanicDummyConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_Ad if !checkConnAlloc(cnxn, err, "AdbcConnectionRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(cnxn.private_data)) + h := handleFromPtr(cnxn.private_data) conn := h.Value().(*cConn) defer func() { @@ -1482,7 +1484,7 @@ func PanicDummyStatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_Adbc if !checkStmtAlloc(stmt, err, "AdbcStatementRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(stmt.private_data)) + h := handleFromPtr(stmt.private_data) st := h.Value().(*cStmt) defer func() { diff --git a/go/adbc/pkg/snowflake/driver.go b/go/adbc/pkg/snowflake/driver.go index 30b9fe04f6..837067beff 100644 --- a/go/adbc/pkg/snowflake/driver.go +++ b/go/adbc/pkg/snowflake/driver.go @@ -233,22 +233,24 @@ func printLoggingHelp() { fmt.Fprintf(os.Stderr, "Snowflake: to enable logging, set %s to 'debug', 'info', 'warn', or 'error'", logLevelEnvVar) } -// Allocate a new cgo.Handle and store its address in a heap-allocated -// uintptr_t. Experimentally, this was found to be necessary, else -// something (the Go runtime?) would corrupt (garbage-collect?) the -// handle. +// Store the cgo.Handle's numeric value in C-allocated memory so that +// private_data (a C pointer field) never holds a Go pointer — CGO forbids +// C from retaining a copy of a Go pointer after a call returns. +// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the +// GC does not trace or move it; the handle keeps the pointed-to Go object +// alive via an internal global map until h.Delete() is called. func createHandle(hndl cgo.Handle) unsafe.Pointer { - // uintptr_t* hptr = malloc(sizeof(uintptr_t)); hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - // *hptr = (uintptr)hndl; *hptr = C.uintptr_t(uintptr(hndl)) return unsafe.Pointer(hptr) } +func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { + return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) +} + func getFromHandle[T any](ptr unsafe.Pointer) *T { - // uintptr_t* hptr = (uintptr_t*)ptr; - hptr := (*C.uintptr_t)(ptr) - return cgo.Handle((uintptr)(*hptr)).Value().(*T) + return handleFromPtr(ptr).Value().(*T) } func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusCode { @@ -256,7 +258,7 @@ func exportStringOption(val string, out *C.char, length *C.size_t) C.AdbcStatusC if lenWithTerminator <= *length { sink := fromCArr[byte]((*byte)(unsafe.Pointer(out)), int(*length)) copy(sink, val) - sink[lenWithTerminator] = 0 + sink[len(val)] = 0 } *length = lenWithTerminator return C.ADBC_STATUS_OK @@ -420,7 +422,7 @@ func SnowflakeArrayStreamRelease(stream *C.struct_ArrowArrayStream) { if stream == nil || stream.release != (*[0]byte)(C.SnowflakeArrayStreamRelease) || stream.private_data == nil { return } - h := (*(*cgo.Handle)(stream.private_data)) + h := handleFromPtr(stream.private_data) cStream := h.Value().(*cArrayStream) cStream.rdr.Release() @@ -617,7 +619,7 @@ func SnowflakeDatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcError if !checkDBAlloc(db, err, "AdbcDatabaseRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(db.private_data)) + h := handleFromPtr(db.private_data) cdb := h.Value().(*cDatabase) if cdb.db != nil { @@ -1032,7 +1034,7 @@ func SnowflakeConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_Adb if !checkConnAlloc(cnxn, err, "AdbcConnectionRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(cnxn.private_data)) + h := handleFromPtr(cnxn.private_data) conn := h.Value().(*cConn) defer func() { @@ -1482,7 +1484,7 @@ func SnowflakeStatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_AdbcE if !checkStmtAlloc(stmt, err, "AdbcStatementRelease") { return C.ADBC_STATUS_INVALID_STATE } - h := (*(*cgo.Handle)(stmt.private_data)) + h := handleFromPtr(stmt.private_data) st := h.Value().(*cStmt) defer func() { From 6812a412e0d88c8a5fe08ba434371bdf34a0d01c Mon Sep 17 00:00:00 2001 From: Matt Topol Date: Thu, 30 Apr 2026 13:11:29 -0400 Subject: [PATCH 2/2] fix(go/pkg): eliminate C wrapper allocation for cgo.Handle; fix Delete 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. --- go/adbc/pkg/_tmpl/driver.go.tmpl | 40 +++++++++++++------------------- go/adbc/pkg/flightsql/driver.go | 40 +++++++++++++------------------- go/adbc/pkg/panicdummy/driver.go | 40 +++++++++++++------------------- go/adbc/pkg/snowflake/driver.go | 40 +++++++++++++------------------- 4 files changed, 64 insertions(+), 96 deletions(-) diff --git a/go/adbc/pkg/_tmpl/driver.go.tmpl b/go/adbc/pkg/_tmpl/driver.go.tmpl index 2d62215d0f..05b2b11a3d 100644 --- a/go/adbc/pkg/_tmpl/driver.go.tmpl +++ b/go/adbc/pkg/_tmpl/driver.go.tmpl @@ -230,20 +230,18 @@ func printLoggingHelp() { } -// Store the cgo.Handle's numeric value in C-allocated memory so that -// private_data (a C pointer field) never holds a Go pointer — CGO forbids -// C from retaining a copy of a Go pointer after a call returns. -// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the -// GC does not trace or move it; the handle keeps the pointed-to Go object -// alive via an internal global map until h.Delete() is called. +// cgo.Handle is a uintptr integer (not a pointer). Packing it directly into +// a void* field is safe: the CGO checker only rejects Go heap pointers, and +// handle values (small non-zero integers from a global counter) never alias +// Go-allocated memory. The GC does not scan C-managed memory, so it will +// never misinterpret the stored integer as a live pointer. No C allocation +// is needed — the handle value itself fits in the pointer-sized field. func createHandle(hndl cgo.Handle) unsafe.Pointer { - hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - *hptr = C.uintptr_t(uintptr(hndl)) - return unsafe.Pointer(hptr) + return unsafe.Pointer(uintptr(hndl)) } func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { - return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) + return cgo.Handle(uintptr(ptr)) } func getFromHandle[T any](ptr unsafe.Pointer) *T { @@ -420,16 +418,15 @@ func {{.Prefix}}ArrayStreamRelease(stream *C.struct_ArrowArrayStream) { return } h := handleFromPtr(stream.private_data) + stream.private_data = nil cStream := h.Value().(*cArrayStream) + h.Delete() cStream.rdr.Release() if cStream.adbcErr != nil { C.{{.Prefix}}errRelease(cStream.adbcErr) C.free(unsafe.Pointer(cStream.adbcErr)) } - C.free(unsafe.Pointer(stream.private_data)) - stream.private_data = nil - h.Delete() runtime.GC() } @@ -617,18 +614,15 @@ func {{.Prefix}}DatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcErr return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(db.private_data) + db.private_data = nil cdb := h.Value().(*cDatabase) + h.Delete() if cdb.db != nil { cdb.db.Close() cdb.db = nil } cdb.opts = nil - if db.private_data != nil { - C.free(unsafe.Pointer(db.private_data)) - db.private_data = nil - } - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1032,14 +1026,13 @@ func {{.Prefix}}ConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_A return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(cnxn.private_data) + cnxn.private_data = nil conn := h.Value().(*cConn) + h.Delete() defer func() { conn.cancelContext() conn.cnxn = nil - C.free(cnxn.private_data) - cnxn.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1482,14 +1475,13 @@ func {{.Prefix}}StatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_Adb return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(stmt.private_data) + stmt.private_data = nil st := h.Value().(*cStmt) + h.Delete() defer func() { st.cancelContext() st.stmt = nil - C.free(stmt.private_data) - stmt.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually diff --git a/go/adbc/pkg/flightsql/driver.go b/go/adbc/pkg/flightsql/driver.go index e3420e7363..3cecf4f79a 100644 --- a/go/adbc/pkg/flightsql/driver.go +++ b/go/adbc/pkg/flightsql/driver.go @@ -233,20 +233,18 @@ func printLoggingHelp() { fmt.Fprintf(os.Stderr, "FlightSQL: to enable logging, set %s to 'debug', 'info', 'warn', or 'error'", logLevelEnvVar) } -// Store the cgo.Handle's numeric value in C-allocated memory so that -// private_data (a C pointer field) never holds a Go pointer — CGO forbids -// C from retaining a copy of a Go pointer after a call returns. -// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the -// GC does not trace or move it; the handle keeps the pointed-to Go object -// alive via an internal global map until h.Delete() is called. +// cgo.Handle is a uintptr integer (not a pointer). Packing it directly into +// a void* field is safe: the CGO checker only rejects Go heap pointers, and +// handle values (small non-zero integers from a global counter) never alias +// Go-allocated memory. The GC does not scan C-managed memory, so it will +// never misinterpret the stored integer as a live pointer. No C allocation +// is needed — the handle value itself fits in the pointer-sized field. func createHandle(hndl cgo.Handle) unsafe.Pointer { - hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - *hptr = C.uintptr_t(uintptr(hndl)) - return unsafe.Pointer(hptr) + return unsafe.Pointer(uintptr(hndl)) } func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { - return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) + return cgo.Handle(uintptr(ptr)) } func getFromHandle[T any](ptr unsafe.Pointer) *T { @@ -423,16 +421,15 @@ func FlightSQLArrayStreamRelease(stream *C.struct_ArrowArrayStream) { return } h := handleFromPtr(stream.private_data) + stream.private_data = nil cStream := h.Value().(*cArrayStream) + h.Delete() cStream.rdr.Release() if cStream.adbcErr != nil { C.FlightSQLerrRelease(cStream.adbcErr) C.free(unsafe.Pointer(cStream.adbcErr)) } - C.free(unsafe.Pointer(stream.private_data)) - stream.private_data = nil - h.Delete() runtime.GC() } @@ -620,18 +617,15 @@ func FlightSQLDatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcError return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(db.private_data) + db.private_data = nil cdb := h.Value().(*cDatabase) + h.Delete() if cdb.db != nil { cdb.db.Close() cdb.db = nil } cdb.opts = nil - if db.private_data != nil { - C.free(unsafe.Pointer(db.private_data)) - db.private_data = nil - } - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1035,14 +1029,13 @@ func FlightSQLConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_Adb return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(cnxn.private_data) + cnxn.private_data = nil conn := h.Value().(*cConn) + h.Delete() defer func() { conn.cancelContext() conn.cnxn = nil - C.free(cnxn.private_data) - cnxn.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1485,14 +1478,13 @@ func FlightSQLStatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_AdbcE return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(stmt.private_data) + stmt.private_data = nil st := h.Value().(*cStmt) + h.Delete() defer func() { st.cancelContext() st.stmt = nil - C.free(stmt.private_data) - stmt.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually diff --git a/go/adbc/pkg/panicdummy/driver.go b/go/adbc/pkg/panicdummy/driver.go index 6126539f2b..07bbc4ac62 100644 --- a/go/adbc/pkg/panicdummy/driver.go +++ b/go/adbc/pkg/panicdummy/driver.go @@ -233,20 +233,18 @@ func printLoggingHelp() { fmt.Fprintf(os.Stderr, "PanicDummy: to enable logging, set %s to 'debug', 'info', 'warn', or 'error'", logLevelEnvVar) } -// Store the cgo.Handle's numeric value in C-allocated memory so that -// private_data (a C pointer field) never holds a Go pointer — CGO forbids -// C from retaining a copy of a Go pointer after a call returns. -// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the -// GC does not trace or move it; the handle keeps the pointed-to Go object -// alive via an internal global map until h.Delete() is called. +// cgo.Handle is a uintptr integer (not a pointer). Packing it directly into +// a void* field is safe: the CGO checker only rejects Go heap pointers, and +// handle values (small non-zero integers from a global counter) never alias +// Go-allocated memory. The GC does not scan C-managed memory, so it will +// never misinterpret the stored integer as a live pointer. No C allocation +// is needed — the handle value itself fits in the pointer-sized field. func createHandle(hndl cgo.Handle) unsafe.Pointer { - hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - *hptr = C.uintptr_t(uintptr(hndl)) - return unsafe.Pointer(hptr) + return unsafe.Pointer(uintptr(hndl)) } func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { - return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) + return cgo.Handle(uintptr(ptr)) } func getFromHandle[T any](ptr unsafe.Pointer) *T { @@ -423,16 +421,15 @@ func PanicDummyArrayStreamRelease(stream *C.struct_ArrowArrayStream) { return } h := handleFromPtr(stream.private_data) + stream.private_data = nil cStream := h.Value().(*cArrayStream) + h.Delete() cStream.rdr.Release() if cStream.adbcErr != nil { C.PanicDummyerrRelease(cStream.adbcErr) C.free(unsafe.Pointer(cStream.adbcErr)) } - C.free(unsafe.Pointer(stream.private_data)) - stream.private_data = nil - h.Delete() runtime.GC() } @@ -620,18 +617,15 @@ func PanicDummyDatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcErro return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(db.private_data) + db.private_data = nil cdb := h.Value().(*cDatabase) + h.Delete() if cdb.db != nil { cdb.db.Close() cdb.db = nil } cdb.opts = nil - if db.private_data != nil { - C.free(unsafe.Pointer(db.private_data)) - db.private_data = nil - } - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1035,14 +1029,13 @@ func PanicDummyConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_Ad return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(cnxn.private_data) + cnxn.private_data = nil conn := h.Value().(*cConn) + h.Delete() defer func() { conn.cancelContext() conn.cnxn = nil - C.free(cnxn.private_data) - cnxn.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1485,14 +1478,13 @@ func PanicDummyStatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_Adbc return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(stmt.private_data) + stmt.private_data = nil st := h.Value().(*cStmt) + h.Delete() defer func() { st.cancelContext() st.stmt = nil - C.free(stmt.private_data) - stmt.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually diff --git a/go/adbc/pkg/snowflake/driver.go b/go/adbc/pkg/snowflake/driver.go index 837067beff..6168587512 100644 --- a/go/adbc/pkg/snowflake/driver.go +++ b/go/adbc/pkg/snowflake/driver.go @@ -233,20 +233,18 @@ func printLoggingHelp() { fmt.Fprintf(os.Stderr, "Snowflake: to enable logging, set %s to 'debug', 'info', 'warn', or 'error'", logLevelEnvVar) } -// Store the cgo.Handle's numeric value in C-allocated memory so that -// private_data (a C pointer field) never holds a Go pointer — CGO forbids -// C from retaining a copy of a Go pointer after a call returns. -// cgo.Handle itself is just a uintptr (an integer), not a pointer, so the -// GC does not trace or move it; the handle keeps the pointed-to Go object -// alive via an internal global map until h.Delete() is called. +// cgo.Handle is a uintptr integer (not a pointer). Packing it directly into +// a void* field is safe: the CGO checker only rejects Go heap pointers, and +// handle values (small non-zero integers from a global counter) never alias +// Go-allocated memory. The GC does not scan C-managed memory, so it will +// never misinterpret the stored integer as a live pointer. No C allocation +// is needed — the handle value itself fits in the pointer-sized field. func createHandle(hndl cgo.Handle) unsafe.Pointer { - hptr := (*C.uintptr_t)(C.calloc(C.sizeof_uintptr_t, C.size_t(1))) - *hptr = C.uintptr_t(uintptr(hndl)) - return unsafe.Pointer(hptr) + return unsafe.Pointer(uintptr(hndl)) } func handleFromPtr(ptr unsafe.Pointer) cgo.Handle { - return cgo.Handle(uintptr(*(*C.uintptr_t)(ptr))) + return cgo.Handle(uintptr(ptr)) } func getFromHandle[T any](ptr unsafe.Pointer) *T { @@ -423,16 +421,15 @@ func SnowflakeArrayStreamRelease(stream *C.struct_ArrowArrayStream) { return } h := handleFromPtr(stream.private_data) + stream.private_data = nil cStream := h.Value().(*cArrayStream) + h.Delete() cStream.rdr.Release() if cStream.adbcErr != nil { C.SnowflakeerrRelease(cStream.adbcErr) C.free(unsafe.Pointer(cStream.adbcErr)) } - C.free(unsafe.Pointer(stream.private_data)) - stream.private_data = nil - h.Delete() runtime.GC() } @@ -620,18 +617,15 @@ func SnowflakeDatabaseRelease(db *C.struct_AdbcDatabase, err *C.struct_AdbcError return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(db.private_data) + db.private_data = nil cdb := h.Value().(*cDatabase) + h.Delete() if cdb.db != nil { cdb.db.Close() cdb.db = nil } cdb.opts = nil - if db.private_data != nil { - C.free(unsafe.Pointer(db.private_data)) - db.private_data = nil - } - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1035,14 +1029,13 @@ func SnowflakeConnectionRelease(cnxn *C.struct_AdbcConnection, err *C.struct_Adb return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(cnxn.private_data) + cnxn.private_data = nil conn := h.Value().(*cConn) + h.Delete() defer func() { conn.cancelContext() conn.cnxn = nil - C.free(cnxn.private_data) - cnxn.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually @@ -1485,14 +1478,13 @@ func SnowflakeStatementRelease(stmt *C.struct_AdbcStatement, err *C.struct_AdbcE return C.ADBC_STATUS_INVALID_STATE } h := handleFromPtr(stmt.private_data) + stmt.private_data = nil st := h.Value().(*cStmt) + h.Delete() defer func() { st.cancelContext() st.stmt = nil - C.free(stmt.private_data) - stmt.private_data = nil - h.Delete() // manually trigger GC for two reasons: // 1. ASAN expects the release callback to be called before // the process ends, but GC is not deterministic. So by manually