[PATCH] ocaml/xenctrl: Fix stub_xc_readconsolering()

[PATCH] ocaml/xenctrl: Fix stub_xc_readconsolering()

[Andrew Cooper]

The Ocaml stub to retrieve the hypervisor console ring had a few problems.

 * A single 32k buffer would truncate a large console ring.
 * The buffer was static and not under the protection of the Ocaml GC lock so
   could be clobbered by concurrent accesses.
 * Embedded NUL characters would cause caml_copy_string() (which is strlen()
   based) to truncate the buffer.

The function is rewritten from scratch, using the same algorithm as the python
stubs, but uses the protection of the Ocaml GC lock to maintain a static
running total of the ring size, to avoid redundant realloc()ing in future
calls.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Dave Scott <dave.scott@eu.citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/ocaml/libs/xc/xenctrl_stubs.c |   59 +++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 10 deletions(-)

diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index 92d064f..0340c64 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -529,26 +529,65 @@ CAMLprim value stub_xc_evtchn_reset(value xch, value domid)
 }
 
 
-#define RING_SIZE 32768
-static char ring[RING_SIZE];
-
 CAMLprim value stub_xc_readconsolering(value xch)
 {
-	unsigned int size = RING_SIZE - 1;
-	char *ring_ptr = ring;
-	int retval;
+	/* Safe to use outside of blocking sections because of Ocaml GC lock. */
+	static unsigned int conring_size = 16384 + 1;
+
+	unsigned int count = conring_size, size = count, index = 0;
+	char *str = NULL, *ptr;
+	int ret;
 
 	CAMLparam1(xch);
+	CAMLlocal1(ring);
 
+	str = malloc(size);
+	if (!str)
+		caml_raise_out_of_memory();
+
+	/* Hopefully our conring_size guess is sufficient */
 	caml_enter_blocking_section();
-	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
+	ret = xc_readconsolering(_H(xch), str, &count, 0, 0, &index);
 	caml_leave_blocking_section();
 
-	if (retval)
+	if (ret < 0) {
+		free(str);
 		failwith_xc(_H(xch));
+	}
+
+	while (count == size && ret >= 0) {
+		size += count - 1;
+		if (size < count)
+			break;
+
+		ptr = realloc(str, size);
+		if (!ptr)
+			break;
+
+		str = ptr + count;
+		count = size - count;
+
+		caml_enter_blocking_section();
+		ret = xc_readconsolering(_H(xch), str, &count, 0, 1, &index);
+		caml_leave_blocking_section();
+
+		count += str - ptr;
+		str = ptr;
+	}
+
+	/*
+	 * If we didn't break because of an overflow with size, and we have
+	 * needed to realloc() ourself more space, update our tracking of the
+	 * real console ring size.
+	 */
+	if (size > conring_size)
+		conring_size = size;
+
+	ring = caml_alloc_string(count);
+	memcpy(String_val(ring), str, count);
+	free(str);
 
-	ring[size] = '\0';
-	CAMLreturn(caml_copy_string(ring));
+	CAMLreturn(ring);
 }
 
 CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
-- 
1.7.10.4

Re: [PATCH] ocaml/xenctrl: Fix stub_xc_readconsolering()

[Dave Scott]

> On 30 Jan 2015, at 14:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> The Ocaml stub to retrieve the hypervisor console ring had a few problems.
> 
> * A single 32k buffer would truncate a large console ring.
> * The buffer was static and not under the protection of the Ocaml GC lock so
>   could be clobbered by concurrent accesses.
> * Embedded NUL characters would cause caml_copy_string() (which is strlen()
>   based) to truncate the buffer.
> 
> The function is rewritten from scratch, using the same algorithm as the python
> stubs, but uses the protection of the Ocaml GC lock to maintain a static
> running total of the ring size, to avoid redundant realloc()ing in future
> calls.
> 

As far as the OCaml FFI goes, this looks fine to me. I can’t spot any problems with the more regular C parts either, although someone who is more used to spotting bugs in C code should probably take a look.

