Closed cpu closed 6 months ago
Investigate Windows cmake build failures
Something seems up with the cmake
build tasks. They're both failing to link with unresolved external symbol errors:
rustls_ffi.lib(std-828e2398afa6ec92.std.37d137a6c0aa880e-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __imp_WakeByAddressSingle referenced in function _ZN4core3ptr56drop_in_place$LT$std..thread..Packet$LT$$LP$$RP$$GT$$GT$17h27aa9d331296562bE [D:\a\rustls-ffi\rustls-ffi\build\tests\client.vcxproj]
rustls_ffi.lib(std-828e2398afa6ec92.std.37d137a6c0aa880e-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __imp_WaitOnAddress referenced in function _ZN3std6thread4park17hc58846d9dce664faE [D:\a\rustls-ffi\rustls-ffi\build\tests\client.vcxproj]
I'm not sure why aws-lc-rs is still being built as a dependency of rustls with the no-default-features and feature opt-in we've done in this branch,
Ah! :bulb: It's std
: https://github.com/rustls/rustls/pull/1818
Something seems up with the cmake build tasks.
Unrelated, but on my mind: https://github.com/rustls/rustls-ffi/issues/390 I'm not sure I understand why it makes sense to invest resources into the parallel cmake build infrastructure.
Something seems up with the cmake build tasks. They're both failing to link with unresolved external symbol errors:
Fixing this seems to require the addition of Synchronization.lib
to the Cmake target_link_libraries
invocations (fd92f13). I suspect this is from the addition of once_cell
as a dependency upstream for the process default crypto provider.
I thought the CI static libs test was meant to catch this. I'm also not sure why the base Makefile
approach doesn't require the update :thinking:
I'm not sure why aws-lc-rs is still being built as a dependency of rustls with the no-default-features and feature opt-in we've done in this branch,
Ah! 💡 It's std: https://github.com/rustls/rustls/pull/1818
I updated the git patch to pick up this change and was able to drop the CI changes adding nasm
to the Windows build tasks.
Finish support for Acceptor error alert handling.
This is related to https://github.com/rustls/rustls/pull/1811 and https://github.com/rustls/rustls/issues/1792 - the previous API swallowed alerts generated during accept
or into_connection
. As implemented in this branch, we do the same thing: dropping any alerts on the floor. The upstream Rustls implementation now returns (Error, AcceptedAlert)
in the error condition, and the caller must use AcceptedAlert::write to potentially write TLS alerts to the wire, allowing for better handling here.
I'm not sure the best way to represent this for the FFI boundary. One idea: the rustls_acceptor_accept
and rustls_accepted_into_connection
fn can gain two new out parameters, one *mut u8
for the data, and one *mut size_t
for how much alert data was written. The wrapper would handle the error condition by writing from AcceptedAlert
into the out buffer, and then returning a failure rustls_result
. The caller would be responsible for copying the correct number of bytes from the out buffer to the socket, but only for the error condition. This seems awkward, but so is basically every C API... :laughing:
Any other ideas?
One idea: the rustls_acceptor_accept and rustls_accepted_into_connection fn can gain two new out parameters, one mut u8 for the data, and one mut size_t for how much alert data was written. The wrapper would handle the error condition by writing from AcceptedAlert into the out buffer, and then returning a failure rustls_result. The caller would be responsible for copying the correct number of bytes from the out buffer to the socket, but only for the error condition. This seems awkward, but so is basically every C API... 😆
I think an additional out parameter is the correct direction. But it needs to be an owned type with its own lifetime, so the C code can free it when done. If we ask the caller to provide a buffer as you're proposing, we run into problems like figuring out how big a buffer to allocate[1]. Since the Rust side is going to allocate a ChunkVecBuffer anyhow, we might as well use that.
So this looks like
AcceptedAlert
type as *rustls_accepted_alert
rustls_accepted_alert_free()
[1]: But thinking about it a second time: can we put a small upper bound on the size of the alert bytes? If so, that makes the problem of the output buffer less annoying. We could tell people to allocate a small array on the stack and simply write to that, avoiding complications of malloc'ing and so on.
I think an additional out parameter is the correct direction. But it needs to be an owned type with its own lifetime
Aha, that does making arranging things easier :+1:
Since the Rust side is going to allocate a ChunkVecBuffer anyhow, we might as well use that.
I ended up introducing a new crate-internal type on our side for this (AcceptedAlertBuffer
) . The ChunkVecBuffer
in the upstream AcceptedAlert
isn't accessible. With the current API in 0.23 you have to call write to copy the data out.
So this looks like ...
Other than the caveat about needing our own Vec
I think the plan you proposed and what I've implemented in-branch were pretty much in-sync :+1: Thanks!
[1]: But thinking about it a second time: can we put a small upper bound on the size of the alert bytes? If so, that makes the problem of the output buffer less annoying.
I thought about this a little bit but the AcceptedAlert
's ChunkVecBuffer
is the sendable_tls
buffer from the connection's common data. It isn't scoped to alerts specifically, or guaranteed to be a specific number of alerts. It seemed fragile/dangerous to try and assume a safe static buffer size given that so I went with the other design hashed out in the comment thread.
I think this is ready for review now. (@ctz maybe you would be interested in giving it a once-over as well?)
@jsha Are there any other changes you'd like to see in this branch?
Addresses upstream breaking API changes and updates associated Rustls dependencies to use the newest Rustls release.
Most notable for rustls-ffi are the changes in the
rustls_acceptor_accept
andrustls_accepted_into_connection
functions where a newrustls_accepted_alert
out parameter is added.This type wraps the upstream
AcceptedAlert
type and allows the caller to gather any to-be-written TLS data (most notably, TLS alerts) that was produced while trying to accept the client connection. The caller should handle the to-be-written alert bytes by callingrustls_accepted_alert_write_tls
with therustls_accepted_alert
and arustls_write_callback
implementation to write the returned bytes to the connection before callingrustls_accepted_alert_free
.