PySSL_SetError needs to be updated

Bug report

Bug description:

It was dealt with other issues before, but the contents are not intuitive, so I rearrange them.

The SSL_write() and SSL_read() used in python 3.9 were modified from python 3.10 to SSL_write_ex() and SSL_read_ex(), and the return value was fixed to 0 on error. However, in PySSL_SetError, there is no update accordingly, so this part needs to be modified.

Related issue can be referred to the following link.
OpenSSL 1.1.1: use SSL_write_ex() and SSL_read_ex()

The difference between SSL_write() and SSL_write_ex() is as follows.

int SSL_write(SSL *s, const void *buf, int num)
{
    int ret;
    size_t written;

    if (num < 0) {
        ERR_raise(ERR_LIB_SSL, SSL_R_BAD_LENGTH);
        return -1;
    }

    ret = ssl_write_internal(s, buf, (size_t)num, 0, &written);

    /*
     * The cast is safe here because ret should be <= INT_MAX because num is
     * <= INT_MAX
     */
    if (ret > 0)
        ret = (int)written;

    return ret;
}

SSL_write() has a return value of 0 or less when an error occurs.

int SSL_write_ex(SSL *s, const void *buf, size_t num, size_t *written)
{
    return SSL_write_ex2(s, buf, num, 0, written);
}

int SSL_write_ex2(SSL *s, const void *buf, size_t num, uint64_t flags,
                  size_t *written)
{
    int ret = ssl_write_internal(s, buf, num, flags, written);

    if (ret < 0)
        ret = 0;
    return ret;
}

SSL_write_ex() is fixed to 0 when an error occurs. As the return value changes, the behavior of the PySSL_SetError function changes.

cpython/Modules/_ssl.c

Line 648 in f9154f8

if (ret == 0 || (((PyObject *)s) == Py_None)) {

case SSL_ERROR_SYSCALL:
{
	if (e == 0) {
		PySocketSockObject *s = GET_SOCKET(sslsock);
		if (ret == 0 || (((PyObject *)s) == Py_None)) {
			p = PY_SSL_ERROR_EOF;
			type = state->PySSLEOFErrorObject;
			errstr = "EOF occurred in violation of protocol";
		} else if (s && ret == -1) {
			/* underlying BIO reported an I/O error */
			ERR_clear_error();
#ifdef MS_WINDOWS
			if (err.ws) {
				return PyErr_SetFromWindowsErr(err.ws);
			}
#endif
			if (err.c) {
				errno = err.c;
				return PyErr_SetFromErrno(PyExc_OSError);
			}
			else {
				p = PY_SSL_ERROR_EOF;
				type = state->PySSLEOFErrorObject;
				errstr = "EOF occurred in violation of protocol";
			}
		} else { /* possible? */
			p = PY_SSL_ERROR_SYSCALL;
			type = state->PySSLSyscallErrorObject;
			errstr = "Some I/O error occurred";
		}
	} else {
		if (ERR_GET_LIB(e) == ERR_LIB_SSL &&
				ERR_GET_REASON(e) == SSL_R_CERTIFICATE_VERIFY_FAILED) {
			type = state->PySSLCertVerificationErrorObject;
		}
		p = PY_SSL_ERROR_SYSCALL;
	}
	break;
}

According to the part if (ret == 0 || ((PyObject *) == Py_None)), the retrun of the function SSL_write_ex() is fixed to 0, resulting in SSLEOFError instead of OSError.

I determined that the change to this error was not intended. If the connection is terminated by the other peer, it is better for the user to respond to it by returning it as an error such as BrokenPIPE rather than an error that is a protocol rule violation.

The modifications affected by the change have been identified as follows.

#25553

The behavior of the SSL test server for OSError has changed. ConnectionError did not previously shut down the server, but now it does.
I think this is because the change has not been tested as OSError does not occur at write or read.

#25574

It is believed that an intermittent EOF error occurred instead of an OSError.

#26520

OSError has disappeared, only EOF errors have been handled.

Related issues are as follows.

#110467
urllib3/urllib3#2733

CPython versions tested on:

3.10, 3.11, 3.12, 3.13, CPython main branch

Operating systems tested on:

Linux, macOS

Linked PRs

pythongh-115627: Fix PySSL_SetError handling SSL_ERROR_SYSCALL

Merge branch 'main' into pythongh-115627

pythongh-115627: Fix PySSL_SetError handling SSL_ERROR_SYSCALL

gh-115627: Fix PySSL_SetError handling SSL_ERROR_SYSCALL (GH-115628)

This possibly needs backporting, but I'd prefer testing it in main for a while first.

This possibly needs backporting, but I'd prefer testing it in main for a while first.

According to test_ssl.py changes, it seems like the exception type changes from SSLError to OSError in some cases. I'm not sure that such case is straightforward for a backport: it can break code expecting only SSLError, no? Maybe it's less risky to not backport such change and leave 3.11 and 3.12 code as it is.

Or maybe the backport should be different to not require to change code handling exceptions... which is the purpose of the change, change how exceptions are raised :-)

