lidavidm opened a new issue, #33900:
URL: https://github.com/apache/arrow/issues/33900

   ### Describe the bug, including details regarding any error messages, 
version, and platform.
   
   [cgo requires that C memory cannot contain persistent pointers to Go 
memory](https://pkg.go.dev/cmd/cgo):
   
   > Go code may not store a Go pointer in C memory. C code may store Go 
pointers in C memory, subject to the rule above: it must stop storing the Go 
pointer when the C function returns. 
   
   But we do exactly this in the C Data Interface: 
   
   
https://github.com/apache/arrow/blob/4f1d255f3dc57457e5c78d98c4b76fc523cf9961/go/arrow/cdata/cdata_exports.go#L392
   
   We _want_ to do this because we do not want to copy data. However, 
technically, we can't. And if we enable the runtime checks for this via 
`GODEBUG=cgocheck=2`, we indeed get a crash:
   
   <details>
   
   ```
   write of Go pointer 0xc0004a8000 to non-Go memory 0x55cdba7ebd18
   fatal error: Go pointer stored into non-Go memory
   
   runtime stack:
   runtime.throw({0x7f6c404fc97f?, 0xc0000044e0?})
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/panic.go:992
 +0x71
   runtime.cgoCheckWriteBarrier.func1()
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/cgocheck.go:55
 +0x8a
   runtime.systemstack()
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/asm_amd64.s:469
 +0x46
   
   goroutine 17 [running, locked to thread]:
   runtime.systemstack_switch()
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/asm_amd64.s:436
 fp=0xc0001eb8c8 sp=0xc0001eb8c0 pc=0x7f6c40676d40
   runtime.cgoCheckWriteBarrier(0x55cdba7ebd18, 0xc0004a8000)
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/cgocheck.go:53
 +0xe8 fp=0xc0001eb8f8 sp=0xc0001eb8c8 pc=0x7f6c4061a7c8
   runtime.wbBufFlush(0xc0001eb950?, 0x7f6c40619433?)
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/mwbbuf.go:190
 +0x2e fp=0xc0001eb918 sp=0xc0001eb8f8 pc=0x7f6c40644aee
   runtime.wbBufFlush(0x55cdba7ebd18, 0xc0004a8000)
        <autogenerated>:1 +0x2c fp=0xc0001eb938 sp=0xc0001eb918 
pc=0x7f6c4067b46c
   runtime.gcWriteBarrier()
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/asm_amd64.s:1669
 +0xa3 fp=0xc0001eb9b8 sp=0xc0001eb938 pc=0x7f6c406790c3
   runtime.gcWriteBarrierR8()
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/asm_amd64.s:1729
 +0x7 fp=0xc0001eb9c0 sp=0xc0001eb9b8 pc=0x7f6c406791a7
   github.com/apache/arrow/go/v11/arrow/cdata.exportArray({0x7f6c40e79bd8?, 
0xc0001d44c0?}, 0x55cdb9f4dd20, 0xc0001d45c0?)
        
/home/lidavidm/go/pkg/mod/github.com/apache/arrow/go/[email protected]/arrow/cdata/cdata_exports.go:392
 +0xb05 fp=0xc0001ebb30 sp=0xc0001eb9c0 pc=0x7f6c40d04365
   github.com/apache/arrow/go/v11/arrow/cdata.exportArray({0x7f6c40e7a188?, 
0xc0001d4480?}, 0x7ffdebc06300, 0xc0000d6aa0?)
        
/home/lidavidm/go/pkg/mod/github.com/apache/arrow/go/[email protected]/arrow/cdata/cdata_exports.go:421
 +0x9d2 fp=0xc0001ebca0 sp=0xc0001ebb30 pc=0x7f6c40d04232
   
github.com/apache/arrow/go/v11/arrow/cdata.ExportArrowRecordBatch({0x7f6c40e79390,
 0xc0003210e0}, 0xc00019c5b0?, 0x0)
        
/home/lidavidm/go/pkg/mod/github.com/apache/arrow/go/[email protected]/arrow/cdata/interface.go:217
 +0x2a5 fp=0xc0001ebd88 sp=0xc0001ebca0 pc=0x7f6c40cf9505
   
github.com/apache/arrow/go/v11/arrow/cdata.cRecordReader.next({{0x7f6c40e76328?,
 0xc0004007e0?}, 0x0?}, 0x7ffdebc06688?)
        
/home/lidavidm/go/pkg/mod/github.com/apache/arrow/go/[email protected]/arrow/cdata/cdata_exports.go:459
 +0x65 fp=0xc0001ebdd0 sp=0xc0001ebd88 pc=0x7f6c40d04565
   github.com/apache/arrow/go/v11/arrow/cdata.streamGetNext(0x0?, 0xc000004601?)
        
/home/lidavidm/go/pkg/mod/github.com/apache/arrow/go/[email protected]/arrow/cdata/exports.go:136
 +0x6d fp=0xc0001ebe10 sp=0xc0001ebdd0 pc=0x7f6c40d0582d
   _cgoexp_c84825904f89_streamGetNext(0x7ffdebc062c0)
        _cgo_gotypes.go:379 +0x28 fp=0xc0001ebe30 sp=0xc0001ebe10 
pc=0x7f6c40d06a08
   runtime.cgocallbackg1(0x7f6c40d069e0, 0xc0001ebfe0?, 0x0)
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/cgocall.go:314
 +0x2c2 fp=0xc0001ebf00 sp=0xc0001ebe30 pc=0x7f6c40619902
   runtime.cgocallbackg(0x0?, 0x0?, 0x0?)
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/cgocall.go:233
 +0x109 fp=0xc0001ebf90 sp=0xc0001ebf00 pc=0x7f6c40619589
   runtime.cgocallbackg(0x7f6c40d069e0, 0x7ffdebc062c0, 0x0)
        <autogenerated>:1 +0x31 fp=0xc0001ebfb8 sp=0xc0001ebf90 
pc=0x7f6c4067b351
   runtime.cgocallback(0x0, 0x0, 0x0)
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/asm_amd64.s:971
 +0xb3 fp=0xc0001ebfe0 sp=0xc0001ebfb8 pc=0x7f6c40678d93
   runtime.goexit()
        
/nix/store/5xayx09nj7rxwkf1c2pxzdslp4ilrv5v-go-1.18.5/share/go/src/runtime/asm_amd64.s:1571
 +0x1 fp=0xc0001ebfe8 sp=0xc0001ebfe0 pc=0x7f6c40678fe1
   ```
   
   </details>
   
   Now, in practice, this is OK, because separately, we keep the Go memory 
alive by storing the `ArrayData` in the C Data Interface `private_data` via a 
[`cgo.Handle`](https://pkg.go.dev/runtime/cgo#Handle). (Well, assuming Go's GC 
is 
[non-moving](https://tip.golang.org/doc/gc-guide#Tracing_Garbage_Collection).) 
But technically, this is wrong.
   
   What are some solutions?
   
   - We can use the `CgoArrowAllocator`, which allocates buffers via 
`libarrow`. Then we're storing pointers to C-allocated memory in C-allocated 
structures. But this gives you a dependency on `libarrow`.
   - We could add a `CgoAllocator` that allocates via `malloc`. This still 
means we have to pay FFI overhead, which is not ideal.
   
   Both of these require you to remember to do the right thing; if you forget 
to use the right allocator, you'll still be in violation of the rules. So on 
top of that, we could add a field to `Buffer` that indicates whether the buffer 
is safe to export. Then the cgo-based allocators can set this flag, and during 
exporting, we can error (or possibly copy) if the buffer is not safe for export.
   
   ### Component(s)
   
   Go


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to