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

[Dave Scott <Dave.Scott@xxxxxxxxxx>]

> On 30 Jan 2015, at 14:11, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>

Dave

> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Dave Scott <dave.scott@xxxxxxxxxxxxx>
> CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel