Re: [Xen-devel] [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC)

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2011-09-30 at 16:39 +0100, Zheng Li wrote:
> This was a bug reported by Roberto Di Cosmo. When he tried to reuse
> the mmap library for his own project, Mmap.read occasionally got
> different result when reading from the same map. This turned out to be
> a bug in the binding, where a C pointer was created pointing to a
> OCaml value, and the OCaml value was subsequently moved around by the
> GC after memory allocation and hence invalidated the C pointer. This
> patch removes the indirection of C pointer and uses OCaml macro to
> access values directly.

I was initially quite confused about how the value of a parameter to a C
function could change while that function was running but I see now that
the CAMLparam* stuff apparently enables that behaviour by stashing the
address of the parameters so if you ever call back into the runtime (and
presumably asynchronously if you are multithreaded) the GC can indeed
move stuff around.

The proposed patch only appears to fix the issue if there is some higher
level locking which prevents another thread from triggering a gc while
this function is running. IIRC there is a single global lock which
covers all C code called from ocaml, is that right?

Ian.

> Only Mmap.read function had this problem. The other functions, despite
> having the same code style, didn't have memory allocation involved
> hence wouldn't intrigue such an error. I've changed all of them to the
> safer style for future proof. Directly casting OCaml value's *data
> block* (rather than the value itself) as a C pointer is not a common
> practice either, but I'll leave it as it is.
> 
> The bug hadn't occured on XenServer because XenServer didn't make use
> of the Mmap.read function (except in one place for debugging). In
> XenServer, most mmap operations were going through another pair of
> separately implemented functions (Xs_ring.read/write).
> 
> 
> Signed-off-by: Zheng Li <dev@xxxxxxxx>
> 
> 
>  tools/ocaml/libs/mmap/mmap_stubs.c |  24 +++++++++---------------
>  1 files changed, 9 insertions(+), 15 deletions(-)
> 
> 
> ----
> diff --git a/tools/ocaml/libs/mmap/mmap_stubs.c 
> b/tools/ocaml/libs/mmap/mmap_stubs.c
> --- a/tools/ocaml/libs/mmap/mmap_stubs.c
> +++ b/tools/ocaml/libs/mmap/mmap_stubs.c
> @@ -71,12 +71,10 @@ CAMLprim value stub_mmap_init(value fd, 
>  CAMLprim value stub_mmap_final(value interface)
>  {
>       CAMLparam1(interface);
> -     struct mmap_interface *intf;
>  
> -     intf = GET_C_STRUCT(interface);
> -     if (intf->addr != MAP_FAILED)
> -             munmap(intf->addr, intf->len);
> -     intf->addr = MAP_FAILED;
> +     if (GET_C_STRUCT(interface)->addr != MAP_FAILED)
> +             munmap(GET_C_STRUCT(interface)->addr, 
> GET_C_STRUCT(interface)->len);
> +     GET_C_STRUCT(interface)->addr = MAP_FAILED;
>  
>       CAMLreturn(Val_unit);
>  }
> @@ -85,21 +83,19 @@ CAMLprim value stub_mmap_read(value inte
>  {
>       CAMLparam3(interface, start, len);
>       CAMLlocal1(data);
> -     struct mmap_interface *intf;
>       int c_start;
>       int c_len;
>  
>       c_start = Int_val(start);
>       c_len = Int_val(len);
> -     intf = GET_C_STRUCT(interface);
>  
> -     if (c_start > intf->len)
> +     if (c_start > GET_C_STRUCT(interface)->len)
>               caml_invalid_argument("start invalid");
> -     if (c_start + c_len > intf->len)
> +     if (c_start + c_len > GET_C_STRUCT(interface)->len)
>               caml_invalid_argument("len invalid");
>  
>       data = caml_alloc_string(c_len);
> -     memcpy((char *) data, intf->addr + c_start, c_len);
> +     memcpy((char *) data, GET_C_STRUCT(interface)->addr + c_start, c_len);
>  
>       CAMLreturn(data);
>  }
> @@ -108,20 +104,18 @@ CAMLprim value stub_mmap_write(value int
>                                 value start, value len)
>  {
>       CAMLparam4(interface, data, start, len);
> -     struct mmap_interface *intf;
>       int c_start;
>       int c_len;
>  
>       c_start = Int_val(start);
>       c_len = Int_val(len);
> -     intf = GET_C_STRUCT(interface);
>  
> -     if (c_start > intf->len)
> +     if (c_start > GET_C_STRUCT(interface)->len)
>               caml_invalid_argument("start invalid");
> -     if (c_start + c_len > intf->len)
> +     if (c_start + c_len > GET_C_STRUCT(interface)->len)
>               caml_invalid_argument("len invalid");
>  
> -     memcpy(intf->addr + c_start, (char *) data, c_len);
> +     memcpy(GET_C_STRUCT(interface)->addr + c_start, (char *) data, c_len);
>  
>       CAMLreturn(Val_unit);
>  }



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel