All of lore.kernel.org
 help / color / mirror / Atom feed
From: Zheng Li <dev@zheng.li>
To: xen-devel@lists.xensource.com
Subject: [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC)
Date: Fri, 30 Sep 2011 15:39:59 +0000	[thread overview]
Message-ID: <a980cfb2e4da7ce1780f.1317397199@eta> (raw)
In-Reply-To: <patchbomb.1317397198@eta>

[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]

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@zheng.li>


 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 #2: xen-unstable.hg-1.patch --]
[-- Type: text/x-patch, Size: 3278 bytes --]

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@zheng.li>

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 #3: Type: text/plain, Size: 138 bytes --]

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

  reply	other threads:[~2011-09-30 15:39 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-09-30 15:39 [PATCH 0 of 2] Invalid memory access in OCaml mmap library Zheng Li
2011-09-30 15:39 ` Zheng Li [this message]
2011-09-30 16:34   ` [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC) Ian Campbell
2011-09-30 17:45     ` Zheng Li
2011-10-06 16:47   ` Ian Jackson
2011-09-30 15:40 ` [PATCH 2 of 2] Change GET_C_STRUCT to Intf_val to follow the common naming scheme of OCaml macro Zheng Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a980cfb2e4da7ce1780f.1317397199@eta \
    --to=dev@zheng.li \
    --cc=xen-devel@lists.xensource.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.