Re: [PATCH] tools/ocaml: Factor out compatiblity handling

[Edwin Torok <edwin.torok@xxxxxxxxx>]

On Wed, Aug 28, 2024 at 2:30 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> ... rather than having each library implement its own subset.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Christian Lindig <christian.lindig@xxxxxxxxxx>
> CC: David Scott <dave@xxxxxxxxxx>
> CC: Edwin Török <edwin.torok@xxxxxxxxx>
> CC: Rob Hoes <Rob.Hoes@xxxxxxxxxx>
> CC: Andrii Sultanov <andrii.sultanov@xxxxxxxxx>
>
> Broken out of a larger series, to help Andrii with his dynlib work.
> ---
>  tools/ocaml/libs/xc/Makefile        |  2 +-
>  tools/ocaml/libs/xc/xenctrl_stubs.c | 13 +++----------
>  tools/ocaml/libs/xen-caml-compat.h  | 23 +++++++++++++++++++++++
>  3 files changed, 27 insertions(+), 11 deletions(-)
>  create mode 100644 tools/ocaml/libs/xen-caml-compat.h
>
> diff --git a/tools/ocaml/libs/xc/Makefile b/tools/ocaml/libs/xc/Makefile
> index 1d9fecb06ef2..cdf4d01dac52 100644
> --- a/tools/ocaml/libs/xc/Makefile
> +++ b/tools/ocaml/libs/xc/Makefile
> @@ -2,7 +2,7 @@ OCAML_TOPLEVEL=$(CURDIR)/../..
>  XEN_ROOT=$(OCAML_TOPLEVEL)/../..
>  include $(OCAML_TOPLEVEL)/common.make
>
> -CFLAGS += -I../mmap $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest)
> +CFLAGS += -I../ -I../mmap $(CFLAGS_libxenctrl) $(CFLAGS_libxenguest)
>  CFLAGS += $(APPEND_CFLAGS)
>  OCAMLINCLUDE += -I ../mmap -I ../eventchn
>
> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c 
> b/tools/ocaml/libs/xc/xenctrl_stubs.c
> index a52908012960..c78191f95abc 100644
> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c
> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
> @@ -25,6 +25,8 @@
>  #include <caml/fail.h>
>  #include <caml/callback.h>
>
> +#include "xen-caml-compat.h"
> +
>  #include <sys/mman.h>
>  #include <stdint.h>
>  #include <string.h>
> @@ -37,14 +39,6 @@
>
>  #include "mmap_stubs.h"
>
> -#ifndef Val_none
> -#define Val_none (Val_int(0))
> -#endif
> -
> -#ifndef Tag_some
> -#define Tag_some 0
> -#endif
> -
>  static inline xc_interface *xch_of_val(value v)
>  {
>         xc_interface *xch = *(xc_interface **)Data_custom_val(v);
> @@ -744,8 +738,7 @@ CAMLprim value stub_xc_evtchn_status(value xch_val, value 
> domid, value port)
>         Store_field(result_status, 0, Val_int(status.vcpu));
>         Store_field(result_status, 1, stat);
>
> -       result = caml_alloc_small(1, Tag_some);
> -       Store_field(result, 0, result_status);
> +       result = caml_alloc_some(result_status);
>
>         CAMLreturn(result);
>  }
> diff --git a/tools/ocaml/libs/xen-caml-compat.h 
> b/tools/ocaml/libs/xen-caml-compat.h
> new file mode 100644
> index 000000000000..b4a0ca4ce90c
> --- /dev/null
> +++ b/tools/ocaml/libs/xen-caml-compat.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: LGPL-2.1-only WITH OCaml-LGPL-linking-exception 
> */
> +#ifndef XEN_CAML_COMPAT_H
> +#define XEN_CAML_COMPAT_H
> +
> +#ifndef Val_none /* Option handling.  Compat for Ocaml < 4.12 */
> +
> +#define Val_none Val_int(0)
> +#define Tag_some 0
> +#define Some_val(v) Field(v, 0)
> +
> +static inline value caml_alloc_some(value v)
> +{
> +    CAMLparam1(v);
> +
> +    value some = caml_alloc_small(1, Tag_some);
> +    Store_field(some, 0, v);

The compiler uses Field() rather than Store_field() here.
I think using Store_field here can potentially read uninitialized
data, because 'caml_alloc_small' gives you uninitialized memory
that you must immediately fill with valid values.
Looking at the implementation Store_field calls caml_modify which will
read the old value to figure out whether it was in minor or major
heap,
and doing that on uninitialized data is unpredictable.

We should follow what the manual says and use Field() when
caml_alloc_small() is used, and use Store_field() when caml_alloc() is
used,
and not attempt to mix them:
See https://ocaml.org/manual/5.2/intfc.html#ss:c-low-level-gc-harmony

> +
> +    CAMLreturn(some);
> +}
> +
> +#endif /* !Val_none */
> +
> +#endif /* XEN_CAML_COMPAT_H */
>
> base-commit: 75c64db3722f0245137a1e8cfd3435f4790d0fd7
> --
> 2.39.2
>