gh-123954: proposal for improving logic for setting SSL exceptions

[picnixz]

I'd recommend also reviewing each commit separately due to the changes arising from logic extraction. I've also observed that we incorrectly use ERR_GET_REASON(p) on a custom error code. It's not really an issue because this is a no-op but I've added an assertion and removed the call for the semantics to match (we format the message using the error coming from ERR_peek_last_error() but the exception object is initialized with another error code that can also be obtained by SSL_get_error() or is a custom one).

I haven't benchmarked the branch, I just want to share a proposal. Note that refactoring the logic could help target the operations that take longer time.

cc @tarasko@kumaraditya303@gpshead

• Issue: Inefficient ssl.SSLWantReadError exception slows down very common use-case #123954

[picnixz]

Since all strings that will be rendered in the exception are actually ASCII messages (no localization), I'm considering switching from a PyDict storage to a _Py_hashtable_t structure but I don't know which one is the most efficient. With a _Py_hashtable_t structure, I can simply forget about Unicode PyObject * objects and deal with const char * objects only.

I haven't checked more in details though since I already want to know whether the proposal performs well.

[ZeroIntensity]

_Py_hashtable_t will probably perform better, but it's not thread safe: is it possible for concurrent writes to be an issue here?

[picnixz]

No, we're only reading from the hash table. It is cached in the module's state and we only lookup strings by integer keys or pairs of integers.

[ZeroIntensity]

OpenSSL has fixed the issue upstream, but it looks like the release isn't due until April.

[picnixz]

Oups I misclicked on my mobile app. It's still a draft.

[ZeroIntensity]

Converted it back for you. IIRC the GitHub app doesn't let you mark something as a draft.

[picnixz]

Btw, I think we also have another issue with another code:

:1058: ImportWarning: SSL data contains incompatible entries for (41, 104; 0x14800068, 343933032). Old mnemonic is BROTLI_INFLATE_ERROR but new mnemonic is BROTLI_NOT_SUPPORTED.

I didn't check whether this is still the case, but it's probably worth to check as well.

[ZeroIntensity]

Frustratingly, looks like OpenSSL again: https://github.com/openssl/openssl/blob/b049ce0e354011be075e620b9ba7cf4d7c8f9577/crypto/err/openssl.txt#L414 and https://github.com/openssl/openssl/blob/b049ce0e354011be075e620b9ba7cf4d7c8f9577/include/openssl/comperr.h#L28

I guess I'll go pester them again. Are there any other cases like this, or is this the last one?

[picnixz]

I guess I'll go pester them again. Are there any other cases like this, or is this the last one?

I think it's the last one. At least, I don't see more warnings in the CI.

[ZeroIntensity]

I found a few others while reporting the issue, FWIW.

• Mismatch between several other macros in openssl.txt and comperr.h openssl/openssl#26388

[picnixz]

I found a few others while reporting the issue,

Thanks! I think we don't use those, hence I didn't have the warning (or maybe we're using it and something else is wrong in my warnings being emitted?)

[picnixz]

I'm going to abandon that specific PR. I may at some point refactor SSL but it's more pressing to fix other issues than this specific one (it didn't give much improvement and didn't really fix the issue users have).