Don't call SSL_shutdown() when receiving SSL_ERROR_SYSCALL or SSL_ERROR_SSL#3577
Don't call SSL_shutdown() when receiving SSL_ERROR_SYSCALL or SSL_ERROR_SSL#3577
Conversation
pjlib/src/pj/ssl_sock_ossl.c
Outdated
|
|
||
| static void ssl_reset_sock_state_with_error(pj_ssl_sock_t* ssock, pj_bool_t check_error) | ||
| { | ||
| ossl_sock_t* ossock = (ossl_sock_t*)ssock; |
There was a problem hiding this comment.
Minor: our PointerAlignment is actually Right, i.e. see the initial code:
ossl_sock_t *ossock = (ossl_sock_t *)ssock
pjlib/src/pj/ssl_sock_ossl.c
Outdated
| * Avoid calling SSL_shutdown() if handshake wasn't completed. | ||
| * OpenSSL 1.0.2f complains if SSL_shutdown() is called during an | ||
| * SSL handshake, while previous versions always return 0. | ||
| * Don't send notify when the last error is SSL_ERROR_SYSCALL or SSL_ERROR_SSL. |
There was a problem hiding this comment.
Replace send notify with call SSL_shutdown().
Yes, SSL_shutdown() will send notify, but it's only one of the steps.
`Note that SSL_shutdown() must not be called if a previous fatal error has occurred on a connection i.e. if SSL_get_error() has returned SSL_ERROR_SYSCALL or SSL_ERROR_SSL.
The shutdown procedure consists of two steps: sending of the close_notify shutdown alert, and reception of the peer's close_notify shutdown alert. The order of those two steps depends on the application.`
https://www.openssl.org/docs/manmaster/man3/SSL_shutdown.html
Also change the boolean below.
- change pointer alignment - change comments and variable name
|
The only concern I have is that last_err might be set from an ssl_write context while ssl_read is being executed (due to concerns brought up in #3575). I just pushed my patch up for it, so we'll probably have a merge conflict to deal with whenever your branch or mine goes in, but in any case, I think the locking in the fix for #3575 will make things a bit better. Oh, we also need to do this check on the results from the SSL_read() in pjlib/src/pj/ssl_sock_ossl.c, anywhere else libssl might return these errors. |
|
Since #3583 has been integrated, you can merge with the latest master to resolve the conflict. |
creslin2877
left a comment
creslin2877
left a comment
There was a problem hiding this comment.
As mentioned in my previous comment, we need to cover the case where we get an SSL_ERROR_SSL from the SSL_read() function too.
Since, after merging, it now already uses the locking in |
|
@creslin2877, let us know if there's any issue with the latest patch, so we can merge it for 2.13.1. |
|
I just realised that the addition of |
|
I think you've covered the case that triggered my concern. Thanks! |
…OR_SSL (pjsip#3577) (cherry picked from commit 806b7c2)
4 participants
This is to fix #3576.
Before calling
SSL_shutdown(), a check to SSL_ERROR_SYSCALL and SSL_ERROR_SSL is required.