Re: [PATCH 08/28] libxc: ocaml: add simple binding for xentoollog (output only).

[David Scott <dave.scott@eu.citrix.com>]

On 25/03/13 14:45, Rob Hoes wrote:
[snip]
> +/* external _create_logger: (string * string) -> handle = "stub_xtl_create_logger" */
> +CAMLprim value stub_xtl_create_logger(value cbs)
> +{
> +       CAMLparam1(cbs);
> +       struct caml_xtl *xtl = malloc(sizeof(*xtl));
> +       if (xtl == NULL)
> +               caml_raise_out_of_memory();
> +
> +       memset(xtl, 0, sizeof(*xtl));
> +
> +       xtl->vtable.vmessage = &stub_xtl_ocaml_vmessage;
> +       xtl->vtable.progress = &stub_xtl_ocaml_progress;
> +       xtl->vtable.destroy = &xtl_destroy;
> +
> +       xtl->vmessage_cb = dup_String_val(Field(cbs, 0));
> +       xtl->progress_cb = dup_String_val(Field(cbs, 1));
> +       CAMLreturn((value)xtl);
> +}

I think we should avoid returning "bare pointers" to the OCaml heap for 
two reasons:

Firstly it makes us vulnerable to a sequence like the following:

   1. malloc() something out of heap
   2. return the "bare pointer" to the heap
   3. GC runs, ignores the bare pointer because it points out of heap
   ... some time later ...
   4. we call free() on the out of heap thing
   ... some time later ...
   5. GC runs, ignores the bare pointer because it points out of heap
   ... some time and many heap allocations later ...
   6. GC expands
   <-- the heap now includes the old address used by the bare pointer
   7. GC runs, follows the bare pointer because it points inside the 
heap and segfaults

Secondly it prevents some heap optimisations that are being experimented 
with by people at OCamlLabs, so we'd be storing up some incompatibility 
for the future.

Instead of returning a "bare pointer" I think we should use a "Custom" 
value. This involves declaring a "struct custom_operations" like this:

   static struct custom_operations foo_custom_operations = {
      "foo_custom_operations",
      custom_finalize_default,
      custom_compare_default,
      custom_hash_default,
      custom_serialize_default,
      custom_deserialize_default
   };

And then wrapping and unwrapping "Custom" blocks using something like:

   #define Foo_val(x) (*((struct foo *)Data_custom_val(x)))

   static value
   Val_foo (struct foo *x)
   {
     CAMLparam0 ();
     CAMLlocal1 (result);
     result = caml_alloc_custom (&foo_custom_operations,
                                sizeof (struct foo*), 0, 1);
     Foo_val (result) = x;
     CAMLreturn (result);
  }

There's more information here:

http://caml.inria.fr/pub/docs/manual-ocaml-4.00/manual033.html#toc150

The ocaml-libvirt bindings are a good example of this pattern:

http://git.annexia.org/?p=ocaml-libvirt.git;a=summary

It's also worth considering whether to use the finalizer support to 
automatically free the underlying C resource when the last reference to 
it from OCaml has been GCed. This would be safer than exposing a direct 
"free" function in the OCaml interface, since it would prevent 
use-after-free.

Cheers,
Dave