Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[OCaml] Fix a possible crash in llvm_struct_name

Authored by jberdine on Mar 25 2021, 5:04 PM.



The implementation of llvm_struct_name before this diff calls
caml_copy_string, which allocates, while the result local variable
points to a block allocated by caml_alloc_small that has not yet
been initialized. If the allocation in caml_copy_string triggers a
garbage collection, then the GC root result contains a pointer to
uninitialized data, which may crash the GC or lead to a memory

This diff fixes this by allocating and initializing the string first
and then allocating and initializing the option, thereby leaving no
dangling pointers when allocations are made.

The conversion from a C string to an OCaml string option is refactored
into a function, cstr_to_string_option. This function is also used
to simplify the definitions of llvm_get_mdstring and

Diff Detail

Event Timeline

jberdine created this revision.Mar 25 2021, 5:04 PM
jberdine requested review of this revision.Mar 25 2021, 5:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2021, 5:04 PM
vaivaswatha added inline comments.Mar 25 2021, 8:06 PM

Given that the order of argument evaluation is unspecified in C, if the second argument Len to cstr_to_string_option is evaluated first (at which point it is uninitialized), then an incorrect value gets passed.


Same as the previous change.


Would it be a good idea to define a macro #define OCAML_OPTION_NONE Val_Int(0) to use in such places? I'm not saying we should do it in this patch (if you think it's a good idea to do it at all), but may be in a later one that changes it at all places uniformly.

jberdine updated this revision to Diff 333517.Mar 26 2021, 3:02 AM

fix bug spotted in CR, formatting


Gah, yes, of course, thanks!


It might help, but I am also slightly concerned that Val_int(0) is the representation of many other OCaml values too, like []. So it would be a leaky abstraction. It seems that choosing to name the macro after None is arbitrary, and potentially confusing.

jberdine marked 2 inline comments as done.Mar 26 2021, 3:04 AM
vaivaswatha accepted this revision.Mar 26 2021, 3:15 AM
This revision is now accepted and ready to land.Mar 26 2021, 3:15 AM
This revision was landed with ongoing or failed builds.Mar 26 2021, 5:02 AM
This revision was automatically updated to reflect the committed changes.