ygrek / ocurl

OCaml bindings to libcurl
https://ygrek.org/p/ocurl
MIT License
60 stars 32 forks source link

Segfault with `set_closesocketfunction` #58

Open dwwoelfel opened 2 years ago

dwwoelfel commented 2 years ago

I am seeing segfaults when using set_closesocketfunction. I think this happens because the close socket function is called after the memory that holds the function is freed.

I did some tracing and this is a typical sequence of events:

I create a new handle, set it up with a few options to make a request, call Curl.set_closesocketfunction handle ignore, then add it to the multi handle. Curl completes its request, so I call Curl.cleanup and curl-helper.c calls caml_remove_global_root on the connection's ocamlValues.

Then some time later, Curl decides to close the socket and runs the CURLOPT_CLOSESOCKETFUNCTION callback after the handle has been cleaned up. ocurl tries to access the ocamlValues to run the function I set. It's already been freed, so we get a segfault with type EXC_BAD_ACCESS when trying to access ocamlValues.

I only see this with the close socket function. That might be because it is the only callback that will still be called after the handle is cleaned up.

I haven't seen the segfault if I don't call Curl.cleanup, but ocurl might still call it on its own from op_curl_easy_finalize, so I'm hesitant to try to handle this case from user code.

Any ideas for how ocurl could prevent the segfault?

It seems like ocurl would have to store the callback somewhere other than ocamlValues or wait longer to cleanup ocamlValues, but I'm not sure how ocurl would know when it could be cleaned up. One idea would be to allow only one closesocketfunction globally or one per multi handle.

ygrek commented 2 years ago

Thanks for a thorough description.

That might be because it is the only callback that will still be called after the handle is cleaned up.

heh, this explains indeed :)

re keeping OcamlValues for longer I believe we cannot depend on CLOSESOCKETFUNCTION to be called exactly once for every handle because curl may decide to reuse socket (connection to same server with http pipelining enabled)?

One idea would be to allow only one closesocketfunction globally or one per multi handle.

This sounds good to me, it fixes immediate need and if anyone really needs a different closure per handle they can dispatch on ocaml side by fd value. Do you mind to provide the patch?

ygrek commented 2 years ago

(disabled, this code was never released, so we have freedom to redesign as needed)