Skip to content

Commit 1c7c0c0

Browse files
committed
Fix race condition and improve robustness during socket I/O
Fixes to make socket I/O more resilient during connection teardown. 1. BufferedWriter's write(): Added error handling to ignore common socket errors (e.g., ECONNRESET, EPIPE, ENOTCONN, EBADF) that occur when the underlying connection has been unexpectedly closed by the client or OS. This prevents a crash when attempting to write to a defunct socket. 2. BufferedWriters's close(): Made idempotent, allowing safe repeated calls without raising exceptions. 3. Needed to add explicit handling of WINDOWS environments as these are seen to throw Windows specific WSAENOTSOCK errors. Includes new unit tests to cover the idempotency and graceful handling of already closed underlying buffers.
1 parent e9b4361 commit 1c7c0c0

File tree

7 files changed

+158
-8
lines changed

7 files changed

+158
-8
lines changed

cheroot/errors.py

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,14 @@
33
import errno
44
import sys
55

6+
from cheroot._compat import IS_WINDOWS
7+
8+
9+
try:
10+
from OpenSSL.SSL import SysCallError as _OpenSSL_SysCallError
11+
except ImportError:
12+
_OpenSSL_SysCallError = None
13+
614

715
class MaxSizeExceeded(Exception):
816
"""Exception raised when a client sends more data then allowed under limit.
@@ -64,13 +72,31 @@ def plat_specific_errors(*errnames):
6472
socket_errors_to_ignore.extend(plat_specific_errors('EPROTOTYPE'))
6573
socket_errors_nonblocking.extend(plat_specific_errors('EPROTOTYPE'))
6674

67-
6875
acceptable_sock_shutdown_error_codes = {
76+
errno.EBADF,
6977
errno.ENOTCONN,
7078
errno.EPIPE,
7179
errno.ESHUTDOWN, # corresponds to BrokenPipeError in Python 3
7280
errno.ECONNRESET, # corresponds to ConnectionResetError in Python 3
7381
}
82+
83+
if IS_WINDOWS:
84+
# Define Windows socket error code constant
85+
# WSAENOTSOCK: Socket operation on non-socket
86+
# Should be available from errno, but provide
87+
# a value as backup
88+
WSAENOTSOCK = 10038
89+
acceptable_sock_shutdown_error_codes.add(
90+
getattr(errno, 'WSAENOTSOCK', WSAENOTSOCK),
91+
)
92+
93+
acceptable_sock_shutdown_exceptions = (
94+
BrokenPipeError,
95+
ConnectionResetError,
96+
# conditionally add _OpenSSL_SysCallError to the list
97+
*(() if _OpenSSL_SysCallError is None else (_OpenSSL_SysCallError,)),
98+
)
99+
74100
"""Errors that may happen during the connection close sequence.
75101
76102
* ENOTCONN — client is no longer connected
@@ -86,5 +112,3 @@ def plat_specific_errors(*errnames):
86112
* https://github.com/python/cpython/blob/c39b52f/Lib/poplib.py#L297-L302
87113
* https://docs.microsoft.com/windows/win32/api/winsock/nf-winsock-shutdown
88114
"""
89-
90-
acceptable_sock_shutdown_exceptions = (BrokenPipeError, ConnectionResetError)

cheroot/errors.pyi

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
from typing import List, Set, Tuple, Type
22

3+
from cheroot._compat import IS_WINDOWS as IS_WINDOWS
4+
35
class MaxSizeExceeded(Exception): ...
46
class NoSSLError(Exception): ...
57
class FatalSSLAlert(Exception): ...

cheroot/makefile.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44
import _pyio as io
55
import socket
66

7+
from OpenSSL.SSL import SysCallError
8+
9+
from cheroot import errors
10+
711

812
# Write only 16K at a time to sockets
913
SOCK_WRITE_BLOCKSIZE = 16384
@@ -32,8 +36,57 @@ def _flush_unlocked(self):
3236
n = self.raw.write(bytes(self._write_buf))
3337
except io.BlockingIOError as e:
3438
n = e.characters_written
39+
except OSError as e:
40+
error_code = e.errno
41+
if error_code in errors.acceptable_sock_shutdown_error_codes:
42+
# The socket is gone, so just ignore this error.
43+
return
44+
raise
45+
except SysCallError as e:
46+
error_code = e.args[0]
47+
if error_code in errors.acceptable_sock_shutdown_error_codes:
48+
# The socket is gone, so just ignore this error.
49+
return
50+
raise
51+
else:
52+
# The 'try' block completed without an exception
53+
if n is None:
54+
# This could happen with non-blocking write
55+
# when nothing was written
56+
break
57+
3558
del self._write_buf[:n]
3659

60+
def close(self):
61+
"""
62+
Close the stream and its underlying file object.
63+
64+
This method is designed to be idempotent (it can be called multiple
65+
times without side effects). It gracefully handles a race condition
66+
where the underlying socket may have already been closed by the remote
67+
client or another thread.
68+
69+
A SysCallError or OSError with ``EBADF`` or ``ENOTCONN`` is caught
70+
and ignored, as these indicate a normal, expected connection teardown.
71+
Other exceptions are re-raised.
72+
"""
73+
if self.closed: # pylint: disable=W0125
74+
return
75+
76+
try:
77+
super().close()
78+
except (OSError, SysCallError) as sock_err:
79+
error_code = (
80+
sock_err.errno
81+
if isinstance(sock_err, OSError)
82+
else sock_err.args[0]
83+
)
84+
if error_code in errors.acceptable_sock_shutdown_error_codes:
85+
# The socket is already closed, which is expected during
86+
# a race condition.
87+
return
88+
raise
89+
3790

3891
class StreamReader(io.BufferedReader):
3992
"""Socket stream reader."""

cheroot/server.py

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767

6868
import contextlib
6969
import email.utils
70+
import errno
7071
import io
7172
import logging
7273
import os
@@ -81,6 +82,13 @@
8182
import urllib.parse
8283
from functools import lru_cache
8384

85+
from OpenSSL.SSL import SysCallError
86+
87+
from cheroot.errors import (
88+
acceptable_sock_shutdown_error_codes,
89+
acceptable_sock_shutdown_exceptions,
90+
)
91+
8492
from . import __version__, connections, errors
8593
from ._compat import IS_PPC, bton
8694
from .makefile import MakeFile, StreamWriter
@@ -1189,9 +1197,21 @@ def write(self, chunk):
11891197
if self.chunked_write and chunk:
11901198
chunk_size_hex = hex(len(chunk))[2:].encode('ascii')
11911199
buf = [chunk_size_hex, CRLF, chunk, CRLF]
1192-
self.conn.wfile.write(EMPTY.join(buf))
1200+
data = EMPTY.join(buf)
11931201
else:
1194-
self.conn.wfile.write(chunk)
1202+
data = chunk
1203+
1204+
try:
1205+
self.conn.wfile.write(data)
1206+
except (SysCallError, ConnectionError, OSError) as write_error:
1207+
if isinstance(write_error, OSError):
1208+
error_code = write_error.errno
1209+
else:
1210+
error_code = write_error.args[0]
1211+
if error_code in acceptable_sock_shutdown_error_codes:
1212+
# The socket is gone, so just ignore this error.
1213+
return
1214+
raise
11951215

11961216
def send_headers(self): # noqa: C901 # FIXME
11971217
"""Assert, process, and send the HTTP response message-headers.
@@ -1285,7 +1305,27 @@ def send_headers(self): # noqa: C901 # FIXME
12851305
for k, v in self.outheaders:
12861306
buf.append(k + COLON + SPACE + v + CRLF)
12871307
buf.append(CRLF)
1288-
self.conn.wfile.write(EMPTY.join(buf))
1308+
try:
1309+
self.conn.wfile.write(EMPTY.join(buf))
1310+
except (SysCallError, ConnectionError, OSError) as e:
1311+
# We explicitly ignore these errors because they indicate the
1312+
# client has already closed the connection, which is a normal
1313+
# occurrence during a race condition.
1314+
1315+
# The .errno attribute is only available on OSError
1316+
# The .args[0] attribute is available on SysCallError
1317+
# Check for both cases to handle different exception types
1318+
error_code = e.errno if isinstance(e, OSError) else e.args[0]
1319+
if error_code in {
1320+
errno.ECONNRESET,
1321+
errno.EPIPE,
1322+
errno.ENOTCONN,
1323+
errno.EBADF,
1324+
}:
1325+
self.close_connection = True
1326+
self.conn.close()
1327+
return
1328+
raise
12891329

