Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
9aac18d
Refactor of _from_server_socket()
julianz- Apr 7, 2026
b469389
Simply the socket OS error handling
julianz- Apr 7, 2026
4341144
Move the wrap() operation into a separate helper
julianz- Apr 7, 2026
0f28843
refactor _from_server_socket exception handler
julianz- Apr 7, 2026
526c291
Move socket acceptance into separate helper
julianz- Apr 7, 2026
63a63ad
Add socket configuration helper
julianz- Apr 7, 2026
45b26ef
Simplify _from_server_socket into two steps
julianz- Apr 7, 2026
9524853
Added IPv4 and IPv6 to spell check dicitonary
julianz- Apr 7, 2026
3b39b17
Added towncrier statement for whole refactor
julianz- Apr 7, 2026
de7bee4
Added tests for refactored code to improve coverage
julianz- Apr 7, 2026
be92e4b
Fix test_tls_client_auth for Windows 2025
julianz- Apr 9, 2026
57031ff
Updated tests for from_server_socket
julianz- Apr 11, 2026
6892f6b
Fix socket leak in _wrap_socket_for_tls on FatalSSLAlert
julianz- Jun 19, 2026
5a26b85
Rename _ignore_socket_oserror to _is_ignorable_socket_error
julianz- Jun 19, 2026
046a870
Replace sentinel tuple returns with exceptions in _prepare_socket
julianz- Jun 19, 2026
0348719
Use full variable names and positional-only args in socket methods
julianz- Jun 19, 2026
6a2809c
Use InterruptedError instead of OSError(EINTR) in transport error test
julianz- Jun 20, 2026
809dd22
Simplify transport error test by removing unnecessary pipe_fd fixture
julianz- Jun 20, 2026
01fde84
Document socket.timeout as OSError subclass and TimeoutError alias
julianz- Jun 20, 2026
050a198
Fix changelog fragment formatting
julianz- Jun 20, 2026
96aa39f
Rephrase _prepare_socket docstring to avoid spell check failures
julianz- Jun 20, 2026
2b63a58
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 20, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 122 additions & 74 deletions cheroot/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -295,88 +295,136 @@ def _remove_invalid_sockets(self):
with _cm.suppress(OSError):
conn.close()

