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

[Edwin Torok <edwin.torok@xxxxxxxxx>]

On Wed, Aug 28, 2024 at 3:20 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 28/08/2024 3:15 pm, Edwin Torok wrote:
> > 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
>
> Lovely, this got changed in Ocaml with no information or justification...
>
> https://github.com/ocaml/ocaml/pull/9819
>

Looking at the code more it only dereferences the old value if
Is_young returns false. And Is_young is done as a pointer comparison.
Newly allocated values will live in the young (minor) heap by default
and you're not allowed to have a GC run between caml_alloc_small and
Store_field anyway.
So in practice Store_field is probably OK, but is on very dangerous
ground as it relies on an implementation detail.

> I'll resync this locally, but I shaltn't repost.
>
> ~Andrew