1. Patchew
  2. Xen
  3. View series

[PATCH] tools/ocaml: Factor out compatiblity handling

Andrew Cooper posted 1 patch 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240828133033.2378322-1-andrew.cooper3@citrix.com
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
[PATCH] tools/ocaml: Factor out compatiblity handling
Posted by Andrew Cooper 2 months, 3 weeks ago 
... rather than having each library implement its own subset.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Christian Lindig <christian.lindig@citrix.com>
CC: David Scott <dave@recoil.org>
CC: Edwin Török <edwin.torok@cloud.com>
CC: Rob Hoes <Rob.Hoes@citrix.com>
CC: Andrii Sultanov <andrii.sultanov@cloud.com>

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);
+
+    CAMLreturn(some);
+}
+
+#endif /* !Val_none */
+
+#endif /* XEN_CAML_COMPAT_H */

base-commit: 75c64db3722f0245137a1e8cfd3435f4790d0fd7
-- 
2.39.2
Re: [PATCH] tools/ocaml: Factor out compatiblity handling
Posted by Christian Lindig 2 months, 3 weeks ago 

> On 28 Aug 2024, at 14:30, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote:
> 
> ... rather than having each library implement its own subset.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Christian Lindig <christian.lindig@citrix.com>
> CC: David Scott <dave@recoil.org>
> CC: Edwin Török <edwin.torok@cloud.com>
> CC: Rob Hoes <Rob.Hoes@citrix.com>
> CC: Andrii Sultanov <andrii.sultanov@cloud.com>
> 
> 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);
> +
> +    CAMLreturn(some);
> +}
> +
> +#endif /* !Val_none */
> +
> +#endif /* XEN_CAML_COMPAT_H */
> 
> base-commit: 75c64db3722f0245137a1e8cfd3435f4790d0fd7
> -- 
> 2.39.2
> 

Acked-by: Christian Lindig <christian.lindig@cloud.com>
Re: [PATCH] tools/ocaml: Factor out compatiblity handling
Posted by Edwin Torok 2 months, 3 weeks ago 
On Wed, Aug 28, 2024 at 2:30 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> ... rather than having each library implement its own subset.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Christian Lindig <christian.lindig@citrix.com>
> CC: David Scott <dave@recoil.org>
> CC: Edwin Török <edwin.torok@cloud.com>
> CC: Rob Hoes <Rob.Hoes@citrix.com>
> CC: Andrii Sultanov <andrii.sultanov@cloud.com>
>
> 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
>
Re: [PATCH] tools/ocaml: Factor out compatiblity handling
Posted by Andrew Cooper 2 months, 3 weeks ago 
On 28/08/2024 3:15 pm, Edwin Torok wrote:
> On Wed, Aug 28, 2024 at 2:30 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> ... rather than having each library implement its own subset.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Christian Lindig <christian.lindig@citrix.com>
>> CC: David Scott <dave@recoil.org>
>> CC: Edwin Török <edwin.torok@cloud.com>
>> CC: Rob Hoes <Rob.Hoes@citrix.com>
>> CC: Andrii Sultanov <andrii.sultanov@cloud.com>
>>
>> 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

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

~Andrew
Re: [PATCH] tools/ocaml: Factor out compatiblity handling
Posted by Edwin Torok 2 months, 3 weeks ago 
On Wed, Aug 28, 2024 at 3:20 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 28/08/2024 3:15 pm, Edwin Torok wrote:
> > On Wed, Aug 28, 2024 at 2:30 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >> ... rather than having each library implement its own subset.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Christian Lindig <christian.lindig@citrix.com>
> >> CC: David Scott <dave@recoil.org>
> >> CC: Edwin Török <edwin.torok@cloud.com>
> >> CC: Rob Hoes <Rob.Hoes@citrix.com>
> >> CC: Andrii Sultanov <andrii.sultanov@cloud.com>
> >>
> >> 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
Re: [PATCH] tools/ocaml: Factor out compatiblity handling
Posted by Edwin Torok 2 months, 3 weeks ago 
On Wed, Aug 28, 2024 at 3:15 PM Edwin Torok <edwin.torok@cloud.com> wrote:
>
> On Wed, Aug 28, 2024 at 2:30 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> > ... rather than having each library implement its own subset.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > CC: Christian Lindig <christian.lindig@citrix.com>
> > CC: David Scott <dave@recoil.org>
> > CC: Edwin Török <edwin.torok@cloud.com>
> > CC: Rob Hoes <Rob.Hoes@citrix.com>
> > CC: Andrii Sultanov <andrii.sultanov@cloud.com>
> >
> > 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

Which probably means we've got a bunch of other pre-existing bugs like
these that we need to fix,
as otherwise we do quite a lot of operations on uninitialized data...

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.