Skip to content

Commit a11d38d

Browse files
committed
🐛 Handle stray exceptions in worker threads
This patch aims to prevent a situation when the working threads are killed by arbitrary exceptions that bubble up to their entry point layers that aren't handled properly or at all. This was causing DoS in many situations, including TLS errors and attempts to close the underlying sockets erroring out. Fixes #358 Ref #375 Resolves #365
1 parent 37c2196 commit a11d38d

File tree

1 file changed

+69
-2
lines changed

1 file changed

+69
-2
lines changed

cheroot/workers/threadpool.py

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
"""
77

88
import collections
9+
import logging
910
import threading
1011
import time
1112
import socket
@@ -118,7 +119,52 @@ def run(self):
118119
keep_conn_open = False
119120
try:
120121
keep_conn_open = conn.communicate()
122+
except ConnectionError as connection_error:
123+
keep_conn_open = False # Drop the connection cleanly
124+
self.server.error_log(
125+
'Got a connection error while handling a '
126+
f'connection from {conn.remote_addr !s}:'
127+
f'{conn.remote_port !s} ({connection_error !s})',
128+
level=logging.INFO,
129+
)
130+
continue
131+
except (KeyboardInterrupt, SystemExit) as shutdown_request:
132+
# Shutdown request
133+
keep_conn_open = False # Drop the connection cleanly
134+
self.server.error_log(
135+
'Got a server shutdown request while handling a '
136+
f'connection from {conn.remote_addr !s}:'
137+
f'{conn.remote_port !s} ({shutdown_request !s})',
138+
level=logging.DEBUG,
139+
)
140+
raise SystemExit(
141+
str(shutdown_request),
142+
) from shutdown_request
143+
except Exception as unhandled_error:
144+
# NOTE: Only a shutdown request should bubble up to the
145+
# NOTE: external cleanup code. Otherwise, this thread dies.
146+
# NOTE: If this were to happen, the threadpool would still
147+
# NOTE: list a dead thread without knowing its state. And
148+
# NOTE: the calling code would fail to schedule processing
149+
# NOTE: of new requests.
150+
self.server.error_log(
151+
'Unhandled error while processing an incoming '
152+
f'connection {unhandled_error !r}',
153+
level=logging.ERROR,
154+
traceback=True,
155+
)
156+
continue # Prevent the thread from dying
121157
finally:
158+
# NOTE: Any exceptions coming from within `finally` may
159+
# NOTE: kill the thread, causing the threadpool to only
160+
# NOTE: contain references to dead threads rendering the
161+
# NOTE: server defunct, effectively meaning a DoS.
162+
# NOTE: Ideally, things called here should process
163+
# NOTE: everything recoverable internally. Any unhandled
164+
# NOTE: errors will bubble up into the outer try/except
165+
# NOTE: block. They will be treated as fatal and turned
166+
# NOTE: into server shutdown requests and then reraised
167+
# NOTE: unconditionally.
122168
if keep_conn_open:
123169
self.server.put_conn(conn)
124170
else:
@@ -130,8 +176,29 @@ def run(self):
130176
self.work_time += time.time() - self.start_time
131177
self.start_time = None
132178
self.conn = None
133-
except (KeyboardInterrupt, SystemExit) as ex:
134-
self.server.interrupt = ex
179+
except (KeyboardInterrupt, SystemExit) as interrupt_exc:
180+
interrupt_cause = interrupt_exc.__cause__ or interrupt_exc
181+
self.server.error_log(
182+
f'Setting the server interrupt flag to {interrupt_cause !r}',
183+
level=logging.DEBUG,
184+
)
185+
self.server.interrupt = interrupt_cause
186+
except BaseException as underlying_exc:
187+
# NOTE: This is the last resort logging with the last dying breath
188+
# NOTE: of the worker. It is only reachable when exceptions happen
189+
# NOTE: in the `finally` branch of the internal try/except block.
190+
self.server.error_log(
191+
'A fatal exception happened. Setting the server interrupt flag'
192+
f'to {underlying_exc !r} and giving up.'
193+
'\N{NEW LINE}\N{NEW LINE}'
194+
'Please, report this on the Cheroot tracker at '
195+
'<https://github.com/cherrypy/cheroot/issues/new/choose>, '
196+
'providing a full reproducer with as much context and details as possible.',
197+
level=logging.CRITICAL,
198+
traceback=True,
199+
)
200+
self.server.interrupt = underlying_exc
201+
raise
135202

136203

137204
class ThreadPool:

0 commit comments

Comments
 (0)