Fix flickery haproxy tests - #145
Conversation
OpenSSL by default only reads a record a time, which is visible to callers as it affects IO readiness of the underlying socket. See `SSL_get_read_ahead` and associated. Rustls reads blindly the underlying transport. This means haproxy sometimes hangs as enters the TLS library entirely on the basis of IO readiness, rather than looking at any libssl API. This change constrains reading to follow TLS records, which is OpenSSL's default.
If we can't make progress to handshake completion due to IO, we shouldn't be reporting success.
| /// Account for `data`, which was just read from the transport. | ||
| fn consume(&mut self, data: &[u8]) { | ||
| if self.body_remaining > 0 { | ||
| self.body_remaining -= data.len().min(self.body_remaining); |
There was a problem hiding this comment.
Nit: maybe Ord::min()? (Also below.)
| } | ||
|
|
||
| /// Account for `data`, which was just read from the transport. | ||
| fn consume(&mut self, data: &[u8]) { |
There was a problem hiding this comment.
Nit: this consume() name seems a little confusing, since it's not like RecordLimit is ingesting the bytes -- maybe update()?
| self.header_used += take; | ||
|
|
||
| if self.header_used == Self::HEADER_LEN { | ||
| self.body_remaining = u16::from_be_bytes([self.header[3], self.header[4]]) as usize; |
There was a problem hiding this comment.
Isn't it possible for data to contain some prefix of the body at this point?
| /// `try_io()` can return `Ok(())` with the handshake still in flight: | ||
| /// `complete_io` gives up early if the `BIO` blocks part-way through |
There was a problem hiding this comment.
Nit: initial documentation comment line?
| /// (it never reaches the error stack) and leaves `SSL_get_error` to | ||
| /// report `SSL_ERROR_WANT_READ`/`_WRITE` from the `BIO`'s retry flags, | ||
| /// which record whichever direction actually blocked. | ||
| fn check_handshake_complete(&self) -> Result<(), error::Error> { |
There was a problem hiding this comment.
Nit: is there a good reason for the error:: qualification here?
| if let Err(e) = acceptor.read_tls(bio) { | ||
| return Err(error::Error::from_io(e)); | ||
| }; | ||
| // Keep reading until we have the whole `ClientHello`. Stopping |
There was a problem hiding this comment.
Pre-existing, but maybe yield this out of the match to reduce drift a little?
| } | ||
|
|
||
| /// Account for `data`, which was just read from the transport. | ||
| fn consume(&mut self, data: &[u8]) { |
There was a problem hiding this comment.
I think it might be helpful here to add a debug_assert!(data.len() <= self.allowance()); to maintain the cross-function invariant where consume() is only fed at most allowance() bytes. WDYT?
| let mut offset = 0; | ||
| let mut reads = 0; | ||
| while offset < record.len() { | ||
| let take = limit.allowance().min(chunk).min(record.len() - offset); |
There was a problem hiding this comment.
I think this clamping is maybe forgiving an over-allowance that we want to test?
If I mutate allowance() to change the 0 arm to return just Self::HEADER_LEN without subtracting self.header_used, the tests still pass.
Maybe better as:
fn drive(limit: &mut RecordLimit, record: &[u8], chunk: usize) -> usize {
let mut offset = 0;
let mut reads = 0;
while offset < record.len() {
// the limiter must ask for exactly the rest of the header,
// then exactly the rest of the body
let expected = match offset < RecordLimit::HEADER_LEN {
true => RecordLimit::HEADER_LEN - offset,
false => record.len() - offset,
};
assert_eq!(limit.allowance(), expected);
let take = expected.min(chunk);
limit.consume(&record[offset..offset + take]);
offset += take;
reads += 1;
}
reads
}With that form in place the tests fail w/ my mutant version:
thread 'bio::tests::handles_split_header' (268500) panicked at src/bio.rs:479:13:
assertion `left == right` failed
left: 5
right: 4
| match rc { | ||
| 1 => Ok(read_bytes), | ||
| 1 => { | ||
| self.record_limit.consume(&buf[..read_bytes]); |
There was a problem hiding this comment.
Maybe worth a bit of defense in depth here against a buggy BIO_read_ex that stores more than the dlen it updates? Something like:
let read_bytes = read_bytes.min(buf.len());
self.record_limit.consume(&buf[..read_bytes]);
Ok(read_bytes)
3 participants
Assisted by Opus 5.