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

[Docs] C producer examples leak memory? #36050

Closed
lord opened this issue Jun 13, 2023 · 1 comment · Fixed by #36051
Closed

[Docs] C producer examples leak memory? #36050

lord opened this issue Jun 13, 2023 · 1 comment · Fixed by #36051
Assignees
Milestone

Comments

@lord
Copy link
Contributor

lord commented Jun 13, 2023

Describe the usage question you have. Please include as many useful details as possible.

Perhaps my limited C knowledge is making me miss something obvious, but I think the producer examples on this page of the documentation leak memory? Child schemas are malloced:

 //
 // Initialize child type #0
 //
 child = schema->children[0] = malloc(sizeof(struct ArrowSchema));

Then in the release implementation, we free the array of child pointers and call release on the children array, but don't actually free the child ArrowSchema allocations:

static void release_malloced_type(struct ArrowSchema* schema) {
   int i;
   for (i = 0; i < schema->n_children; ++i) {
      struct ArrowSchema* child = schema->children[i];
      if (child->release != NULL) {
         child->release(child);
      }
   }
   free(schema->children);
   // Mark released
   schema->release = NULL;
}

Since (as far as I can tell?) the docs don't otherwise explicitly state what the lifetime of these children is, it could be nice to fix this? Reading the C++ ExportArray implementation, it seems like these are in fact freed in the release call via the private_data pointer in at least the C++ implementation.

Component(s)

C, Documentation

@zeroshade
Copy link
Member

The docs specify the lifetime and memory management semantics here: https://arrow.apache.org/docs/format/CDataInterface.html#memory-management

That said, it looks like you're right. There should be a free(child); after child->release(child); Would you be willing to contribute a PR to fix it? 😄

@kou kou changed the title C producer examples leak memory? [Docs] C producer examples leak memory? Jun 13, 2023
kou pushed a commit that referenced this issue Jun 13, 2023
### Rationale for this change
Fix memory leak in C export examples. Fixes #36050.

### What changes are included in this PR?
Change to example in docs

### Are these changes tested?
Docs only change.

### Are there any user-facing changes?
No
* Closes: #36050

Authored-by: lord <lord@users.noreply.github.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 13.0.0 milestone Jun 13, 2023
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.

3 participants