From 67e62627240b3f925c765cbda63ccbec328fed29 Mon Sep 17 00:00:00 2001
From: Matthew Fredrickson <mfredrickson@fluentstream.com>
Date: Tue, 30 May 2023 04:33:05 -0500
Subject: [PATCH 302/303] Locking fix so that SSL_shutdown and SSL_write are
 not called at same time (#3583)

---
 pjlib/src/pj/ssl_sock_ossl.c | 82 ++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/pjlib/src/pj/ssl_sock_ossl.c b/pjlib/src/pj/ssl_sock_ossl.c
index 56841f80a..5c68edb2b 100644
--- a/pjlib/src/pj/ssl_sock_ossl.c
+++ b/pjlib/src/pj/ssl_sock_ossl.c
@@ -1230,44 +1230,58 @@ static void ssl_destroy(pj_ssl_sock_t *ssock)
     /* Potentially shutdown OpenSSL library if this is the last
      * context exists.
      */
     shutdown_openssl();
 }
 
 
 /* Reset SSL socket state */
 static void ssl_reset_sock_state(pj_ssl_sock_t *ssock)
 {
+    int post_unlock_flush_circ_buf = 0;
+
     ossl_sock_t *ossock = (ossl_sock_t *)ssock;
 
+    /* Must lock around SSL calls, particularly SSL_shutdown
+     * as it can modify the write BIOs and destructively
+     * interfere with any ssl_write() calls in progress
+     * above in a multithreaded environment */
+    pj_lock_acquire(ssock->write_mutex);
+
     /* Detach from SSL instance */
     if (ossock->ossl_ssl) {
 	SSL_set_ex_data(ossock->ossl_ssl, sslsock_idx, NULL);
     }
 
     /**
      * 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.
      */
     if (ossock->ossl_ssl && SSL_in_init(ossock->ossl_ssl) == 0) {
-	int ret = SSL_shutdown(ossock->ossl_ssl);
-	if (ret == 0) {
-	    /* Flush data to send close notify. */
-	    flush_circ_buf_output(ssock, &ssock->shutdown_op_key, 0, 0);
-	}
+        int ret = SSL_shutdown(ossock->ossl_ssl);
+        if (ret == 0) {
+            /* SSL_shutdown will potentially trigger a bunch of
+             * data to dump to the socket */
+            post_unlock_flush_circ_buf = 1;
+        }
     }
 
-    pj_lock_acquire(ssock->write_mutex);
     ssock->ssl_state = SSL_STATE_NULL;
+
     pj_lock_release(ssock->write_mutex);
 
+    if (post_unlock_flush_circ_buf) {
+        /* Flush data to send close notify. */
+        flush_circ_buf_output(ssock, &ssock->shutdown_op_key, 0, 0);
+    }
+
     ssl_close_sockets(ssock);
 
     /* Upon error, OpenSSL may leave any error description in the thread 
      * error queue, which sometime may cause next call to SSL API returning
      * false error alarm, e.g: in Linux, SSL_CTX_use_certificate_chain_file()
      * returning false error after a handshake error (in different SSL_CTX!).
      * For now, just clear thread error queue here.
      */
     ERR_clear_error();
 }
@@ -1855,52 +1869,58 @@ static pj_status_t ssl_read(pj_ssl_sock_t *ssock, void *data, int *size)
 {
     ossl_sock_t *ossock = (ossl_sock_t *)ssock;
     int size_ = *size;
     int len = size_;
 
     /* SSL_read() may write some data to write buffer when re-negotiation
      * is on progress, so let's protect it with write mutex.
      */
     pj_lock_acquire(ssock->write_mutex);
     *size = size_ = SSL_read(ossock->ossl_ssl, data, size_);
-    pj_lock_release(ssock->write_mutex);
 
     if (size_ <= 0) {
 	pj_status_t status;
 	int err = SSL_get_error(ossock->ossl_ssl, size_);
 
-	/* SSL might just return SSL_ERROR_WANT_READ in 
-	 * re-negotiation.
-	 */
-	if (err != SSL_ERROR_NONE && err != SSL_ERROR_WANT_READ) {
-	    if (err == SSL_ERROR_SYSCALL && size_ == -1 &&
-		ERR_peek_error() == 0 && errno == 0)
-	    {
-		status = STATUS_FROM_SSL_ERR2("Read", ssock, size_,
-					      err, len);
-		PJ_LOG(4,("SSL", "SSL_read() = -1, with "
-				 "SSL_ERROR_SYSCALL, no SSL error, "
-				 "and errno = 0 - skip BIO error"));
-		/* Ignore these errors */
-	    } else {
-		/* Reset SSL socket state, then return PJ_FALSE */
-		status = STATUS_FROM_SSL_ERR2("Read", ssock, size_,
-		        		      err, len);
-		ssl_reset_sock_state(ssock);
-		return status;
-	    }
-	}
-	
-	/* Need renegotiation */
-	return PJ_EEOF;
+        /* SSL might just return SSL_ERROR_WANT_READ in 
+         * re-negotiation.
+         */
+        if (err != SSL_ERROR_NONE && err != SSL_ERROR_WANT_READ) {
+            if (err == SSL_ERROR_SYSCALL && size_ == -1 &&
+                ERR_peek_error() == 0 && errno == 0)
+            {
+                status = STATUS_FROM_SSL_ERR2("Read", ssock, size_,
+                                              err, len);
+                PJ_LOG(4,("SSL", "SSL_read() = -1, with "
+                                 "SSL_ERROR_SYSCALL, no SSL error, "
+                                 "and errno = 0 - skip BIO error"));
+                /* Ignore these errors */
+            } else {
+                /* Reset SSL socket state, then return PJ_FALSE */
+                status = STATUS_FROM_SSL_ERR2("Read", ssock, size_,
+                                              err, len);
+                pj_lock_release(ssock->write_mutex);
+                /* Unfortunately we can't hold the lock here to reset all the state.
+                  * We probably should though.
+                  */
+                ssl_reset_sock_state(ssock);
+                return status;
+            }
+        }
+        
+        pj_lock_release(ssock->write_mutex);
+        /* Need renegotiation */
+        return PJ_EEOF;
     }
 
+    pj_lock_release(ssock->write_mutex);
+
     return PJ_SUCCESS;
 }
 
 
 /* Write plain data to SSL and flush write BIO. */
 static pj_status_t ssl_write(pj_ssl_sock_t *ssock, const void *data,
 			     pj_ssize_t size, int *nwritten)
 {
     ossl_sock_t *ossock = (ossl_sock_t *)ssock;
     pj_status_t status = PJ_SUCCESS;
-- 
2.41.0