def _from_server_socket(self, server_socket): # noqa: C901 # FIXME
def _setup_conn_addr(self, conn, s, addr):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use full words for variable names (things like i/j/k/s aren't really descriptive, nor are the shortened words). It'd also be a good idea to settle for all the args being keyword-only or positional-only (seems like in this case positional args of different types make up bad DX).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"""Configure remote address and port for the connection.

Populates the connection object with remote address metadata.
If the address is unavailable following the handshake, this
method detects the socket's address family (IPv4/IPv6)
and provides an appropriate 'any' address placeholder.
"""
# optional values
# Until we do DNS lookups, omit REMOTE_HOST
if addr is None: # sometimes this can happen
# figure out if AF_INET or AF_INET6.
if len(s.getsockname()) == 2:
# AF_INET
addr = ('0.0.0.0', 0)
else:
# AF_INET6
addr = ('::', 0)
conn.remote_addr = addr[0]
conn.remote_port = addr[1]

def _ignore_socket_oserror(self, exc):
"""Determine if an OSError during socket operations should be ignored."""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"determine" is another clue that the helper probably shouldn't say "ignore".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

if self.server.stats['Enabled']:
self.server.stats['Socket Errors'] += 1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably belongs outside a function saying "ignore". Though, maybe the helper could be renamed since it doesn't actually ignore anything but checks for expected exceptions.

How about something like def _is_suppressable_exc(exc: OSError, /) -> bool:? OTOH, could you check if specific OSError subclasses aren't being raised? Maybe, we could replace checking the low-level codes with suppressing the corresponding specific exceptions like InterruptedError and similar?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, though I prefer the slightly more intuitive _is_ignorable_socket_error() as the name. I added the / but since none of the other functions have type annotations I left those out β€” maybe that's better done in a separate PR?

err_code = exc.args[0]

# EINTR should occur when a signal is received during accept();
# retry the call. See https://github.com/cherrypy/cherrypy/issues/707.
is_eintr = err_code in errors.socket_error_eintr

# Just try again. See https://github.com/cherrypy/cherrypy/issues/479.
is_nonblocking = err_code in errors.socket_errors_nonblocking

# Our socket was closed. See https://github.com/cherrypy/cherrypy/issues/686.
is_ignored = err_code in errors.socket_errors_to_ignore

return is_eintr or is_nonblocking or is_ignored

def _wrap_socket_for_tls(self, raw_socket, addr):
"""Handle the TLS wrap and log specific error responses.

on success returns e.g. (SSLSocket, {'SSL_PROTOCOL': 'TLSv1.3', ...}).
on failure returns None, {}
"""
try:
s, addr = server_socket.accept()
if self.server.stats['Enabled']:
self.server.stats['Accepts'] += 1
prevent_socket_inheritance(s)
if hasattr(s, 'settimeout'):
s.settimeout(self.server.timeout)
return self.server.ssl_adapter.wrap(raw_socket)
except errors.FatalSSLAlert as tls_connection_drop_error:
self.server.error_log(
f'Client {addr!s} lost β€” peer dropped the TLS '
'connection suddenly, during handshake: '
f'{tls_connection_drop_error!s}',
)
return None, {}
except errors.NoSSLError as http_over_https_err:
self.server.error_log(
f'Client {addr!s} attempted to speak plain HTTP into '
'a TCP connection configured for TLS-only traffic β€” '
'trying to send back a plain HTTP error response: '
f'{http_over_https_err!s}',
)
self._send_bad_request_plain_http_error(raw_socket)

mf = MakeFile
ssl_env = {}
# if ssl cert and key are set, we try to be a secure HTTP server
if self.server.ssl_adapter is not None:
# FIXME: WPS505 -- too many nested blocks
try: # noqa: WPS505
s, ssl_env = self.server.ssl_adapter.wrap(s)
except errors.FatalSSLAlert as tls_connection_drop_error:
self.server.error_log(
f'Client {addr!s} lost β€” peer dropped the TLS '
'connection suddenly, during handshake: '
f'{tls_connection_drop_error!s}',
)
return None
except errors.NoSSLError as http_over_https_err:
self.server.error_log(
f'Client {addr!s} attempted to speak plain HTTP into '
'a TCP connection configured for TLS-only traffic β€” '
'trying to send back a plain HTTP error response: '
f'{http_over_https_err!s}',
)
self._send_bad_request_plain_http_error(s)
return None
mf = self.server.ssl_adapter.makefile
# Re-apply our timeout since we may have a new socket object
if hasattr(s, 'settimeout'):
s.settimeout(self.server.timeout)

conn = self.server.ConnectionClass(self.server, s, mf)

if not isinstance(self.server.bind_addr, (str, bytes)):
# optional values
# Until we do DNS lookups, omit REMOTE_HOST
if addr is None: # sometimes this can happen
# figure out if AF_INET or AF_INET6.
if len(s.getsockname()) == 2:
# AF_INET
addr = ('0.0.0.0', 0)
else:
# AF_INET6
addr = ('::', 0)
conn.remote_addr = addr[0]
conn.remote_port = addr[1]

conn.ssl_env = ssl_env
return conn
# If we hit either exception, close the socket and signal failure
Comment thread
julianz- marked this conversation as resolved.
with _cm.suppress(OSError):
raw_socket.close()
return None, {}

def _accept_conn(self, server_socket):
"""Accept the connection."""
try:
s, addr = server_socket.accept()
except socket.timeout:
Comment thread
julianz- marked this conversation as resolved.
# The only reason for the timeout in start() is so we can
# notice keyboard interrupts on Win32, which don't interrupt
# accept() by default
return None
except OSError as ex:
if self.server.stats['Enabled']:
self.server.stats['Socket Errors'] += 1
if ex.args[0] in errors.socket_error_eintr:
# I *think* this is right. EINTR should occur when a signal
# is received during the accept() call; all docs say retry
# the call, and I *think* I'm reading it right that Python
# will then go ahead and poll for and handle the signal
# elsewhere. See
# https://github.com/cherrypy/cherrypy/issues/707.
return None
if ex.args[0] in errors.socket_errors_nonblocking:
# Just try again. See
# https://github.com/cherrypy/cherrypy/issues/479.
return None
if ex.args[0] in errors.socket_errors_to_ignore:
# Our socket was closed.
# See https://github.com/cherrypy/cherrypy/issues/686.
except OSError as exc:
if self._ignore_socket_oserror(exc):
return None
raise

if self.server.stats['Enabled']:
self.server.stats['Accepts'] += 1

return s, addr

def _configure_socket(self, s):
"""Apply standard settings to a new socket."""
prevent_socket_inheritance(s)
if hasattr(s, 'settimeout'):
s.settimeout(self.server.timeout)

def _prepare_socket(self, server_socket):
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
"""Handle physical accept and TLS negotiation."""
result = self._accept_conn(server_socket)
if result is None:
return None, None, None, None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having an interface returning a tuple of multiple differently typed values is rather fragile.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revamped.

s, addr = result

self._configure_socket(s)

mf = MakeFile
ssl_env = {}

if self.server.ssl_adapter is not None:
s, ssl_env = self._wrap_socket_for_tls(s, addr)
if not s:
return None, None, None, None

mf = self.server.ssl_adapter.makefile
# Re-apply our timeout since we may have a new socket object
if hasattr(s, 'settimeout'):
s.settimeout(self.server.timeout)

return s, addr, mf, ssl_env

def _from_server_socket(self, server_socket):
"""Orchestrate the creation of a connection from a server socket."""
# Handle the connection acceptance and the TLS wrapping if applicable
s, addr, mf, ssl_env = self._prepare_socket(server_socket)
if s is None:
return None

# Initialize the Connection object
conn = self.server.ConnectionClass(self.server, s, mf)
conn.ssl_env = ssl_env

if not isinstance(self.server.bind_addr, (str, bytes)):
self._setup_conn_addr(conn, s, addr)

return conn

def close(self):
"""Close all monitored connections."""
for _, conn in self._selector.connections:
Expand Down
Loading
Loading