Re: [PATCH 11/28] libxl: ocaml: propagate the libxl return error code in exceptions

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

On 25/03/13 14:45, Rob Hoes wrote:
> +static void failwith_xl(int error, char *fname)
> +{
> +       CAMLlocal1(arg);
>          value *exc = caml_named_value("Xenlight.Error");
> +
>          if (!exc)
>                  caml_invalid_argument("Exception Xenlight.Error not initialized, please link xl.cma");
> -       caml_raise_with_string(*exc, fname);
> +
> +       arg = caml_alloc_small(2, 0);
> +
> +       Field(arg, 0) = Val_error(error);
> +       Field(arg, 1) = caml_copy_string(fname);

I think this violates Rule 5 in the OCaml FFI manual[*]. In the 
low-level interface when you allocate a block with "caml_alloc_small" 
all the fields contain random values. The assignment:

   Field(arg, 1) = caml_copy_string(fname);

will first call "caml_copy_string" which performs an allocation before 
setting the field to a valid value. Any function which performs an 
allocation can trigger a GC which will segfault if it sees the random 
data in field 1.

I strongly recommend using the "simple interface" i.e.

   caml_alloc()
   caml_alloc_tuple()
   Store_field()

If you look in the definition of "caml_alloc" [**] it does this:

   CAMLexport value caml_alloc (mlsize_t wosize, tag_t tag)
   {
     value result;
     mlsize_t i;

     Assert (tag < 256);
     Assert (tag != Infix_tag);
     if (wosize == 0){
       result = Atom (tag);
     }else if (wosize <= Max_young_wosize){
       Alloc_small (result, wosize, tag);
       if (tag < No_scan_tag){
         for (i = 0; i < wosize; i++) Field (result, i) = 0;
       }
       ^^^^^ -- it sets the fields to 0 preventing the GC seeing a 
random value

Whereas "caml_alloc_small" just does the "Alloc_small".

> +
> +       caml_raise_with_arg(*exc, arg);
> +}

[*] http://caml.inria.fr/pub/docs/manual-ocaml-4.00/manual033.html
[**] https://github.com/ocaml/ocaml/blob/trunk/byterun/alloc.c