Skip to content

Commit 0f378e6

Browse files
committed
Squashed commit: Fix for problem caused by SSL_WANT_READ or SSL_WANT_WRITE errors.
When SSL_WANT_READ or SSL_WANT_WRITE are encountered, it's typical to retry the call but this must be repeated with the exact same arguments. Without this change, openSSL requires that the address of the buffer passed is the same. However, buffers in python can change location in some circumstances which cause the retry to fail. By add the setting SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER, the requirement for the same buffer address is forgiven and the retry has a better chance of success. See cherrypy/cheroot#245 for discussion.
1 parent a33fe4f commit 0f378e6

File tree

5 files changed

+305
-3
lines changed

5 files changed

+305
-3
lines changed

.github/workflows/ci.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ jobs:
5757
python-version: ${{ matrix.PYTHON.VERSION }}
5858
- run: python -m pip install nox
5959
- run: nox
60-
env:
60+
env:
6161
NOXSESSION: ${{ matrix.PYTHON.NOXSESSION }}
6262
- uses: ./.github/actions/upload-coverage
6363

CHANGELOG.rst

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,22 @@ Changes:
1717

1818
- Added ``OpenSSL.SSL.Context.set_tls13_ciphersuites`` that allows the allowed TLS 1.3 ciphers.
1919

20+
25.2.0 (UNRELEASED)
21+
-------------------
22+
23+
Backward-incompatible changes:
24+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
25+
26+
pyOpenSSL now sets SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER by default, matching CPython's behavior. #1287.
27+
The minimum cryptography version is now 42.0.0.
28+
29+
Deprecations:
30+
^^^^^^^^^^^^^
31+
32+
Changes:
33+
^^^^^^^^
34+
35+
2036
25.1.0 (2025-05-17)
2137
-------------------
2238

setup.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ def find_meta(meta):
9494
packages=find_packages(where="src"),
9595
package_dir={"": "src"},
9696
install_requires=[
97-
"cryptography>=41.0.5,<46",
97+
"cryptography>=42.0.0,<46",
9898
(
9999
"typing-extensions>=4.9; "
100100
"python_version < '3.13' and python_version >= '3.8'"

src/OpenSSL/SSL.py

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -915,7 +915,10 @@ def __init__(self, method: int) -> None:
915915
)
916916
self._cookie_verify_helper: _CookieVerifyCallbackHelper | None = None
917917

918-
self.set_mode(_lib.SSL_MODE_ENABLE_PARTIAL_WRITE)
918+
self.set_mode(
919+
_lib.SSL_MODE_ENABLE_PARTIAL_WRITE
920+
| _lib.SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER
921+
)
919922
if version is not None:
920923
self.set_min_proto_version(version)
921924
self.set_max_proto_version(version)
@@ -1740,6 +1743,14 @@ def set_mode(self, mode: int) -> int:
17401743

17411744
return _lib.SSL_CTX_set_mode(self._context, mode)
17421745

1746+
@_require_not_used
1747+
def clear_mode(self, mode_to_clear: int) -> int:
1748+
"""
1749+
Modes previously set cannot be overwritten without being
1750+
cleared first. This method should be used to clear existing modes.
1751+
"""
1752+
return _lib.SSL_CTX_clear_mode(self._context, mode_to_clear)
1753+
17431754
@_require_not_used
17441755
def set_tlsext_servername_callback(
17451756
self, callback: Callable[[Connection], None]

tests/test_ssl.py

Lines changed: 275 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@
77

88
from __future__ import annotations
99

10+
import contextlib
1011
import datetime
1112
import gc
13+
import logging
1214
import os
1315
import pathlib
1416
import select
@@ -31,6 +33,9 @@
3133
AF_INET6,
3234
MSG_PEEK,
3335
SHUT_RDWR,
36+
SO_RCVBUF,
37+
SO_SNDBUF,
38+
SOL_SOCKET,
3439
gaierror,
3540
socket,
3641
)
@@ -137,6 +142,8 @@
137142
)
138143
from .util import NON_ASCII, WARNING_TYPE_EXPECTED
139144

