-
Notifications
You must be signed in to change notification settings - Fork 38
Description
We offer three getter methods on connection that return a pointer to borrowed data:
const struct rustls_certificate *rustls_connection_get_peer_certificate(struct rustls_connection *conn,
size_t i);
void rustls_connection_get_alpn_protocol(const struct rustls_connection *conn,
const uint8_t **protocol_out,
size_t *protocol_out_len);
const struct rustls_supported_ciphersuite *rustls_connection_get_negotiated_ciphersuite(const struct rustls_connection *conn);
Each of these has documentation in rustls-ffi that says "the returned pointer lives as long as the program" or similar. However, this is not guaranteed by Rust's lifetime system.
For instance, consider rustls_connection_get_peer_certificate. Inside the rustls-ffi implementation, we take the input *const rustls_connection and cast it into an &Connection. We can then call methods on Connection that take &self, for instance:
pub fn peer_certificates(&self) -> Option<&[Certificate]>
So the lifetime of the returned &[Certificate] is bounded by our &Connection - which drops when we return from rustls_connection_get_peer_certificate. But we're returning a pointer to data in that &[Certificate].
This would be fine if C only had a const rustls_connection *. But in practice, C has a rustls_connection *, and does in fact interleave mutating calls like _read_tls with calls that create & references. As a concrete example: if rustls ever implemented renegotiation (I know it's on the "will never" list), C code could make a call to rustls_connection_get_peer_certificates and store the returned pointer, then _read_tls / _process_new_packets some data that reallocates the internal Vec<Certificates>, which would cause C's stored pointer to be invalid.
I believe as a practical matter the data pointed to by these methods is set only once per connection, so we don't run an actual risk of it changing out from underneath us. But I'd like to figure out if we can use the type system somehow to ensure that. If we can't do that, we should ask rustls to commit to the set-once-per-connection property via documentation (and possibly code to the extent it is possible).
Thoughts @djc?