Add [Foreign.dynamic_funptr]

[tiash]

[Foreign.dynamic_funptr] is a safer alternative for [Foreign.funptr], in
particular this new version requires explicit life cycle management making it
easier to avoid difficult to debug segmentation faults where the ocaml
closure is released before it is used from C.

We discovered one of these in Async_ssl which motivated us to propose this addition.

[yallop]

Thanks for the proposal!
There might be a delay before I can review it in detail, but I think this approach is a good direction to go overall.

[fdopen]

The OCaml type systems only enforces the types on the OCaml side, not the corresponding types at the C side, that are also part of the contract. It's not a problem with the current interface, because you define the function and its callbacks together.

With your new interface there can be a mismatch that is not longer captured by the type system, e.g with:

let foo = foreign "foo"  (int @-> dynamic_funptr (int @-> int @-> returning int) @-> returning void)

I could allocate the corresponding dnymaic_funptr for foo in multiple ways that are incompatible with the definition above:

let dynptr1 = dynamic_funptr_of_fun (int8_t @-> int8_t @-> returning int8_t) (+)
let dynptr2 = dynamic_funptr_of_fun (camlint @-> camlint @-> returning camlint) (+)
let no_int_at_all = Ctypes.view ~read:(fun _ -> 1) ~write:(fun _ -> "aaa") Ctypes.string
let dynptr3 = dynamic_funptr_of_fun (no_int_at_all @-> no_int_at_all @-> returning no_int_at_all) (+)

[tiash]

Tanks for flagging this!

We have a couple of ideas to fix this, I'll follow up once we have this fleshed out a bit more.

[tiash]

I have just pushed what I think should fix this nicely, here's a slightly contrived example of how to use this

