yallop / ocaml-ctypes

Library for binding to C libraries using pure OCaml
MIT License
363 stars 95 forks source link

Ctypes.bigarray_start corrupts memory for type 'int' on x86-64. #744

Closed edwintorok closed 11 months ago

edwintorok commented 1 year ago

Ctypes.int is the C type int (4 bytes on x86-64). Bigarray.kind_size_in_bytes Bigarray.int returns 8 (implemented as Sys.word_size / 8) on x86-64. This is documented at https://v2.ocaml.org/releases/5.0/api/Bigarray.html#1_Elementkinds (there are only element kinds for OCaml integers, not C integers)

Ctypes.bigarray_start converts the ctypes 'a type to the bigarray 'a type, but they do not represent the same element size!

AFAICT there is no bigarray type that matches the C int type (well you can use int32 for that on x86-64).

According to https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models sizeof(int) can be either 32-bit or 64-bit (although I don't think OCaml currently supports any OSes where sizeof(int) would be 64, they appear to be historic).

I found this when I've seen some 'Invalid write of size 8' in valgrind on an argument declared as ptr int in Ctypes and then written to using bigarray_start and bigarray ops.

I'm not sure what the best solution would be, because it cannot be solved at the type system level, perhaps document that 'int' is not a valid type here, and raise an exception at runtime when attempting to convert from 'ptr int' to 'int bigarray'?

Workaround: use CArray.from_ptr, that works correctly, but bigarray_start, or bigarray_start |> array_of_bigarray doesn't (it uses the wrong size 8, instead of 4).

yallop commented 1 year ago

Thank you for the report. Do you have some code that can be used to reproduce the problem?

edwintorok commented 1 year ago

babug.zip

$ dune exec ./babug.exe
start
f32: 1., 2., 3.
f64: 1., 2., 3.
i8: 1, 2, 3
i16: 1, 2, 3
i32: 1, 2, 3
i64: 1, 2, 3
nativeint: 1, 2, 3
complex32: re: 1.000000, im: 0.000000, re: 2.000000, im: 0.000000, re: 3.000000, im: 0.000000
complex64: re: 1.000000, im: 0.000000, re: 2.000000, im: 0.000000, re: 3.000000, im: 0.000000
int: 8589934593, 3, 0

valgrind:

==68643== Invalid read of size 8
==68643==    at 0x4E1D95: caml_ba_get_N (bigarray.c:601)
==68643==    by 0x4E1E67: caml_ba_get_1 (bigarray.c:615)
==68643==    by 0x50055A: caml_c_call (in /var/home/edwin/portfolio/ngportfolio/_build/default/lib/old0/babug.exe)
==68643==    by 0x4B57B5F: ???
==68643==    by 0x4B5FB17: ???
==68643==  Address 0x4c94d10 is 4 bytes after a block of size 12 alloc'd
==68643==    at 0x484182F: malloc (vg_replace_malloc.c:431)
==68643==    by 0x4DDBA4: mk_int (foo.c:22)
==68643==    by 0x4DD524: libfoo_stubs_7_mk_int (libfoo__c_cout_generated_functions__Function_description__Function.c:83)
==68643==    by 0x50055A: caml_c_call (in /var/home/edwin/portfolio/ngportfolio/_build/default/lib/old0/babug.exe)
==68643==    by 0x4B57B5F: ???
==68643==    by 0x4B5FB07: ???
yallop commented 1 year ago

Thank you for the example. You should use camlint, not int to interact with bigarrays with int elements:

val int   : int typ
(** Value representing the C type ([signed]) [int]. *)

val camlint : int typ
(** Value representing an integer type with the same storage requirements as
    an OCaml [int]. *)

In your example that involves a change to make_int:

-  let make_int = make "mk_int" int
+  let make_int = make "mk_int" camlint

and corresponding changes in the calls to MK:

-MK(int, int)
+MK(intnat, int)
edwintorok commented 1 year ago

Thanks, but I can't change the type used on the C side, that is fixed and determined by the library I'm writing the binding to. Here is a concrete example:

I can use CArray to access these, but I cannot use bigarrays, and what seems wrong is that Ctypes code that would attempt to use bigarrays would compile and run but access memory out of bounds. Would it be possible to detect mismatches between the size declared for ptr element type and the bigarray element type (at runtime, unless you can come up with a way to make this safe at compile time) and raise an error?

yallop commented 1 year ago

Would it be possible to detect mismatches between the size declared for ptr element type and the bigarray element type [...] and raise an error?

Yes, in principle, but I'm not sure it's worth it overall. It'll always be possible to convert between pointer types by other means, so there'll never be a guarantee that the element type of the pointer used to create the bigarray reflects the intended use of the memory it points to.

edwintorok commented 1 year ago

int is quite a common type in C, it'd be nice if it'd be possible to avoid this pitfall somehow (especially that ctypes and bigarray assign different meaning to the type parameter int, so just by reading the code it all seems to make sense).

How about a warning in the documentation of ctypes? (somewhere around here https://ocaml.org/p/ctypes/latest/doc/Ctypes/index.html#array-values) I did read that, several times, while trying to figure out why my binding was crashing. I've also read the documentation of bigarray:

val int : (int, int_elt) kind See Bigarray.char.

val char : (char, int8_unsigned_elt) kind As shown by the types of the values above, Bigarrays of kind float32_elt and float64_elt are accessed using the OCaml type float. Bigarrays of complex kinds complex32_elt, complex64_elt are accessed with the OCaml type Complex.t. Bigarrays of integer kinds are accessed using the smallest OCaml integer type large enough to represent the array elements: int for 8- and 16-bit integer Bigarrays, as well as OCaml-integer Bigarrays; int32 for 32-bit integer Bigarrays; int64 for 64-bit integer Bigarrays; and nativeint for platform-native integer Bigarrays. Finally, Bigarrays of kind int8_unsigned_elt can also be accessed as arrays of characters instead of arrays of small integers, by using the kind value char instead of int8_unsigned.

Nothing there about the size of int, and whether it is the C or OCaml int. It is only mentioned at https://v2.ocaml.org/releases/5.0/api/Bigarray.html#1_Elementkinds (perhaps that is something to improve in the upstream docs)

yallop commented 11 months ago

Closing, since there's no bug here.