ocaml-opasswd we are using ctypes to bind some functions from
shadow.h. In rare cases, when using the library to update an entry in the shadow file, it seems that the two strings in the passwd struct are read from uninitialised memory instead of being copies of the ocaml strings that we passed to create the new shadow struct.
The code is here: https://github.com/xapi-project/ocaml-opasswd/blob/master/lib/shadow.ml#L20 and my fear is that declaring those as
let sp_name = field shadow_t "sp_name" string
let sp_passwd = field shadow_t "sp_passwd" string
and then reading or writing them with
name = getf !@sp sp_name;
passwd = getf !@sp sp_passwd;
setf sp_t sp_name sp.name;
setf sp_t sp_passwd sp.passwd;
could be wrong. Reading the documentation, it seems that strings view are doing the right copies and allocation there but I don’t see other major points of failure. This code has been there for ages, and if it is wrong I am not sure how it should be fixed.
Is that clearly wrong? Ho should I declare those strings and properly copy them between c and ocaml when using ctypes?
UPDATE: To add some more context, the part of the code that seems to behave badly (sometimes, rarely) is https://github.com/xapi-project/ocaml-opasswd/blob/master/lib/common.ml#L19-L21
I have got a detailed explanation from Jeremy Yallop in the ctypes mailing list: http://lists.ocaml.org/pipermail/ctypes/2018-March/000276.html
When the memory is owned by C code – for example, when a C function
passes a struct to OCaml with fields already initialized – then using
the string view works well: reading a string field creates an OCaml
copy, which persists as long as needed.
However, when things are the other way round – i.e. when you’re
creating or initializing the struct in OCaml before passing it to C –
then the string view isn’t a good choice, because there’s no way to
manage the lifetime of the C copy of the OCaml string that’s written
to the struct field. […]
A reasonable way to do this would be to allocate stable memory for the
string fields before the C call, and ensure that the memory stays
around until the call is complete. Stable memory could mean any of: a
bigarray, a Ctypes.CArray.t value, memory returned by malloc, memory
returned by Ctypes.allocate, etc. In most of these cases ensuring the
memory stays around is a matter of holding onto the handle (bigarray
value, Ctypes ptr, etc.) until after the call.
I would like to suggest to add a new function to
Ctypes.CArray that creates such an array from an OCaml string. The presence of such a function would suggest that OCaml strings cannot be passed directly to C.
You read my mind. I have made this PR just a few minutes ago: https://github.com/ocamllabs/ocaml-ctypes/pull/562
For future reference, note that it is quite tricky to make sure that the lifetime of the arrays pointed by the struct are correct. In the end we had to hide them in a module: https://github.com/xapi-project/ocaml-opasswd/pull/14