module Progress_callback = Foreign.Make_funptr(val (
  Foreign.dynamic_funptr (int @-> int @-> ptr void @-> returning void))

let keygen =
  foreign "RSA_generate_key" (int @-> int @-> Progress_callback.t @-> ptr void @-> returning rsa_key)

let secret_key =
  Progress_callback.with_fun
    (fun a b _ -> printf "progress: a:%d, b:%d\n" a b) 
    (fun progress -> keygen 2048 65537 progress null)

The Foreign.Make_funptr functor is necessary to ensure that the types remain non-generative so they are safe to use in cstubs.

the (val (Foreign.dynamic_funptr ...)) is a helper fill in defaults for the calling convention and to avoid having to repeat the types (the 4.07 compiler infers everything in the example)

[andrewray]

FWIW I have been using this new API to bind to C with maybe 50 callbacks. I basically allow them to leak because it is test code - but otherwise, the project would not have been possible because of the crashes that the GC would (and did) cause.

I am not sure I like the way the new functions integrate with the old ones - some distinction between safe and "raw" would be nice I think.

[yallop]

Thanks again for this PR, and apologies for my long delay in responding. As I said above, I think adding explicit allocation/deallocation for function pointers is a good direction. Looking at the code, I see several other things I like: using roots, catching double frees, and catching memory leaks by attaching a finaliser.

One thing I'm not so convinced about is the proposed interface, so I'm going to suggest an alternative that seems simpler to me. If I'm not mistaken, we can do everything we need with a pair of functions that construct and deallocate static_funptr values:

val make_funptr : ('a -> 'b) static_funptr typ -> ('a -> 'b) -> ('a -> 'b) static_funptr
val release_funptr : ('a -> 'b) static_funptr -> unit

For example, your RSA_generate_key example could be written using this interface as follows:

let progress_callback_t = static_funptr (int @-> int @-> ptr void @-> returning void)
let keygen = foreign "RSA_generate_key"
       (int @-> int @-> progress_callback_t @-> ptr void @-> returning rsa_key)
let secret_key = 
  let progress = make_funptr progress_callback_t
                   (fun a b _ -> printf "progress: a:%d, b:%d\n" a b) in
  let key = keygen 2048 65537 progress null in
  let () = release_funptr progress in
  key

(If there's something that you think can't be expressed with this interface, please do say!)

Then the semantics follow what you have already: the valid pattern usage pattern is

1. p = make_funptr ...
2. use p: pass it to C, write it to memory, convert it to an OCaml function and call it.
3. release_funptr p

Everything else is invalid, including

• using p after the call to release_funptr
• calling release_funptr twice
• not calling release_funptr at all

Regarding the implementation, I was going to suggest extending static_ptr to store the root. But static_ptr already has a place to store associated OCaml objects, namely the object passed as the optional managed argument to Ctypes_ptr.make. I think storing the root there could work well, and avoid the need to change the type.

I hope to merge this change when everything is addressed, so I'm going to mention a couple more things now. First, the change needs some tests --- ideally, tests that exercise the various failure cases (use-after-free, double-free, leak, etc.). Second, the documentation is currently not to my taste: it's too implementation-specific (e.g. talking about segmentation faults), too opinionated and sometimes too vague. I'm happy to make some more concrete suggestions for documentation once we have consensus on the interface.

[tiash]

Thanks for the feedback!

I originally attempted to do something similar to what you're suggesting but this had soundness issues that @fdopen pointed out.

We could get something similar to your proposal by changing the interface to use a phantom type (similar to [Core.Map]), but I think we still need to retain some type witness to ensure that the ctype of the funptr agrees with the argument its being used in.

Something like the following maybe?

type ('ocaml_type, 'witness) safe_funptr
val make_funptr : ('ocaml_type, 'witness) safe_funptr typ -> 'ocaml_type -> ('ocaml_type,'witness) safe_funptr
val release_funptr : ('ocaml_type,'witness) safe_funptr -> unit

module Progress_callback = Foreign.Make_funptr(val (
  Foreign.safe_funptr (int @-> int @-> ptr void @-> returning void))

let keygen =
  foreign "RSA_generate_key" (int @-> int @-> Progress_callback.t @-> ptr void @-> returning rsa_key)

let secret_key = 
  let progress = make_funptr progress_callback_t
                   (fun a b _ -> printf "progress: a:%d, b:%d\n" a b) in
  let key = keygen 2048 65537 progress null in
  let () = release_funptr progress in
  key

I don't think its possible to avoid the use of a functor to get a unique (witness) type. The nesting of first class module and functor while a bit obscure is a trick to get the compiler to infer all the necessary types from the ctype without having to spell out everything.

It is also possible to create something more like to old interface where the FFI closure is dynamically allocated, but with a more explicit free-step. This would allow an interface very similar to your proposal and also address the safety issues when dealing with closures from ocaml, however there remains some non-obvious issues when dealing with funptrs returned from C.

Please let me know which direction you think is a better fit for ctypes?

Re documentation & testing - point taken

[yminsky]

Anyone know what this is stuck on? This is current blocking our next stable release, all it would be lovely to get it unstuck...

[yallop]

Thanks for the prompt, and apologies (again!) for the slow response. I'd also like to have this resolved soon, and will follow up properly later today.

[yallop]

Okay, I've read through the commit history and I see @fdopen's point about type safety and your neat functor-based fix for the problem he points out.

I'm happy to have the extra type safety of the functor approach, but I'd like to keep the interface as simple as possible. Do you think the following interface (which removes Funptr_spec, Make_funptr and funptr_spec and adds a single function static_funptr) is sufficient?

module type Funptr = sig (* what you have already *) end

val static_funptr :  ?abi:Libffi_abi.abi -> ?runtime_lock:bool -> ?thread_registration:bool -> ('a -> 'b) Ctypes.fn ->
   (module Funptr with type fn = 'a -> 'b)

For example, with this interface your RSA_generate_key code might be written as follows:

module Progress_callback = (val static_funptr (int @-> int @-> ptr void @-> returning void))

let keygen =
  foreign "RSA_generate_key" (int @-> int @-> Progress_callback.t @-> ptr void @-> returning rsa_key)

let secret_key =
  Progress_callback.with_fun
    (fun a b _ -> Printf.printf "progress: a:%d, b:%d\n" a b) 
    (fun progress -> keygen 2048 65537 progress null)

[tiash]

Unfortunately Make_funpstr(val funptr_spec ...) is necessary due to type system restrictions when using the applicative functor the cstubs generator requires.
static_funptr introduces a fresh type (not allowed in applicative functors), while Make_funptr(val funptr_spec ...) exposes to the compiler that the type isn't fresh but depends on the ctype signature (and other parameters) so that its allowed by the compiler.

I have no objections to adding static_funptr, but I'm not sure if having two ways to create a funptr type would make this more confusing?

[yallop]

Unfortunately Make_funpstr(val funptr_spec ...) is necessary due to type system restrictions when using the applicative functor the cstubs generator requires.

Could you expand on this a bit? At the moment, I'm interested in exploring the space of solutions, rather than fixing on a particular approach. Perhaps a concrete example would help, if you have one in mind.

[tiash]

Here's the specific use case where static_funptrcauses compiler errors:

module Bindings (F : Cstubs.FOREIGN) = struct
  (* ... *)
  module Progress_callback = Foreign.Make_funptr(
    (val Foreign.funptr_spec Ctypes_static.(int @-> int @-> ptr void @-> returning void)))
  (* ... *)
  let generate_parameters = foreign "DH_generate_parameters"
      Ctypes.(int @-> int @-> Progress_callback.t_opt @-> ptr void @-> returning Dh.t)
  (* ... *)
end

(https://github.com/janestreet/async_ssl/blob/0e8c5561c1b82d0f5cd29092eb1e301a09416f24/bindings/ffi_bindings.ml#L313)

This Bindings functor is then used with the cstubs generator to generate supporting C + Ocaml (https://github.com/janestreet/async_ssl/blob/0e8c5561c1b82d0f5cd29092eb1e301a09416f24/stubgen/ffi_stubgen.ml#L24) and instantiated using the generated types for use in Async_ssl (https://github.com/janestreet/async_ssl/blob/0e8c5561c1b82d0f5cd29092eb1e301a09416f24/src/import.ml#L2)

The workarounds I can see are

• Define module Progress_callback outside of the the Bindings functor (this splits up the definition and use unnecessarily)
• Modify the code generator to work using generative functors (breaks existing uses of the cstubs generator)
• Use the proposed Make_funptr((val funptr_spec ...)) boilerplate to help the compiler

[yallop]

Thanks for the example.

It feels that there's something not quite right here, somehow. I don't see how we can have something that simultaneously:

• introduces new types (to avoid the problem @fdopen describes)
• can be used in the body of an applicative functor

Doesn't your proposed interface still have @fdopen's type safety problem? Suppose we have two Funptr_spec modules returned from funptr_spec that share OCaml types but represent different C types:

module Spec1 = (val Foreign.funptr_spec Ctypes_static.(int @-> int @-> ptr void @-> returning void))
module Spec2 = (val Foreign.funptr_spec Ctypes_static.(int8_t @-> int8_t @-> ptr void @-> returning void))

Then we might define a functor which builds another Funptr_spec, selecting the fn member at functor application time according to the value of a mutable flag:

let flag = ref false
module F(S: sig end) = struct include Spec1 let fn = if !flag then Spec1.fn else Spec2.fn end

Now calling F twice builds modules with compatible OCaml types but (if the flag is changed between the calls) representing incompatible C types:

module U = struct end
let () = flag := false
module Progress_callback1 = Foreign.Make_funptr(F(U))
let () = flag := true
module Progress_callback2 = Foreign.Make_funptr(F(U))

and so we can build a function f that is specified to take a value of one of the types but that also accepts a value of the other type:

let f = Foreign.foreign "foo" Ctypes.(Progress_callback1.t @-> returning void)
f (Progress_callback2.of_fun (fun _ _ _ -> ()))

The workarounds I can see are

• Define module Progress_callback outside of the the Bindings functor (this splits up the definition and use unnecessarily)

This seems like a fairly reasonable approach. Is moving the type definition away outside of the functor really such a problem? It doesn't seem to depend on the functor argument, so in some respects moving it outside the functor makes the code structure clearer.

• Modify the code generator to work using generative functors (breaks existing uses of the cstubs generator)

Yes, I'd also like to avoid this. (Perhaps one day OCaml will have generativity polymorphism and we can update the cstubs generator to support generative functors in a backwards-compatible way.)

[tiash]

Thanks for the feedback, I agree that writing a custom Funptr_spec does allow subverting type safety.

Making the changes to split out the type definition has proven to be less messy then I expected so lets go with the simpler API, I have pushed the changes.

I think dynamic_funptr might be a better name for the function to avoid confusion with the existing static_funptr type in ctypes (I did this also in this patch).

[yallop]

It's good to see that we've reached consensus on the interface. I'll aim to provide a proper code review in the next few days.

[tiash]

@yallop ping

[yallop]

Please also add some tests, including

1. using Funptr.t values with the dynamic (libffi) interface
2. using Funptr.t values with the static (cstubs) interface
3. using Funptr.t values in a struct or array
4. lifetimes that extend beyond a single call, e.g.:
   (a) allocate a function pointer that toggles a bool ref in OCaml
   (b) pass the function pointer to a C function, which stores it and returns
   (c) call Gc.compact (perhaps twice)
   (d) check from OCaml that the OCaml closure is still alive (e.g. using another flag set in the finalizer)
   (e) call a second C function which invokes the stored function pointer
   (f) check that the call took place by examining the bool ref
5. auxiliary function such as with_fun, t_opt, etc.

[tiash]

Thanks for all the feedback!
I've updated the API and documentation as suggested.
I'm still working on the tests

[tiash]

I've added unit tests that should give basic coverage of everything.

[yallop]

Thanks, @tiash. I had a few final tweaks related to style/layout and compatibility with OCaml 4.02. I've added them as a pull request against this PR: tiash#1.

[yallop]

I've pushed one more commit (760f638) to prevent flambda from turning the closure (+)1 into a top-level function.

The 32-bit Windows build is still failing, but that seems to be unrelated to this PR, so I've opened a separate issue in the mingw opam-repository.

[yallop]

Thanks again for all your work on this very useful addition, @tiash. (And thanks, too, to @fdopen, @andrewray and @avsm for comments and review.)

This is now merged in master (285f119, a41dd9d), and included in the 0.16.0 release, which should be available via OPAM shortly (ocaml/opam-repository#15473).

[tiash]

Many thanks @yallop for taking the time to review this and for all the feedback you provided to improve the pull request!