Acked-by: David Scott <dave.scott@citrix.com>

Dave

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Dave Scott <dave.scott@eu.citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> tools/ocaml/libs/xc/xenctrl_stubs.c |   59 +++++++++++++++++++++++++++++------
> 1 file changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index 92d064f..0340c64 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -529,26 +529,65 @@ CAMLprim value stub_xc_evtchn_reset(value xch, value domid)
> }
> 
> 
> -#define RING_SIZE 32768
> -static char ring[RING_SIZE];
> -
> CAMLprim value stub_xc_readconsolering(value xch)
> {
> -	unsigned int size = RING_SIZE - 1;
> -	char *ring_ptr = ring;
> -	int retval;
> +	/* Safe to use outside of blocking sections because of Ocaml GC lock. */
> +	static unsigned int conring_size = 16384 + 1;
> +
> +	unsigned int count = conring_size, size = count, index = 0;
> +	char *str = NULL, *ptr;
> +	int ret;
> 
> 	CAMLparam1(xch);
> +	CAMLlocal1(ring);
> 
> +	str = malloc(size);
> +	if (!str)
> +		caml_raise_out_of_memory();
> +
> +	/* Hopefully our conring_size guess is sufficient */
> 	caml_enter_blocking_section();
> -	retval = xc_readconsolering(_H(xch), ring_ptr, &size, 0, 0, NULL);
> +	ret = xc_readconsolering(_H(xch), str, &count, 0, 0, &index);
> 	caml_leave_blocking_section();
> 
> -	if (retval)
> +	if (ret < 0) {
> +		free(str);
> 		failwith_xc(_H(xch));
> +	}
> +
> +	while (count == size && ret >= 0) {
> +		size += count - 1;
> +		if (size < count)
> +			break;
> +
> +		ptr = realloc(str, size);
> +		if (!ptr)
> +			break;
> +
> +		str = ptr + count;
> +		count = size - count;
> +
> +		caml_enter_blocking_section();
> +		ret = xc_readconsolering(_H(xch), str, &count, 0, 1, &index);
> +		caml_leave_blocking_section();
> +
> +		count += str - ptr;
> +		str = ptr;
> +	}
> +
> +	/*
> +	 * If we didn't break because of an overflow with size, and we have
> +	 * needed to realloc() ourself more space, update our tracking of the
> +	 * real console ring size.
> +	 */
> +	if (size > conring_size)
> +		conring_size = size;
> +
> +	ring = caml_alloc_string(count);
> +	memcpy(String_val(ring), str, count);
> +	free(str);
> 
> -	ring[size] = '\0';
> -	CAMLreturn(caml_copy_string(ring));
> +	CAMLreturn(ring);
> }
> 
> CAMLprim value stub_xc_send_debug_keys(value xch, value keys)
> -- 
> 1.7.10.4
> 

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

Re: [PATCH] ocaml/xenctrl: Fix stub_xc_readconsolering()

[Ian Campbell]

On Fri, 2015-01-30 at 14:30 +0000, Dave Scott wrote:
> 
> 
> > On 30 Jan 2015, at 14:11, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > 
> > The Ocaml stub to retrieve the hypervisor console ring had a few problems.
> > 
> > * A single 32k buffer would truncate a large console ring.
> > * The buffer was static and not under the protection of the Ocaml GC lock so
> >   could be clobbered by concurrent accesses.
> > * Embedded NUL characters would cause caml_copy_string() (which is strlen()
> >   based) to truncate the buffer.
> > 
> > The function is rewritten from scratch, using the same algorithm as the python
> > stubs, but uses the protection of the Ocaml GC lock to maintain a static
> > running total of the ring size, to avoid redundant realloc()ing in future
> > calls.
> > 
> 
> As far as the OCaml FFI goes, this looks fine to me. I can’t spot any
> problems with the more regular C parts either, although someone who is
> more used to spotting bugs in C code should probably take a look.

It looks ok to me and the fact it is based on some existing code helps
too.

> Acked-by: David Scott <dave.scott@citrix.com>

likewise + applied.

Ian.






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