145+
logger = logging.getLogger(__name__)
146+
140147
# openssl dhparam 2048 -out dh-2048.pem
141148
dhparam = """\
142149
-----BEGIN DH PARAMETERS-----
@@ -413,6 +420,96 @@ def handshake_in_memory(
413420
interact_in_memory(client_conn, server_conn)
414421

415422

423+
def get_ssl_error_reason(ssl_error: SSL.Error) -> str | None:
424+
"""
425+
Extracts the reason string from the first error tuple in an SSL.Error.
426+
Returns None if the expected error structure is not found.
427+
"""
428+
if (
429+
ssl_error.args
430+
and isinstance(ssl_error.args, tuple)
431+
and len(ssl_error.args) > 0
432+
):
433+
error_details = ssl_error.args[0] # list of error tuples
434+
if isinstance(error_details, list) and len(error_details) > 0:
435+
first_error_tuple = error_details[0]
436+
if (
437+
isinstance(first_error_tuple, tuple)
438+
and len(first_error_tuple) >= 3
439+
):
440+
reason = first_error_tuple[2]
441+
if isinstance(reason, str):
442+
return reason
443+
return None
444+
445+
446+
def create_ssl_nonblocking_connection(
447+
mode: int | None, request_send_buffer_size: int
448+
) -> tuple[Connection, Connection, int, int]:
449+
"""
450+
Create a pair of sockets and set up an SSL connection between them.
451+
mode: The mode to set if not None.
452+
request_send_buffer_size: requested size of the send buffer
453+
Returns the SSL Connection objects
454+
and the actual send/receive buffer sizes.
455+
"""
456+
457+
client_socket, server_socket = socket_pair()
458+
459+
# Set up client context
460+
client_ctx = Context(SSLv23_METHOD)
461+
462+
# SSL_MODE_ENABLE_PARTIAL_WRITE and
463+
# SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER modes
464+
# are set by default when ctx is initialized.
465+
# Clear them if requested so tests can
466+
# be run without them if so desired.
467+
if mode is not None:
468+
client_ctx.clear_mode(
469+
_lib.SSL_MODE_ENABLE_PARTIAL_WRITE
470+
| _lib.SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER
471+
)
472+
# Set the new mode to the requested value
473+
client_ctx.set_mode(mode)
474+
475+
# create the SSL connections
476+
client = Connection(client_ctx, client_socket)
477+
server = loopback_server_factory(server_socket)
478+
479+
# Allow caller to request small buffer sizes so they can be easily filled.
480+
# Note the OS may not respect the requested values.
481+
# Make the receive buffer smaller than the send buffer.
482+
requested_receive_buffer_size = request_send_buffer_size // 2
483+
client_socket.setsockopt(SOL_SOCKET, SO_SNDBUF, request_send_buffer_size)
484+
actual_sndbuf = client_socket.getsockopt(SOL_SOCKET, SO_SNDBUF)
485+
logger.debug(
486+
f"Attempted SO_SNDBUF: {request_send_buffer_size}, "
487+
f"Actual SO_SNDBUF: {actual_sndbuf}"
488+
)
489+
490+
server_socket.setsockopt(
491+
SOL_SOCKET, SO_RCVBUF, requested_receive_buffer_size
492+
)
493+
actual_rcvbuf = server_socket.getsockopt(SOL_SOCKET, SO_RCVBUF)
494+
logger.debug(
495+
f"Attempted SO_RCVBUF: {requested_receive_buffer_size}, "
496+
f"Actual SO_RCVBUF: {actual_rcvbuf}"
497+
)
498+
499+
# set the connection state
500+
client.set_connect_state()
501+
# loopback_server_factory already sets the accept state on the server
502+
503+
handshake(client, server)
504+
505+
return (
506+
client,
507+
server,
508+
actual_sndbuf,
509+
actual_rcvbuf,
510+
)
511+
512+
416513
class TestVersion:
417514
"""
418515
Tests for version information exposed by `OpenSSL.SSL.SSLeay_version` and
@@ -3011,6 +3108,184 @@ def test_wantWriteError(self) -> None:
30113108

30123109
# XXX want_read
30133110