12901330

12911331
class HTTPConnection:
@@ -1543,10 +1583,10 @@ def _close_kernel_socket(self):
15431583

15441584
try:
15451585
shutdown(socket.SHUT_RDWR) # actually send a TCP FIN
1546-
except errors.acceptable_sock_shutdown_exceptions:
1586+
except acceptable_sock_shutdown_exceptions: # pylint: disable=E0712
15471587
pass
15481588
except socket.error as e:
1549-
if e.errno not in errors.acceptable_sock_shutdown_error_codes:
1589+
if e.errno not in acceptable_sock_shutdown_error_codes:
15501590
raise
15511591

15521592

cheroot/test/test_makefile.py

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
"""Tests for :py:mod:`cheroot.makefile`."""
22

3+
import io
4+
35
from cheroot import makefile
46

57

@@ -51,3 +53,27 @@ def test_bytes_written():
5153
wfile = makefile.MakeFile(sock, 'w')
5254
wfile.write(b'bar')
5355
assert wfile.bytes_written == 3
56+
57+
58+
def test_close_is_idempotent():
59+
"""Test that close() can be called multiple times safely."""
60+
raw_buffer = io.BytesIO()
61+
buffered_writer = makefile.BufferedWriter(raw_buffer)
62+
63+
# Should not raise any exceptions
64+
buffered_writer.close()
65+
buffered_writer.close() # Second call should be safe
66+
67+
assert buffered_writer.closed
68+
69+
70+
def test_close_handles_already_closed_buffer():
71+
"""Test that close() handles already closed underlying buffer."""
72+
raw_buffer = io.BytesIO()
73+
buffered_writer = makefile.BufferedWriter(raw_buffer)
74+
75+
# Close the underlying buffer first
76+
raw_buffer.close()
77+
78+
# This should not raise an exception
79+
buffered_writer.close()
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fixed race conditions to make socket I/O more resilient during connection teardown.
2+
3+
-- by :user:`julianz-`

docs/spelling_wordlist.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,12 +57,14 @@ signalling
5757
Sigstore
5858
ssl
5959
stdout
60+
stubtest
6061
subclasses
6162
submodules
6263
subpackages
6364
symlinked
6465
syscall
6566
systemd
67+
teardown
6668
threadpool
6769
Tidelift
6870
TLS

0 commit comments

Comments
 (0)