@serhiy-storchaka

@serhiy-storchaka: What do you think on the backport?

I do not know whether the change from SSLError to OSError is good or not.

Regarding #110467, I think backport needs backport for 3.10 ~ version.

I think these backports will solve the issues that libraries using the ssl function, which have been used since version 3.9 or lower, have had since version 3.10, such as the issues I linked to.

The test code of the issue and its results are as follows.

CODE

import http.client
import requests

try :
    print("START send http.client.HTTPSConnection")
    http.client.HTTPSConnection("google.com").request('POST', '/', body=b'A'*1000000)
    print("END send http.client.HTTPSConnection")
except Exception as e:
    print(e)

try :
    print("START send requests.post")
    requests.post('https://google.com/', data=b'A'*1000000)
    print("END send requests.post")
except Exception as e:
    print(e)

result of python 3.9

orange@32thread-server:~/project/pythonssleof$ python3.9 main.py
START send http.client.HTTPSConnection
[Errno 32] Broken pipe
START send requests.post
END send requests.post

result of python3.10

orange@32thread-server:~/project/pythonssleof$ python3.10 main.py
START send http.client.HTTPSConnection
EOF occurred in violation of protocol (_ssl.c:2426)
START send requests.post
HTTPSConnectionPool(host='google.com', port=443): Max retries exceeded with url: / (Caused by SSLError(SSLEOFError(8, 'EOF occurred in violation of protocol (_ssl.c:2426)')))

As you can see in the log, the existing OSError has been changed to SSLEOFError. This is a side effect caused by the change of the SSL_write() function to SSL_write_ex(), which changes the error handling behavior.

It is not wrong that SSLEOFError occurs in this behavior. It is a problem that all unexpected shutdowns occur as SSLEOFError. I think we should be able to provide as much information as possible to the user about these errors.

Agreed, the bugfix backport makes sense. Realize that 3.10 is in security fix only mode - the bug will have to remain there. The final bugfix release of 3.11 is coming up soon.

FWIW, SSLError is a child of OSError and people writing socket code naturally expect OSError for EOF/closed-connection, it'd be odd to see anyone explicitly catching and testing more specific SSLError for such a higher level trait.

Squashed commit of the following:

@keepworking

Apologies for not reviewing earlier, I agree with reverting the behavior back to raising an OSError instead of SSLEOFError and it does appear that this behavior change wasn't intended. Thanks for the PR and careful analysis.

For features developed in Python 3.10 or over, errors that occur during ssl communication should be handled through OSError, not SSLEOFError.

@keepworking It sounds like 3.10 will not receive a fix due to being security-only, so libraries will need to work-around the bug. But it's good to have it confirmed as a bug so exceptions can be rewritten appropriately.

It missed 3.11 too. I'm not confident enough about the fix to backport it to a final bugfix update (which would make reverting it quite painful). Everyone else wasn't either or didn't have time.

FWIW, for the backport to 3.12, I'm waiting for Fedora's testing, which should start this week with alpha 6. If another core dev is more confident or has a better way to estimate the fallout, feel free to backport now.

If exception handling is essential within a specific version range (3.10, 3.11 ~), it seems that backporting is not necessary. In libraries such as urllib3, it would be okay to deal with errors differently within a specific version range (3.10, 3.11 ~).

pythongh-115627: Fix ssl test_pha_required_nocert()

A concrete example of code broken by this change: test_ssl started failing since this change (see above), it's a regression.

Fixing the test is easy, but I warn you again against backporting such change. It can break code which works well currently, I don't think that it's worth it. I suggest to only change how the ssl module raises exceptions in Python 3.13.

I saw this random failure on a buildbot:

