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]

Looks like an upstream issue. (I'll file an issue and CC you.) There's two pieces of relevant code on OpenSSL's end. Here:
https://github.com/openssl/openssl/blob/817a2b2b4955da0233fe7e6e4bd16c0255262b4f/crypto/err/openssl.txt#L409 and here: https://github.com/openssl/openssl/blob/817a2b2b4955da0233fe7e6e4bd16c0255262b4f/include/openssl/comperr.h#L27

[ZeroIntensity]

• Mismatch between COMP_R_BROTLI_DEFLATE_ERROR and COMP_R_BROTLI_ENCODE_ERROR openssl/openssl#26316

[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?)