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

[Zheng Li <dev@xxxxxxxx>]

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.

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);
 }

Attachment: xen-unstable.hg-1.patch
Description: Text Data

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