======================================================================
FAIL: test_pha_required_nocert (test.test_ssl.TestPostHandshakeAuth.test_pha_required_nocert)
----------------------------------------------------------------------
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-arm64-aws.macos-with-brew.refleak.nogil/build/Lib/test/test_ssl.py", line 4538, in test_pha_required_nocert
    with self.assertRaisesRegex(
    ...<11 lines>...
        s.recv(1024)
AssertionError: "certificate required|EOF occurred|closed by the remote host|Connection reset by peer" does not match "[Errno 32] Broken pipe"

By the way, I wrote PR gh-117821 to fix the test.

gh-115627: Fix ssl test_pha_required_nocert() (#117821)

@vstinner

Thank you for the correction. @vstinner
I apologize for the lack of my confirmation.

After my commit, there has been an exception handling for these two Broken Pipe errors.
#117821
#117484

Regarding the Broken Pipe error, the appropriate comment is in the urllib3 code, so I attach it.
https://github.com/urllib3/urllib3/blob/main/src/urllib3/connectionpool.py#L492-L518

        # conn.request() calls http.client.*.request, not the method in
        # urllib3.request. It also calls makefile (recv) on the socket.
        try:
            conn.request(
                method,
                url,
                body=body,
                headers=headers,
                chunked=chunked,
                preload_content=preload_content,
                decode_content=decode_content,
                enforce_content_length=enforce_content_length,
            )

        # We are swallowing BrokenPipeError (errno.EPIPE) since the server is
        # legitimately able to close the connection after sending a valid response.
        # With this behaviour, the received response is still readable.
        except BrokenPipeError:
            pass
        except OSError as e:
            # MacOS/Linux
            # EPROTOTYPE and ECONNRESET are needed on macOS
            # https://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/
            # Condition changed later to emit ECONNRESET instead of only EPROTOTYPE.
            if e.errno != errno.EPROTOTYPE and e.errno != errno.ECONNRESET:
                raise

Here, I was able to check the exception handling other than Broken Pipe, but I can check the part that is different from the exception handling in test_ssl at present.

Errno.EPIPE (broken pipe): This was added this time. Thank you again. This is consistent with the urllib3.
errno.ECONNRESET (Connection reset by peer): This is consistent with the urllib3.
Errno. EPROTOTYPE (Protocol long type for socket): This seems to be a special error that occurs in Apple environment. It's based on 2014 data, but I'm not sure if Apple corrected it. Depending on the results of the investigation, it seems necessary to put it in test_ssl.
errno 10054 (An existing connection was forcibly closed by the remote host.) : This has only been verified in test_ssl. This is a socket error in windows.
- https://learn.microsoft.com/en-us/windows/win32/winsock/windows-sockets-error-codes-2#:~:text=WSAECONNRESET,fail%20with%20WSAECONNRESET.
- In this document it says 'connection reset by peer'. It seems to operate under the same conditions as errno.ECONNRESET.

pythongh-115627: Fix PySSL_SetError handling SSL_ERROR_SYSCALL (pytho…

pythongh-115627: Fix ssl test_pha_required_nocert() (python#117821)

Fixing the test is easy, but I warn you again against backporting such change.

Based on that recommendation, I won't backport.

Noting here that with openssl 3.5 pushed into the Fedora and RHEL ecosystem we started seeing failures of test_pha_required_nocert and test_wrong_cert_tls13 on python3.10, 3.11 and 3.12 when stressing the system, resulting essentially in random flakiness.

Backporting this change made the failures go away.

00493: Fix test_pha_required_nocert() race condition

Navigation Menu

Repository navigation

`PySSL_SetError` needs to be updated #115627

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

5 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

PySSL_SetError needs to be updated #115627

Description

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Activity

encukou commented on Mar 26, 2024

vstinner commented on Mar 26, 2024

vstinner commented on Mar 26, 2024

serhiy-storchaka commented on Mar 26, 2024

keepworking commented on Mar 27, 2024

gpshead commented on Mar 27, 2024

5 remaining items

sethmlarson commented on Apr 6, 2024

encukou commented on Apr 8, 2024

keepworking commented on Apr 11, 2024

vstinner commented on Apr 12, 2024

keepworking commented on Apr 13, 2024

encukou commented on Apr 25, 2024

stratakis commented on May 9, 2025

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

`PySSL_SetError` needs to be updated #115627