Clear SSL error queue on shutdown

[joshuahunt]

SSL_shutdown can return an error and if it is not cleared it will be left on the thread's error queue since they are thread local causing unrelated connections running on the same thread to be incorrectly terminated. Clear the error queue now if SSL_shutdown returns an error to keep us from impacting other connections.

[mirostauder]

Automated message: PR pending admin approval for build testing

[joshuahunt]

This PR fixes a problem I reported in this issue: https://github.com/sysown/proxysql/issues/4556

[renecannao]

add to whitelist

[joshuahunt]

@renecannao @mirostauder it looks like a # of the tests are failing bc of a token issue. Is there something I need to do to fix this?

[joshuahunt]

The commit which added the SSL_shutdown call here is:

commit e8cc7be8fdd62a2188cda78934e300a4b06bc5c6
Author: Javier Jaramago Fernández <jaramago.fernandez.javier@gmail.com>
Date:   Wed Aug 18 23:29:05 2021 +0200

    Added non-blocking calls to 'SSL_shutdown' for sending final 'close_notify' required by SSL standard

diff --git a/lib/mysql_data_stream.cpp b/lib/mysql_data_stream.cpp
index 8eb58b61..1324e8d9 100644
--- a/lib/mysql_data_stream.cpp
+++ b/lib/mysql_data_stream.cpp
@@ -366,6 +366,14 @@ MySQL_Data_Stream::~MySQL_Data_Stream() {
        }
        if ( (myconn) && (myds_type==MYDS_FRONTEND) ) { delete myconn; myconn=NULL; }
        if (encrypted) {
+               if (ssl) {
+                       // NOTE: SSL standard requires a final 'close_notify' alert on socket
+                       // shutdown. But for avoiding any kind of locking IO waiting for the
+                       // other part, we perform a 'quiet' shutdown. For more context see
+                       // MYSQL #29579.
+                       SSL_set_quiet_shutdown(ssl, 1);
+                       SSL_shutdown(ssl);
+               }
                if (ssl) SSL_free(ssl);
 /*
                SSL_free() should also take care of these
@@ -445,7 +453,12 @@ void MySQL_Data_Stream::shut_hard() {
        proxy_debug(PROXY_DEBUG_NET, 4, "Shutdown hard fd=%d. Session=%p, DataStream=%p\n", fd, sess, this);
        set_net_failure();
        if (encrypted) {
+               // NOTE: SSL standard requires a final 'close_notify' alert on socket
+               // shutdown. But for avoiding any kind of locking IO waiting for the
+               // other part, we perform a 'quiet' shutdown. For more context see
+               // MYSQL #29579.
                SSL_set_quiet_shutdown(ssl, 1);
+               SSL_shutdown(ssl);
        }
        if (fd >= 0) {
                shutdown(fd, SHUT_RDWR);

@JavierJF could you please take a look at my change?

[JavierJF]

Hi @joshuahunt,

thanks for the clear and detailed report and for the patch. There is nothing extra for you to do. Patch looks good to me, since datastreams destruction/shutdown will always result in error cleanups, which sounds like a sensible thing to do anyway due to how OpenSSL errors work. I'm proceeding with the merge, and I will also close the associated issue.

Thank again for the contribution!

Regards, Javier.

[brett-ehlen]

@JavierJF What ProxySQL patch will this be available in? When will this be released? We are seeing similar errors in ProxySQL 2.5.5. Found similar issues here:

https://github.com/sysown/proxysql/issues/4556

Thanks!

[brett-ehlen]

@JavierJF Also possibly here?

https://github.com/sysown/proxysql/issues/4500

[JavierJF]

Hi @brett-ehlen,

the case solved by this PR is only the one present in the (very complete) report that @joshuahunt sent in #4556. Regarding #4500, I'm going to elaborate in a comment there, because the issues presented there could be more related to #4537. User provided data so far points that way, yet, we haven't being able to verify that. This last fix ( #4537) is already available in the latest release. We don't comment on release times, but we are doing short release cycles for minor, bug fixing releases.

Thanks, Javier.