Zero callbacks

[billywhizz]

I have been working on JavaScript bindings for rustls-ffi and found these "improvements" useful while doing so. Maybe you would be willing to accept them as a PR? If so, please let me know what needs changing in order for the PR to be accepted.

In JavaScript (on V8) calling back into JS from C++/Rust has quite some overhead (~70 nanoseconds) and adds some complexity to ffi/bindings, so I have added read_tls_from_fd and write_tls_to_fd methods which allows us to bypass the callbacks altogether. I think this will only work on linux/macos, so have marked the new methods and imports as unix only. I'm not expert in Rust so if there is a cleaner way to do this please let me know.

I also added a root_cert_store_builder_load_roots_from_bytes method to load the cert store from an array of bytes rather than being forced to read from an external file. This is useful for me as I want to embed the certs in the executable or download them and load them on the fly without having to read from disk. I imagine others would find this useful too.

This PR also includes 2 small changes to Cargo.toml to add read_buf feature by default and to the Makefile to generate a C++ compatible header file using cbindgen - i cannot compile using C++ compiler with the current src/rustls.h.

[cpu]

Thanks for the PR! Exciting to hear you're working on using this for JS bindings 👍

I'm not sure about the other maintainers but it may take me a few days to get to reviewing this. I just wanted to leave a comment about that up-front.

[cpu]

rustls-ffi / Clippy nightly (optional) (pull_request) Failing after 18s

Merging #382 and rebasing will fix this nightly clippy finding. It can be ignored in this branch :-)

[cpu]

Did you happen to look at using File::from_raw_fd instead of this approach?

I wonder if going that route and using mem::forget to avoid closing the FD on drop would be a better fit.

Admittedly I've been reviewing io safety for the first time today and don't have a ton of confidence in the area. We'll probably want to get multiple reviews on this aspect of the change no matter the approach taken.

[billywhizz]

i'm not really well up on different options here in Rust tbh. Not sure what you mean by avoiding closing the fd. afaik, this method will just wrap the existing fd which is passed from the embedder side on every call. It shouldn't ever be closed on the rust side, unless there is something I am missing?

I just created this PR as a draft for discussion as I am not sure what the best approach would be in Rust but I think it would be nice to find an approach that avoided the callbacks which aren't strictly needed as they are used completely synchronously.

This approach works exactly the same as the callback version for me in JS and I haven't found any issues in my testing so far. I can add some tests in C to this PR when i get some time if you think that would be useful.

[cpu]

Not sure what you mean by avoiding closing the fd

I mean that if we used File::from_raw_fd to create a File from the passed in fd to read/write from instead of using this FDReader abstraction then without care when the File is dropped it would close the fd. I think we wouldn't want that to happen, and so would need to "leak" the File w/ mem::forget instead of letting it drop normally and document that the caller is responsible for closing the fd.

[billywhizz]

i'm happy to take a look at this again and prepare a PR if anyone can advise on how best to proceed? or, if it's not a supported idea, that's fine too. it seems that v8 has methods now to make callbacks from native into JS much faster, but maybe others will find this PR useful regardless?

[cpu]

Speaking only for myself I'm not likely to have the time required to help get this branch into shape. It might help to try and pull out some of the smaller unrelated changes (e.g. the C++ bindgen support, root store init w/o backing file, etc) into their own PRs. Those improvements are likely easier to land. New unsafe code and fd abstractions are a tougher sell.

[billywhizz]

Speaking only for myself I'm not likely to have the time required to help get this branch into shape. It might help to try and pull out some of the smaller unrelated changes (e.g. the C++ bindgen support, root store init w/o backing file, etc) into their own PRs. Those improvements are likely easier to land. New unsafe code and fd abstractions are a tougher sell.

makes sense. i will do a separate PR for the small changes. can leave this open in case anyone does want to take a look, but if you want to go ahead and close it feel free. thanks.

[cpu]

i will do a separate PR for the small changes.

Thanks!

can leave this open in case anyone does want to take a look, but if you want to go ahead and close it feel free.

Since none of the other maintainers have commented to indicate they can support the work I'm going to close it for now.