From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Scott <dave.scott@eu.citrix.com>
Subject: Re: [PATCH 11/28] libxl: ocaml: propagate the libxl
 return error code in exceptions
Date: Tue, 26 Mar 2013 11:33:47 +0000
Message-ID: <5151879B.2070804@eu.citrix.com>
References: <1364222729-6982-1-git-send-email-rob.hoes@citrix.com>
	<1364222729-6982-12-git-send-email-rob.hoes@citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1364222729-6982-12-git-send-email-rob.hoes@citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Rob Hoes <rob.hoes@citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>, "xen-devel@lists.xen.org" <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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