search

rustls / rustls-ffi

Use Rustls from any language
Other
132 stars 30 forks source link

Add feature for early testing of feature(read_buf) #248

Closed divergentdave closed 2 years ago

divergentdave commented 2 years ago

This makes use of rustls/rustls#877. I'm marking it as a draft, particularly because I patched the rustls dependency to use my branch. I'm still undecided on whether the rustls_connection_read API contract should be relaxed to no longer require initialization, or if this should become a new symbol, like rustls_connection_read_2. Note that cbindgen generates the same C API whether the feature is enabled or not, though the Rust function signatures differ, because it always transforms MaybeUninit<T> to T.

jsha commented 2 years ago

Thanks for the patch (and the upstream patch)! For feature enablement, let's add rustls_connection_read_2, and document that it's experimental, available only on nightly with the feature enabled, and will go away some time after read_buf stabilizes in the stdlib.

The advantage of that approach is that we can't wind up with code that depends on the "uninitialized is okay" property linked against a library that doesn't have that property.

When read_buf stabilizes, we'll relax the API requirement for rustls_connection_read to get initialized memory, make rustls_connection_read_2 forward to rustls_connection_read, and deprecate it for eventual removal.

jsha commented 2 years ago

That's strange, the Windows build is failing with:

rustls_ffi.lib(std-f87c887dcbebcf7e.std.06878a67-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __imp_BCryptGenRandom referenced in function _ZN3std6thread5local4fast12Key$LT$T$GT$14try_initialize17h6f0eaa92973cfc73E

Not sure what could cause that. We may be missing some libs from --print native-static-libs on Windows. I never did update the Windows Makefile to follow the native static libs printed by the compiler.

Edit: reported in #246.

divergentdave commented 2 years ago

Okay, I split it off into rustls_connection_read_2. Looks like the Windows build is happy now with the additional static libs. (I assume this was another case of implementation details changing between nightly versions)

jsha commented 2 years ago

This looks good! I'll wait on upstream to merge and an update to point at the upstream-merged version.

jsha commented 2 years ago

Upstream has released a version that includes read_buf support. When you get a chance, would you update this branch to point to 0.20.4 (or pull main after #255 is merged), and take it out of draft? Thanks!