Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Go] Possible to confuse Go GC via C Data Interface #36669

Closed
lidavidm opened this issue Jul 13, 2023 · 0 comments · Fixed by #36670
Closed

[Go] Possible to confuse Go GC via C Data Interface #36669

lidavidm opened this issue Jul 13, 2023 · 0 comments · Fixed by #36670
Assignees
Milestone

Comments

@lidavidm
Copy link
Member

Describe the bug, including details regarding any error messages, version, and platform.

If an ArrowSchema etc. has a (garbage) pointer that looks like a Go pointer (due to reuse of stack memory/uninitialized memory), that C Data struct is passed to Go as an out parameter, and the Go code tries to write a new pointer to that value during a GC, the Go GC can get confused and will crash the program, probably because we hit this:

Note: the current implementation has a bug. While Go code is permitted to write nil or a C pointer (but not a Go pointer) to C memory, the current implementation may sometimes cause a runtime error if the contents of the C memory appear to be a Go pointer. Therefore, avoid passing uninitialized C memory to Go code if the Go code is going to store pointer values in it. Zero out the memory in C before passing it to Go. (https://pkg.go.dev/cmd/cgo)

Component(s)

Go

@lidavidm lidavidm self-assigned this Jul 13, 2023
lidavidm added a commit to lidavidm/arrow that referenced this issue Jul 13, 2023
lidavidm added a commit to lidavidm/arrow that referenced this issue Jul 13, 2023
lidavidm added a commit to lidavidm/arrow that referenced this issue Jul 13, 2023
lidavidm added a commit to lidavidm/arrow that referenced this issue Jul 13, 2023
lidavidm added a commit to lidavidm/arrow that referenced this issue Jul 13, 2023
lidavidm added a commit that referenced this issue Jul 13, 2023
### Rationale for this change

Prevent hard to debug crashes when using Go code with other code via C Data Interface.

### What changes are included in this PR?

In the C Stream Interface implementation, jump through a trampoline that zeroes the out parameters before letting Go see them.

Note that this can only guard against the issue when the C Stream Interface is used. 

Also, fix other issues in the C Data Interface tests with invalid pointers and uninitialized memory that were turned up by the new test here (because it calls `runtime.GC` very frequently).

### Are these changes tested?

Yes

### Are there any user-facing changes?

No

**This PR contains a "Critical Fix".**
* Closes: #36669

Lead-authored-by: David Li <[email protected]>
Co-authored-by: Matt Topol <[email protected]>
Signed-off-by: David Li <[email protected]>
@lidavidm lidavidm added this to the 14.0.0 milestone Jul 13, 2023
@raulcd raulcd modified the milestones: 14.0.0, 13.0.0 Jul 18, 2023
raulcd pushed a commit that referenced this issue Jul 18, 2023
### Rationale for this change

Prevent hard to debug crashes when using Go code with other code via C Data Interface.

### What changes are included in this PR?

In the C Stream Interface implementation, jump through a trampoline that zeroes the out parameters before letting Go see them.

Note that this can only guard against the issue when the C Stream Interface is used. 

Also, fix other issues in the C Data Interface tests with invalid pointers and uninitialized memory that were turned up by the new test here (because it calls `runtime.GC` very frequently).

### Are these changes tested?

Yes

### Are there any user-facing changes?

No

**This PR contains a "Critical Fix".**
* Closes: #36669

Lead-authored-by: David Li <[email protected]>
Co-authored-by: Matt Topol <[email protected]>
Signed-off-by: David Li <[email protected]>
chelseajonesr pushed a commit to chelseajonesr/arrow that referenced this issue Jul 20, 2023
…he#36670)

### Rationale for this change

Prevent hard to debug crashes when using Go code with other code via C Data Interface.

### What changes are included in this PR?

In the C Stream Interface implementation, jump through a trampoline that zeroes the out parameters before letting Go see them.

Note that this can only guard against the issue when the C Stream Interface is used. 

Also, fix other issues in the C Data Interface tests with invalid pointers and uninitialized memory that were turned up by the new test here (because it calls `runtime.GC` very frequently).

### Are these changes tested?

Yes

### Are there any user-facing changes?

No

**This PR contains a "Critical Fix".**
* Closes: apache#36669

Lead-authored-by: David Li <[email protected]>
Co-authored-by: Matt Topol <[email protected]>
Signed-off-by: David Li <[email protected]>
R-JunmingChen pushed a commit to R-JunmingChen/arrow that referenced this issue Aug 20, 2023
…he#36670)

### Rationale for this change

Prevent hard to debug crashes when using Go code with other code via C Data Interface.

### What changes are included in this PR?

In the C Stream Interface implementation, jump through a trampoline that zeroes the out parameters before letting Go see them.

Note that this can only guard against the issue when the C Stream Interface is used. 

Also, fix other issues in the C Data Interface tests with invalid pointers and uninitialized memory that were turned up by the new test here (because it calls `runtime.GC` very frequently).

### Are these changes tested?

Yes

### Are there any user-facing changes?

No

**This PR contains a "Critical Fix".**
* Closes: apache#36669

Lead-authored-by: David Li <[email protected]>
Co-authored-by: Matt Topol <[email protected]>
Signed-off-by: David Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants