Skip to content

Commit 7c0981b

Browse files
youknowoneyouknowone
authored
Fix SSL handshake over-reading in STARTTLS (#7417)
During STARTTLS handshake, sock_recv(16KB) could consume application data that arrived alongside handshake records. The consumed data ended up in rustls's internal buffer where select() could not detect it, causing asyncore-based servers to miss readable events and the peer to time out. Use MSG_PEEK to find the TLS record boundary, then recv() only one complete record. Remaining data stays in the kernel TCP buffer, visible to select(). This matches OpenSSL's default no-read-ahead behaviour. Fixes flaky test_poplib (TestPOP3_TLSClass) failures.
1 parent a733c8b commit 7c0981b

File tree

5 files changed

+125
-35
lines changed

5 files changed

+125
-35
lines changed

‎crates/stdlib/src/openssl.rs‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2859,10 +2859,11 @@ mod _ssl {
28592859
// Wait briefly for peer's close_notify before retrying
28602860
match socket_stream.select(SslNeeds::Read, &deadline) {
28612861
SelectRet::TimedOut => {
2862-
return Err(vm.new_exception_msg(
2863-
vm.ctx.exceptions.timeout_error.to_owned(),
2864-
"The read operation timed out".to_owned(),
2865-
));
2862+
return Err(socket::timeout_error_msg(
2863+
vm,
2864+
"The read operation timed out".to_string(),
2865+
)
2866+
.upcast());
28662867
}
28672868
SelectRet::Closed => {
28682869
return Err(socket_closed_error(vm));
@@ -2901,10 +2902,7 @@ mod _ssl {
29012902
} else {
29022903
"The write operation timed out"
29032904
};
2904-
return Err(vm.new_exception_msg(
2905-
vm.ctx.exceptions.timeout_error.to_owned(),
2906-
msg.to_owned(),
2907-
));
2905+
return Err(socket::timeout_error_msg(vm, msg.to_string()).upcast());
29082906
}
29092907
SelectRet::Closed => {
29102908
return Err(socket_closed_error(vm));

‎crates/stdlib/src/ssl.rs‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4188,10 +4188,11 @@ mod _ssl {
41884188
let now = std::time::Instant::now();
41894189
if now >= dl {
41904190
// Timeout reached - raise TimeoutError
4191-
return Err(vm.new_exception_msg(
4192-
vm.ctx.exceptions.timeout_error.to_owned(),
4193-
"The read operation timed out".into(),
4194-
));
4191+
return Err(timeout_error_msg(
4192+
vm,
4193+
"The read operation timed out".to_string(),
4194+
)
4195+
.upcast());
41954196
}
41964197
Some(dl - now)
41974198
} else {
@@ -4207,11 +4208,11 @@ mod _ssl {
42074208

42084209
if timed_out {
42094210
// Timeout waiting for peer's close_notify
4210-
// Raise TimeoutError
4211-
return Err(vm.new_exception_msg(
4212-
vm.ctx.exceptions.timeout_error.to_owned(),
4213-
"The read operation timed out".into(),
4214-
));
4211+
return Err(timeout_error_msg(
4212+
vm,
4213+
"The read operation timed out".to_string(),
4214+
)
4215+
.upcast());
42154216
}
42164217

42174218
// Try to read data from socket

‎crates/stdlib/src/ssl/compat.rs‎

Lines changed: 93 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1161,6 +1161,83 @@ fn handshake_write_loop(
11611161
///
11621162
/// Waits for and reads TLS records from the peer, handling SNI callback setup.
11631163
/// Returns (made_progress, is_first_sni_read).
1164+
/// TLS record header size (content_type + version + length).
1165+
const TLS_RECORD_HEADER_SIZE: usize = 5;
1166+
1167+
/// Determine how many bytes to read from the socket during a TLS handshake.
1168+
///
1169+
/// OpenSSL reads one TLS record at a time (no read-ahead by default).
1170+
/// Rustls, however, consumes all available TCP data when fed via read_tls().
1171+
/// If application data arrives simultaneously with the final handshake record,
1172+
/// the eager read drains the TCP buffer, leaving the app data in rustls's
1173+
/// internal buffer where select() cannot see it. This causes asyncore-based
1174+
/// servers (which rely on select() for readability) to miss the data and the
1175+
/// peer times out.
1176+
///
1177+
/// Fix: peek at the TCP buffer to find the first complete TLS record boundary
1178+
/// and recv() only that many bytes. Any remaining data (including application
1179+
/// data that piggybacked on the same TCP segment) stays in the kernel buffer
1180+
/// and remains visible to select().
1181+
fn handshake_recv_one_record(socket: &PySSLSocket, vm: &VirtualMachine) -> SslResult<PyObjectRef> {
1182+
// Peek at what is available without consuming it.
1183+
let peeked_obj = match socket.sock_peek(SSL3_RT_MAX_PLAIN_LENGTH, vm) {
1184+
Ok(d) => d,
1185+
Err(e) => {
1186+
if is_blocking_io_error(&e, vm) {
1187+
return Err(SslError::WantRead);
1188+
}
1189+
return Err(SslError::Py(e));
1190+
}
1191+
};
1192+
1193+
let peeked = ArgBytesLike::try_from_object(vm, peeked_obj)
1194+
.map_err(|_| SslError::Syscall("Expected bytes-like object from peek".to_string()))?;
1195+
let peeked_bytes = peeked.borrow_buf();
1196+
1197+
if peeked_bytes.is_empty() {
1198+
return Err(SslError::WantRead);
1199+
}
1200+
1201+
if peeked_bytes.len() < TLS_RECORD_HEADER_SIZE {
1202+
// Not enough data for a TLS record header yet.
1203+
// Read all available bytes so rustls can buffer the partial header;
1204+
// this avoids busy-waiting because the kernel buffer is now empty
1205+
// and select() will only wake us when new data arrives.
1206+
return socket.sock_recv(peeked_bytes.len(), vm).map_err(|e| {
1207+
if is_blocking_io_error(&e, vm) {
1208+
SslError::WantRead
1209+
} else {
1210+
SslError::Py(e)
1211+
}
1212+
});
1213+
}
1214+
1215+
// Parse the TLS record length from the header.
1216+
let record_body_len = u16::from_be_bytes([peeked_bytes[3], peeked_bytes[4]]) as usize;
1217+
let total_record_size = TLS_RECORD_HEADER_SIZE + record_body_len;
1218+
1219+
let recv_size = if peeked_bytes.len() >= total_record_size {
1220+
// Complete record available — consume exactly one record.
1221+
total_record_size
1222+
} else {
1223+
// Incomplete record — consume everything so the kernel buffer is
1224+
// drained and select() will block until more data arrives.
1225+
peeked_bytes.len()
1226+
};
1227+
1228+
// Must drop the borrow before calling sock_recv (which re-enters Python).
1229+
drop(peeked_bytes);
1230+
drop(peeked);
1231+
1232+
socket.sock_recv(recv_size, vm).map_err(|e| {
1233+
if is_blocking_io_error(&e, vm) {
1234+
SslError::WantRead
1235+
} else {
1236+
SslError::Py(e)
1237+
}
1238+
})
1239+
}
1240+
11641241
fn handshake_read_data(
11651242
conn: &mut TlsConnection,
11661243
socket: &PySSLSocket,
@@ -1189,23 +1266,25 @@ fn handshake_read_data(
11891266
}
11901267
}
11911268

1192-
let data_obj = match socket.sock_recv(SSL3_RT_MAX_PLAIN_LENGTH, vm) {
1193-
Ok(d) => d,
1194-
Err(e) => {
1195-
if is_blocking_io_error(&e, vm) {
1196-
return Err(SslError::WantRead);
1197-
}
1198-
// In socket mode, if recv times out and we're only waiting for read
1199-
// (no wants_write), we might be waiting for optional NewSessionTicket in TLS 1.3
1200-
// Consider the handshake complete
1201-
if !is_bio && !conn.wants_write() {
1202-
// Check if it's a timeout exception
1203-
if e.fast_isinstance(vm.ctx.exceptions.timeout_error) {
1204-
// Timeout waiting for optional data - handshake is complete
1269+
let data_obj = if !is_bio {
1270+
// In socket mode, read one TLS record at a time to avoid consuming
1271+
// application data that may arrive alongside the final handshake
1272+
// record. This matches OpenSSL's default (no read-ahead) behaviour
1273+
// and keeps remaining data in the kernel buffer where select() can
1274+
// detect it.
1275+
handshake_recv_one_record(socket, vm)?
1276+
} else {
1277+
match socket.sock_recv(SSL3_RT_MAX_PLAIN_LENGTH, vm) {
1278+
Ok(d) => d,
1279+
Err(e) => {
1280+
if is_blocking_io_error(&e, vm) {
1281+
return Err(SslError::WantRead);
1282+
}
1283+
if !conn.wants_write() && e.fast_isinstance(vm.ctx.exceptions.timeout_error) {
12051284
return Ok((false, false));
12061285
}
1286+
return Err(SslError::Py(e));
12071287
}
1208-
return Err(SslError::Py(e));
12091288
}
12101289
};
12111290

‎crates/vm/src/stdlib/_winapi.rs‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1614,7 +1614,13 @@ mod _winapi {
16141614

16151615
if let Some(err) = err {
16161616
if err == windows_sys::Win32::Foundation::WAIT_TIMEOUT {
1617-
return Err(vm.new_exception_empty(vm.ctx.exceptions.timeout_error.to_owned()));
1617+
return Err(vm
1618+
.new_os_subtype_error(
1619+
vm.ctx.exceptions.timeout_error.to_owned(),
1620+
None,
1621+
"timed out",
1622+
)
1623+
.upcast());
16181624
}
16191625
if err == windows_sys::Win32::Foundation::ERROR_CONTROL_C_EXIT {
16201626
return Err(vm
@@ -1783,7 +1789,13 @@ mod _winapi {
17831789
// Return result
17841790
if let Some(e) = thread_err {
17851791
if e == windows_sys::Win32::Foundation::WAIT_TIMEOUT {
1786-
return Err(vm.new_exception_empty(vm.ctx.exceptions.timeout_error.to_owned()));
1792+
return Err(vm
1793+
.new_os_subtype_error(
1794+
vm.ctx.exceptions.timeout_error.to_owned(),
1795+
None,
1796+
"timed out",
1797+
)
1798+
.upcast());
17871799
}
17881800
if e == windows_sys::Win32::Foundation::ERROR_CONTROL_C_EXIT {
17891801
return Err(vm

‎crates/vm/src/vm/vm_new.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,8 @@ impl VirtualMachine {
110110
debug_assert_eq!(
111111
exc_type.slots.basicsize,
112112
core::mem::size_of::<PyBaseException>(),
113-
"vm.new_exception() is only for exception types without additional payload. The given type '{}' is not allowed.",
114-
exc_type.class().name()
113+
"vm.new_exception() is only for exception types without additional payload. The given type '{}' is not allowed. Use vm.new_os_subtype_error() for OSError subtypes.",
114+
exc_type.name()
115115
);
116116

117117
PyRef::new_ref(

0 commit comments

Comments
 (0)