Share more ssl consts and fix openssl by youknowone · Pull Request #6462 · RustPython/RustPython

youknowone

56-60: LGTM! Import changes align with centralized error handling.

The imports correctly reflect the migration from direct error type usage to helper functions (create_ssl_* helpers), and the use of canonical error type names (PySSLCertVerificationError, PySSLError) improves consistency.

1807-1828: Review ex_data pointer management for safety.

The ex_data setup stores raw pointers to Py<PySslSocket> (index 0) and SniCallbackData (SNI index):

Index 0 pointer (lines 1814-1815, 1882-1883): Stores &*py_ref as *const Py<PySslSocket>. This is safe because:
- py_ref (the PyRef<PySslSocket>) is returned to Python, so the object remains alive.
- The SSL connection is owned by PySslSocket.connection, so the SSL object cannot outlive the PySslSocket.
- Callbacks dereference this pointer, which remains valid as long as the PySslSocket exists.
SNI callback data (lines 1821-1827, 1889-1895): Uses Box::into_raw to leak the SniCallbackData, with cleanup via cleanup_sni_ex_data after handshake. The weak reference to PySslSocket prevents reference cycles.

The implementation appears correct. However, the safety relies on the invariant that the SSL object (stored in PySslSocket.connection) does not outlive the PySslSocket itself. This is enforced by Rust ownership, but worth documenting explicitly.

Also applies to: 1875-1896

1983-2000: LGTM! Socket select logic correctly avoids busy-looping.

The updated logic properly handles blocking sockets by passing None as the timeout to sock_select, allowing the socket to wait indefinitely instead of busy-looping. This is a correctness improvement that aligns with the comment on lines 1983-1984.

2290-2302: LGTM! Thread-local SSL pointer pattern correctly avoids deadlocks.

These methods now use get_ssl_ptr_for_context_change instead of directly acquiring connection.read(). This pattern avoids deadlocks when these methods are called during callbacks (SNI, msg_callback) where the connection lock is already held:

During handshake/callbacks: HANDSHAKE_SSL_PTR thread-local is set by HandshakeVmGuard, so the SSL pointer is retrieved without locking.
Outside callbacks: Thread-local is None, so the lock is safely acquired.

This is a well-designed solution for the callback context problem.

Also applies to: 2311-2372, 2375-2398, 2509-2522, 2724-2728

2538-2541: LGTM! Handshake error handling correctly uses canonical error types.

The handshake error paths now use PySSLCertVerificationError::class() (canonical name) and properly set verification info on certificate verification errors, consistent with the PR's error handling improvements.

Also applies to: 2593-2597

2731-2838: LGTM! Read method signature and logic correctly updated.

The parameter type change from i32 to isize aligns with CPython's use of Py_ssize_t. The complex logic for handling negative n and buffer presence (lines 2737-2756) correctly implements CPython's behavior:

With buffer: If n <= 0 or n > buffer_len, use buffer_len; otherwise use n.
Without buffer: Raise ValueError if n < 0.

The buffer slicing at lines 2777-2780 includes a safety fallback, though it should not normally execute given the earlier validation. The zero-length read optimization (lines 2759-2765) is appropriate.

3318-3427: LGTM! Error handling consistently uses canonical error types.

The convert_openssl_error and new_ssl_error functions now consistently use canonical error type names (PySSLError, PySSLCertVerificationError) instead of aliases, improving code consistency and aligning with the PR's error handling refactoring.

3454-3501: LGTM! SSL error conversion uses centralized helper functions.

The convert_ssl_error function now uses helper functions (create_ssl_want_read_error, create_ssl_want_write_error, create_ssl_eof_error) for specific error cases, aligning with the PR's centralized error handling approach. The error detection logic remains correct, with proper handling of WANT_READ/WANT_WRITE, SYSCALL, and unexpected EOF cases.

1345-1347: LGTM! ASCII validation for cadata string is appropriate.

The added ASCII check for cadata when provided as a string (line 1345) is correct, as PEM format is ASCII-based. This improves input validation and provides a clear error message for invalid input.