* [PATCH 0 of 2] Invalid memory access in OCaml mmap library
@ 2011-09-30 15:39 Zheng Li
2011-09-30 15:39 ` [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC) Zheng Li
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
0 siblings, 2 replies; 6+ messages in thread
From: Zheng Li @ 2011-09-30 15:39 UTC (permalink / raw)
To: xen-devel
Thanks Roberto Di Cosmo for reporting this issue.
tools/ocaml/libs/mmap/mmap_stubs.c | 24 +++++++++---------------
tools/ocaml/libs/mmap/mmap_stubs.c | 34 +++++++++++++++++-----------------
2 files changed, 26 insertions(+), 32 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC)
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
2011-09-30 16:34 ` Ian Campbell
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
1 sibling, 2 replies; 6+ messages in thread
From: Zheng Li @ 2011-09-30 15:39 UTC (permalink / raw)
To: xen-devel
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2 of 2] Change GET_C_STRUCT to Intf_val to follow the common naming scheme of OCaml macro
2011-09-30 15:39 [PATCH 0 of 2] Invalid memory access in OCaml mmap library Zheng Li
2011-09-30 15:39 ` [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC) Zheng Li
@ 2011-09-30 15:40 ` Zheng Li
1 sibling, 0 replies; 6+ messages in thread
From: Zheng Li @ 2011-09-30 15:40 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 3181 bytes --]
... and for better readabilities.
Signed-off-by: Zheng Li <dev@zheng.li>
tools/ocaml/libs/mmap/mmap_stubs.c | 34 +++++++++++++++++-----------------
1 files changed, 17 insertions(+), 17 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
@@ -28,7 +28,7 @@
#include <caml/fail.h>
#include <caml/callback.h>
-#define GET_C_STRUCT(a) ((struct mmap_interface *) a)
+#define Intf_val(a) ((struct mmap_interface *) a)
static int mmap_interface_init(struct mmap_interface *intf,
int fd, int pflag, int mflag,
@@ -61,27 +61,27 @@ CAMLprim value stub_mmap_init(value fd,
result = caml_alloc(sizeof(struct mmap_interface), Abstract_tag);
- if (mmap_interface_init(GET_C_STRUCT(result), Int_val(fd),
+ if (mmap_interface_init(Intf_val(result), Int_val(fd),
c_pflag, c_mflag,
Int_val(len), Int_val(offset)))
caml_failwith("mmap");
CAMLreturn(result);
}
-CAMLprim value stub_mmap_final(value interface)
+CAMLprim value stub_mmap_final(value intf)
{
- CAMLparam1(interface);
+ CAMLparam1(intf);
- 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;
+ if (Intf_val(intf)->addr != MAP_FAILED)
+ munmap(Intf_val(intf)->addr, Intf_val(intf)->len);
+ Intf_val(intf)->addr = MAP_FAILED;
CAMLreturn(Val_unit);
}
-CAMLprim value stub_mmap_read(value interface, value start, value len)
+CAMLprim value stub_mmap_read(value intf, value start, value len)
{
- CAMLparam3(interface, start, len);
+ CAMLparam3(intf, start, len);
CAMLlocal1(data);
int c_start;
int c_len;
@@ -89,33 +89,33 @@ CAMLprim value stub_mmap_read(value inte
c_start = Int_val(start);
c_len = Int_val(len);
- if (c_start > GET_C_STRUCT(interface)->len)
+ if (c_start > Intf_val(intf)->len)
caml_invalid_argument("start invalid");
- if (c_start + c_len > GET_C_STRUCT(interface)->len)
+ if (c_start + c_len > Intf_val(intf)->len)
caml_invalid_argument("len invalid");
data = caml_alloc_string(c_len);
- memcpy((char *) data, GET_C_STRUCT(interface)->addr + c_start, c_len);
+ memcpy((char *) data, Intf_val(intf)->addr + c_start, c_len);
CAMLreturn(data);
}
-CAMLprim value stub_mmap_write(value interface, value data,
+CAMLprim value stub_mmap_write(value intf, value data,
value start, value len)
{
- CAMLparam4(interface, data, start, len);
+ CAMLparam4(intf, data, start, len);
int c_start;
int c_len;
c_start = Int_val(start);
c_len = Int_val(len);
- if (c_start > GET_C_STRUCT(interface)->len)
+ if (c_start > Intf_val(intf)->len)
caml_invalid_argument("start invalid");
- if (c_start + c_len > GET_C_STRUCT(interface)->len)
+ if (c_start + c_len > Intf_val(intf)->len)
caml_invalid_argument("len invalid");
- memcpy(GET_C_STRUCT(interface)->addr + c_start, (char *) data, c_len);
+ memcpy(Intf_val(intf)->addr + c_start, (char *) data, c_len);
CAMLreturn(Val_unit);
}
[-- Attachment #2: xen-unstable.hg-2.patch --]
[-- Type: text/x-patch, Size: 3044 bytes --]
... and for better readabilities.
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
@@ -28,7 +28,7 @@
#include <caml/fail.h>
#include <caml/callback.h>
-#define GET_C_STRUCT(a) ((struct mmap_interface *) a)
+#define Intf_val(a) ((struct mmap_interface *) a)
static int mmap_interface_init(struct mmap_interface *intf,
int fd, int pflag, int mflag,
@@ -61,27 +61,27 @@ CAMLprim value stub_mmap_init(value fd,
result = caml_alloc(sizeof(struct mmap_interface), Abstract_tag);
- if (mmap_interface_init(GET_C_STRUCT(result), Int_val(fd),
+ if (mmap_interface_init(Intf_val(result), Int_val(fd),
c_pflag, c_mflag,
Int_val(len), Int_val(offset)))
caml_failwith("mmap");
CAMLreturn(result);
}
-CAMLprim value stub_mmap_final(value interface)
+CAMLprim value stub_mmap_final(value intf)
{
- CAMLparam1(interface);
+ CAMLparam1(intf);
- 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;
+ if (Intf_val(intf)->addr != MAP_FAILED)
+ munmap(Intf_val(intf)->addr, Intf_val(intf)->len);
+ Intf_val(intf)->addr = MAP_FAILED;
CAMLreturn(Val_unit);
}
-CAMLprim value stub_mmap_read(value interface, value start, value len)
+CAMLprim value stub_mmap_read(value intf, value start, value len)
{
- CAMLparam3(interface, start, len);
+ CAMLparam3(intf, start, len);
CAMLlocal1(data);
int c_start;
int c_len;
@@ -89,33 +89,33 @@ CAMLprim value stub_mmap_read(value inte
c_start = Int_val(start);
c_len = Int_val(len);
- if (c_start > GET_C_STRUCT(interface)->len)
+ if (c_start > Intf_val(intf)->len)
caml_invalid_argument("start invalid");
- if (c_start + c_len > GET_C_STRUCT(interface)->len)
+ if (c_start + c_len > Intf_val(intf)->len)
caml_invalid_argument("len invalid");
data = caml_alloc_string(c_len);
- memcpy((char *) data, GET_C_STRUCT(interface)->addr + c_start, c_len);
+ memcpy((char *) data, Intf_val(intf)->addr + c_start, c_len);
CAMLreturn(data);
}
-CAMLprim value stub_mmap_write(value interface, value data,
+CAMLprim value stub_mmap_write(value intf, value data,
value start, value len)
{
- CAMLparam4(interface, data, start, len);
+ CAMLparam4(intf, data, start, len);
int c_start;
int c_len;
c_start = Int_val(start);
c_len = Int_val(len);
- if (c_start > GET_C_STRUCT(interface)->len)
+ if (c_start > Intf_val(intf)->len)
caml_invalid_argument("start invalid");
- if (c_start + c_len > GET_C_STRUCT(interface)->len)
+ if (c_start + c_len > Intf_val(intf)->len)
caml_invalid_argument("len invalid");
- memcpy(GET_C_STRUCT(interface)->addr + c_start, (char *) data, c_len);
+ memcpy(Intf_val(intf)->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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC)
2011-09-30 15:39 ` [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC) Zheng Li
@ 2011-09-30 16:34 ` Ian Campbell
2011-09-30 17:45 ` Zheng Li
2011-10-06 16:47 ` Ian Jackson
1 sibling, 1 reply; 6+ messages in thread
From: Ian Campbell @ 2011-09-30 16:34 UTC (permalink / raw)
To: Zheng Li; +Cc: xen-devel
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@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);
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC)
2011-09-30 16:34 ` Ian Campbell
@ 2011-09-30 17:45 ` Zheng Li
0 siblings, 0 replies; 6+ messages in thread
From: Zheng Li @ 2011-09-30 17:45 UTC (permalink / raw)
To: xen-devel; +Cc: Ian Campbell
On 30/09/2011 17:34, Ian Campbell wrote:
> 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.
I think the issue itself is not related to multi-thread though. In the original version of the stub_mmap_read,
intf = GET_C_STRUCT(interface);
"interface" was passed to the C function as an OCaml value registered in the OCaml runtime, underline it was just a pointer pointing to a block in the OCaml heap. It was copied and casted to the "intf" on the C side, hence "intf" was initially pointing to the same address as "interface". Unfortunately, before we made any use of "intf", we first called
data = caml_alloc_string(c_len);
which called back to the OCaml runtime and allocated memory in the OCaml heap. Since caml_alloc_string was a function defined by OCaml runtime, it deliberately called quite a few GC routines under the hood which can possibly move any values in the OCaml heap. OCaml runtime would certainly update the "interface" pointer accordingly since it was registered with the runtime. But "intf" was a hard copy on the C side that OCaml had no idea about. So "intf" still pointed to the old address which was no longer valid. Despite the back and forth between C and OCaml territories, all was single threaded here.
> 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?
OCaml runtime has a global lock that will only allow one thread to execute it at a time and the GC is single threaded as well. This ensures the safety but unfortunately introduces the bottleneck. On the other hand, for threads that don't need to access memory on the OCaml side for a while (e.g. long running pure C routines invoked from OCaml side), they can run in parallel by using caml_{enter|leaving}_blocking_section primitives to give up and then retain the rights to enter OCaml runtime.
Cheers
--
Zheng
>> 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);
>> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC)
2011-09-30 15:39 ` [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC) Zheng Li
2011-09-30 16:34 ` Ian Campbell
@ 2011-10-06 16:47 ` Ian Jackson
1 sibling, 0 replies; 6+ messages in thread
From: Ian Jackson @ 2011-10-06 16:47 UTC (permalink / raw)
To: Zheng Li; +Cc: xen-devel
Zheng Li writes ("[Xen-devel] [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC)"):
> [...]
Thanks, this looks plausible to me. I have applied both your patches.
Ian.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-10-06 16:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-30 15:39 [PATCH 0 of 2] Invalid memory access in OCaml mmap library Zheng Li
2011-09-30 15:39 ` [PATCH 1 of 2] Fix invalid memory access in OCaml mmap library (to play nicely with the GC) Zheng Li
2011-09-30 16:34 ` 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
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.