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

caml_output_value_to_malloc: really use malloc #12566

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

gasche
Copy link
Member

@gasche gasche commented Sep 16, 2023

External users (unison, hhvm, pyre-check) assume that the memory they get from caml_output_value_to_malloc can be freed with free. (See #12491 (comment) for some user codebase examples .)

The function was changed to use caml_stat_alloc in 4.06 ( 02a8b99 ); this breaks user code, as one should then use caml_stat_free.

There is no use of caml_output_value_to_malloc in the runtime itself, so we do not need to provide a caml_output_value_to_stat_alloc variant.

@abbysmal
Copy link
Contributor

Change looks good to me: I assume it was changed by mistake in 02a8b99 when grepping for malloc occurrences?

@abbysmal abbysmal self-requested a review September 18, 2023 08:52
@abbysmal
Copy link
Contributor

(Changes entry needs update, will merge afterward.)

External users (unison, hhvm, pyre-check) assume that the memory they
get from `caml_output_value_to_malloc` can be freed with `free`.

The function was changed to use caml_stat_alloc in
4.06 ( 02a8b99 ); this breaks user
code, as one should then use `caml_stat_free`.

There is no use of caml_output_value_to_malloc in the runtime itself,
so we do not need to provide a caml_output_value_to_stat_alloc
variant.
@gasche
Copy link
Member Author

gasche commented Sep 18, 2023

Thanks! I rebased the PR.

@xavierleroy
Copy link
Contributor

OK for this change: ownership of the allocated block is transferred to 3rd-party C code, so the automatic deallocation that can occur with caml_stat_alloc + cleanup-at-exit mode is not right.

Symmetrically, there's caml_input_value_from_malloc, which currently uses caml_stat_free to free the buffer passed to it, implying that is must be allocated with caml_stat_alloc. The way it's used in OCamlMPI currently is inconsistent (allocation is done sometimes with caml_stat_alloc, sometimes with malloc).
\

@gasche
Copy link
Member Author

gasche commented Sep 18, 2023

Ah, good catch, I didn't know about that function and will change it as well.

@xavierleroy
Copy link
Contributor

Ah, good catch, I didn't know about that function and will change it as well.

As you'll see it's a bit more difficult...

@gasche
Copy link
Member Author

gasche commented Sep 18, 2023

I pushed a new commit to use malloc/free to handle the intern_input of input.c, which in particular prevents a bug where we would call caml_stat_free on the user-provided input from caml_input_value_from_malloc.

Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! That was easier than I feared...

@gasche gasche merged commit 9e17cbc into ocaml:trunk Sep 18, 2023
9 checks passed
@gasche
Copy link
Member Author

gasche commented Sep 18, 2023

Thanks both, this is now merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants