On 24/11/2022 09:03, Edwin Torok wrote: >> On 23 Nov 2022, at 22:25, Andrew Cooper <[email protected]> wrote: >> >> The binding for xc_interface_close() free the underlying handle while leaving >> the Ocaml object still in scope and usable. This would make it easy to >> suffer >> a use-after-free, if it weren't for the fact that the typical usage is as a >> singleton that lives for the lifetime of the program. >> >> Ocaml 5 no longer permits storing a naked C pointer in an Ocaml value. >> >> Therefore, use a Custom block. This allows us to use the finaliser callback >> to call xc_interface_close(), if the Ocaml object goes out of scope. >> >> Signed-off-by: Andrew Cooper <[email protected]> >> --- >> CC: Christian Lindig <[email protected]> >> CC: David Scott <[email protected]> >> CC: Edwin Torok <[email protected]> >> CC: Rob Hoes <[email protected]> >> >> I've confirmed that Xenctrl.close_handle does cause the finaliser to be >> called, simply by dropping the handle reference. > > Thanks, a good way to test this is with OCAMLRUNPARAM=c, possible under > valgrind, which causes all finalisers to be called on exit > (normally they are not because the program is exiting anyway)
I do that anyway, but it's not relevant here. What matters is checking that calling close_handle releases the object (albeit with a forced GC sweep) before the program ends. >> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c >> b/tools/ocaml/libs/xc/xenctrl_stubs.c >> index f37848ae0bb3..4e1204085422 100644 >> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c >> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c >> @@ -37,13 +37,28 @@ >> >> #include "mmap_stubs.h" >> >> -#define _H(__h) ((xc_interface *)(__h)) >> +#define _H(__h) (*((xc_interface **)Data_custom_val(__h))) >> #define _D(__d) ((uint32_t)Int_val(__d)) > > I think this requires an update in xenopsd too to match, otherwise it'll > crash: > https://github.com/xapi-project/xenopsd/blob/master/c_stubs/xenctrlext_stubs.c#L32 Urgh. I'll take a note to do that when bringing in the change. > This wasn't an issue with the original patch which used Data_abstract_val > here, because > that (currently) happens to boil down to just a cast (with some GC metadata > *before* it), > so the old way of just casting OCaml value to C pointer still worked. > > However Data_custom_val boils down to accessing a value at +sizeof(value) > offset, > so xenopsd would now read the wrong pointer. > Perhaps it would've been better to have this _H defined in some header, > otherwise extending Xenctrl the way xenopsd does it is quite brittle. Exporting _H won't help because everything is statically built. It's brittle because xenopsd has got a local piece of C playing around with the internals of someone else's library. This violates more rules than I care to list. We (XenServer) should definitely work to improve things, but this is entirely a xenopsd problem, not an upstream Xen problem. ~Andrew
