Skip to content

Commit

Permalink
🐛 Handle stray exceptions in worker threads
Browse files Browse the repository at this point in the history
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
  • Loading branch information
webknjaz committed Mar 13, 2024
1 parent 37c2196 commit a11d38d
Showing 1 changed file with 69 additions and 2 deletions.
71 changes: 69 additions & 2 deletions cheroot/workers/threadpool.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"""

import collections
import logging
import threading
import time
import socket
Expand Down Expand Up @@ -118,7 +119,52 @@ def run(self):
keep_conn_open = False
try:
keep_conn_open = conn.communicate()
except ConnectionError as connection_error:
keep_conn_open = False # Drop the connection cleanly
self.server.error_log(

Check warning on line 124 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L122-L124

Added lines #L122 - L124 were not covered by tests
'Got a connection error while handling a '
f'connection from {conn.remote_addr !s}:'
f'{conn.remote_port !s} ({connection_error !s})',
level=logging.INFO,
)
continue
except (KeyboardInterrupt, SystemExit) as shutdown_request:

Check warning on line 131 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L130-L131

Added lines #L130 - L131 were not covered by tests
# Shutdown request
keep_conn_open = False # Drop the connection cleanly
self.server.error_log(

Check warning on line 134 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L133-L134

Added lines #L133 - L134 were not covered by tests
'Got a server shutdown request while handling a '
f'connection from {conn.remote_addr !s}:'
f'{conn.remote_port !s} ({shutdown_request !s})',
level=logging.DEBUG,
)
raise SystemExit(

Check warning on line 140 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L140

Added line #L140 was not covered by tests
str(shutdown_request),
) from shutdown_request
except Exception as unhandled_error:

Check warning on line 143 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L143

Added line #L143 was not covered by tests
# NOTE: Only a shutdown request should bubble up to the
# NOTE: external cleanup code. Otherwise, this thread dies.
# NOTE: If this were to happen, the threadpool would still
# NOTE: list a dead thread without knowing its state. And
# NOTE: the calling code would fail to schedule processing
# NOTE: of new requests.
self.server.error_log(

Check warning on line 150 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L150

Added line #L150 was not covered by tests
'Unhandled error while processing an incoming '
f'connection {unhandled_error !r}',
level=logging.ERROR,
traceback=True,
)
continue # Prevent the thread from dying

Check warning on line 156 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L156

Added line #L156 was not covered by tests
finally:
# NOTE: Any exceptions coming from within `finally` may
# NOTE: kill the thread, causing the threadpool to only
# NOTE: contain references to dead threads rendering the
# NOTE: server defunct, effectively meaning a DoS.
# NOTE: Ideally, things called here should process
# NOTE: everything recoverable internally. Any unhandled
# NOTE: errors will bubble up into the outer try/except
# NOTE: block. They will be treated as fatal and turned
# NOTE: into server shutdown requests and then reraised
# NOTE: unconditionally.
if keep_conn_open:
self.server.put_conn(conn)
else:
Expand All @@ -130,8 +176,29 @@ def run(self):
self.work_time += time.time() - self.start_time
self.start_time = None
self.conn = None
except (KeyboardInterrupt, SystemExit) as ex:
self.server.interrupt = ex
except (KeyboardInterrupt, SystemExit) as interrupt_exc:
interrupt_cause = interrupt_exc.__cause__ or interrupt_exc
self.server.error_log(

Check warning on line 181 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L179-L181

Added lines #L179 - L181 were not covered by tests
f'Setting the server interrupt flag to {interrupt_cause !r}',
level=logging.DEBUG,
)
self.server.interrupt = interrupt_cause
except BaseException as underlying_exc:

Check warning on line 186 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L185-L186

Added lines #L185 - L186 were not covered by tests
# NOTE: This is the last resort logging with the last dying breath
# NOTE: of the worker. It is only reachable when exceptions happen
# NOTE: in the `finally` branch of the internal try/except block.
self.server.error_log(

Check warning on line 190 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L190

Added line #L190 was not covered by tests
'A fatal exception happened. Setting the server interrupt flag'
f'to {underlying_exc !r} and giving up.'
'\N{NEW LINE}\N{NEW LINE}'
'Please, report this on the Cheroot tracker at '
'<https://github.com/cherrypy/cheroot/issues/new/choose>, '
'providing a full reproducer with as much context and details as possible.',
level=logging.CRITICAL,
traceback=True,
)
self.server.interrupt = underlying_exc
raise

Check warning on line 201 in cheroot/workers/threadpool.py

View check run for this annotation

Codecov / codecov/patch

cheroot/workers/threadpool.py#L200-L201

Added lines #L200 - L201 were not covered by tests


class ThreadPool:
Expand Down

0 comments on commit a11d38d

Please sign in to comment.