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.

Sign up for GitHub

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

Jump to bottom

[Go] cdata package leaks handles, difficulty debugging #33887

Closed
zeroshade opened this issue Jan 26, 2023 · 1 comment · Fixed by #33889
Closed

[Go] cdata package leaks handles, difficulty debugging #33887

zeroshade opened this issue Jan 26, 2023 · 1 comment · Fixed by #33889
Assignees
@zeroshade
Labels
Component: Go Type: bug
Milestone
12.0.0

Comments

@zeroshade
Copy link
Member

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

When the "misuse of cgo.Handle" issue was fixed for #33864, it looks like we forgot to call free on the malloc'd uintptr_t that gets used causing a memory leak which was caught by leak sanitizer when testing ADBC.

Also, we should add static to the definitions of the methods in go/arrow/cdata/arrow/helpers.h so that they don't cause multiple definition errors if you build without optimizations for debugging.

Component(s)

Go
@zeroshade zeroshade self-assigned this Jan 26, 2023
zeroshade added a commit to zeroshade/arrow that referenced this issue Jan 26, 2023
@zeroshade
apacheGH-33887: [Go] cdata package leaks handles, difficult debugging
c81f43f
@github-actions github-actions bot mentioned this issue Jan 26, 2023
Merged
zeroshade added a commit that referenced this issue Jan 26, 2023
@zeroshade
GH-33887: [Go] cdata package leaks handles, difficult debugging (#33889)
8f537ca 
### Rationale for this change
Fixing a memory leak of malloc'd `uintptr_t` values used for cgo handles

### What changes are included in this PR?
calling `free` on the handles after deleting the Go objects they point at, add `static` keyword to the helper function definitions in `helpers.h` so that compiling without optimizations for debugging doesn't fail.

* Closes: #33887

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@zeroshade zeroshade added this to the 12.0.0 milestone Jan 26, 2023
sjperkins pushed a commit to sjperkins/arrow that referenced this issue Feb 10, 2023
@zeroshade @sjperkins
apacheGH-33887: [Go] cdata package leaks handles, difficult debugging (…
20d13e1 
…apache#33889)

### Rationale for this change
Fixing a memory leak of malloc'd `uintptr_t` values used for cgo handles

### What changes are included in this PR?
calling `free` on the handles after deleting the Go objects they point at, add `static` keyword to the helper function definitions in `helpers.h` so that compiling without optimizations for debugging doesn't fail.

* Closes: apache#33887

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
gringasalpastor pushed a commit to gringasalpastor/arrow that referenced this issue Feb 17, 2023
@zeroshade
apacheGH-33887: [Go] cdata package leaks handles, difficult debugging (…
19c6c88 
…apache#33889)

### Rationale for this change
Fixing a memory leak of malloc'd `uintptr_t` values used for cgo handles

### What changes are included in this PR?
calling `free` on the handles after deleting the Go objects they point at, add `static` keyword to the helper function definitions in `helpers.h` so that compiling without optimizations for debugging doesn't fail.

* Closes: apache#33887

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
@pitrou
Copy link
Member

pitrou commented Jun 27, 2023

Also, we should add static to the definitions of the methods in go/arrow/cdata/arrow/helpers.h so that they don't cause multiple definition errors if you build without optimizations for debugging.

Hmm, why would they? They are marked inline so having multiple identical definitions shouldn't be a problem...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zeroshade zeroshade

Labels
Component: Go Type: bug
Projects
None yet
Milestone
12.0.0
Development

Successfully merging a pull request may close this issue.

GH-33887: [Go] cdata package leaks handles, difficult debugging zeroshade/arrow
2 participants
@zeroshade @pitrou