3111+
def _attempt_want_write_error(
3112+
self, client: Connection, buffer_size: int
3113+
) -> bytes:
3114+
"""
3115+
Deliberately attempts to send application data
3116+
over SSL to trigger WantWriteError. The send may need
3117+
to be repeated many times depending on the socket and
3118+
network buffer sizes allocated by the environment.
3119+
Returns the message that triggered the error so that
3120+
the buffer for the message is not immediately reclaimed.
3121+
"""
3122+
initial_want_write_triggered = False
3123+
max_num_of_attempts = 100000
3124+
3125+
for i in range(max_num_of_attempts):
3126+
msg = b"Y" * buffer_size
3127+
try:
3128+
client.send(msg)
3129+
except SSL.WantWriteError:
3130+
initial_want_write_triggered = True
3131+
break # Exit loop as desired error was triggered
3132+
3133+
assert initial_want_write_triggered, (
3134+
f"Could not induce WantWriteError within {i + 1} attempts"
3135+
)
3136+
return msg
3137+
3138+
def _drain_server_buffers(
3139+
self, server: Connection
3140+
) -> None:
3141+
"""Reads from server SSL and raw sockets to drain any pending data."""
3142+
total_ssl_read = 0
3143+
consecutive_empty_ssl_reads = 0
3144+
3145+
while total_ssl_read < 1024 * 1024:
3146+
try:
3147+
data = server.recv(65536)
3148+
# if serverbuffer is empty the call should
3149+
# raise WantReadError not return None
3150+
assert data is not None, "SSL peer closed or empty data"
3151+
total_ssl_read += len(data)
3152+
# Reset counter on successful read
3153+
consecutive_empty_ssl_reads = 0
3154+
except SSL.WantReadError:
3155+
consecutive_empty_ssl_reads += 1
3156+
if consecutive_empty_ssl_reads >= 10:
3157+
# "No more SSL application data available after
3158+
# consecutive_empty_ssl_readss
3159+
return
3160+
# Small delay to allow time for clearing buffers
3161+
time.sleep(0.01)
3162+
3163+
def _perform_moving_buffer_test(
3164+
self, client: Connection, buffer_size: int, want_bad_retry: bool
3165+
) -> bool:
3166+
"""
3167+
Attempts a retry write with a moving buffer and checks for
3168+
'bad write retry' error.
3169+
Returns True if 'bad write retry' occurs, False otherwise.
3170+
"""
3171+
logger.debug("Phase 3: Performing moving buffer retry test")
3172+
3173+
# Attempt retry with different buffer but same size
3174+
msg2 = b"Z" * buffer_size
3175+
logger.debug(f"buffer location for msg2 is {id(msg2):#x}")
3176+
try:
3177+
bytes_written = client.send(msg2)
3178+
assert not want_bad_retry, (
3179+
"_perform_moving_buffer_test() failed as retry succeeded "
3180+
f"unexpectedly with {bytes_written} bytes written."
3181+
)
3182+
return False # Retry succeeded
3183+
except SSL.Error as e:
3184+
reason = get_ssl_error_reason(e)
3185+
assert reason == "bad write retry", (
3186+
f"Retry failed with unexpected SSL error: {e!r}({reason})."
3187+
)
3188+
logger.debug(f"Got SSL error: {e!r} ({reason}).")
3189+
return True # Bad write retry
3190+
3191+
def _shutdown_connections(
3192+
self,
3193+
client: Connection,
3194+
server: Connection,
3195+
) -> None:
3196+
"""Helper to safely shut down SSL connections and close sockets."""
3197+
if client:
3198+
with contextlib.suppress(SSL.Error):
3199+
# When closing connections in the test teardown stage,
3200+
# we don't care about possible TLS-level problems as the test
3201+
# was specifically emulating corner case situations
3202+
# pre-shutdown. We just attempt releasing resources
3203+
# if possible and disregard any possibly related
3204+
# problems that may occur at this point.
3205+
client.shutdown()
3206+
if server:
3207+
with contextlib.suppress(SSL.Error):
3208+
server.shutdown()
3209+
3210+
@pytest.fixture
3211+
def ssl_connection_setup(self, request):
3212+
"""
3213+
Sets up a non-blocking SSL connection for testing
3214+
bad_write_retry errors.
3215+
Modeflag allows the caller to turn off
3216+
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER which is normally
3217+
on by default.
3218+
"""
3219+
want_bad_retry = request.param.get("want_bad_retry")
3220+
request_buffer_size = request.param.get("request_buffer_size")
3221+
modeflag = request.param.get("modeflag")
3222+
3223+
client, server, sndbuf, rcvbuf = (
3224+
create_ssl_nonblocking_connection(modeflag, request_buffer_size)
3225+
)
3226+
# Use a buffer size that is half the size
3227+
# of the allocated socket buffers
3228+
buffer_size = min(sndbuf, rcvbuf) // 2
3229+
3230+
# Yield the resources needed by the test
3231+
yield (
3232+
client,
3233+
server,
3234+
buffer_size,
3235+
want_bad_retry,
3236+
)
3237+
3238+
# Teardown: Clean up the connections after the test finishes
3239+
self._shutdown_connections(
3240+
client, server
3241+
)
3242+
3243+
@pytest.mark.parametrize(
3244+
"ssl_connection_setup",
3245+
[
3246+
{
3247+
"request_buffer_size": 65536,
3248+
"modeflag": _lib.SSL_MODE_ENABLE_PARTIAL_WRITE,
3249+
"want_bad_retry": True,
3250+
},
3251+
{
3252+
"request_buffer_size": 65536,
3253+
"modeflag": None,
3254+
"want_bad_retry": False,
3255+
},
3256+
],
3257+
indirect=True,
3258+
)
3259+
def test_moving_buffer_behavior(self, ssl_connection_setup):
3260+
"""Tests for possible "bad write retry" errors over an SSL connection.
3261+
If an SSL connection partially processes some data,
3262+
and then hits an `OpenSSL.SSL.WantWriteError`,
3263+
the connection may expect a retry. When PyOpenSSL creates
3264+
a new connection object, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER is
3265+
applied by default. This mode allows for data to be sent from a
3266+
different buffer location, something that may happen if Python moves a
3267+
mutable object such as a bytearray as part of its memory management.
3268+
If the mode is turned off, OpenSSL will reject the resend with
3269+
"bad_write_retry" error.
3270+
"""
3271+
(
3272+
client,
3273+
server,
3274+
buffer_size,
3275+
want_bad_retry,
3276+
) = ssl_connection_setup
3277+
3278+
_ = self._attempt_want_write_error(client, buffer_size)
3279+
self._drain_server_buffers(server)
3280+
3281+
# Perform the test and get the result
3282+
result = self._perform_moving_buffer_test(
3283+
client, buffer_size, want_bad_retry
3284+
)
3285+
3286+
# Assert that the result matches the expected outcome from the fixture
3287+
assert result == want_bad_retry
3288+
30143289
def test_get_finished_before_connect(self) -> None:
30153290
"""
30163291
`Connection.get_finished` returns `None` before TLS handshake

0 commit comments

Comments
 (0)