Skip to content

Commit

Permalink
OpenSSL timeout fix:
Browse files Browse the repository at this point in the history 
Since timeout was already set via setsockopt, we call wait_io_or_timeout()
with a very small timeout (5ms) to get a more precise errno, which is used
by OpenSSL's error function.
  • Loading branch information
@9EOR9
9EOR9 committed Sep 17, 2024
1 parent 2804ed9 commit a6fd09f
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 4 deletions.
14 changes: 10 additions & 4 deletions libmariadb/secure/openssl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -567,11 +567,13 @@ my_bool ma_tls_connect(MARIADB_TLS *ctls)
{
switch((SSL_get_error(ssl, rc))) {
case SSL_ERROR_WANT_READ:
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, mysql->options.connect_timeout) < 1)
/* use low timeout, see ma_tls_read */
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, 5) < 1)

This comment has been minimized.

Sign in to view

Copy link
@azat

azat Oct 10, 2024

Contributor

Shouldn't gnutls has the same code? I.e. something like this

diff --git a/libmariadb/secure/gnutls.c b/libmariadb/secure/gnutls.c
index 9bce65d3..9bc1aaa1 100644
--- a/libmariadb/secure/gnutls.c
+++ b/libmariadb/secure/gnutls.c
@@ -1365,7 +1365,7 @@ ssize_t ma_tls_read(MARIADB_TLS *ctls, const uchar* buffer, size_t length)
   {
     if (rc != GNUTLS_E_AGAIN && rc != GNUTLS_E_INTERRUPTED)
       break;
-    if (pvio->methods->wait_io_or_timeout(pvio, TRUE, pvio->mysql->options.read_timeout) < 1)
+    if (pvio->methods->wait_io_or_timeout(pvio, TRUE, 5) < 1)
       break;
   }
   if (rc <= 0) {
@@ -1384,7 +1384,7 @@ ssize_t ma_tls_write(MARIADB_TLS *ctls, const uchar* buffer, size_t length)
   {
     if (rc != GNUTLS_E_AGAIN && rc != GNUTLS_E_INTERRUPTED)
       break;
-    if (pvio->methods->wait_io_or_timeout(pvio, TRUE, pvio->mysql->options.write_timeout) < 1)
+    if (pvio->methods->wait_io_or_timeout(pvio, TRUE, 5) < 1)
       break;
   }
   if (rc <= 0) {

This comment has been minimized.

Sign in to view

Copy link
@azat

azat Oct 10, 2024

Contributor

Also AFAICS wait_io_or_timeout does not rely on socket timeout it uses poll/select, so you may get smaller timeout, or maybe I'm missing something?
try_connect= 0;
break;
case SSL_ERROR_WANT_WRITE:
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, mysql->options.connect_timeout) < 1)
/* use low timeout, see ma_tls_read */
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, 5) < 1)

This comment has been minimized.

Sign in to view

Copy link
@azat

azat Oct 10, 2024

Contributor

Also wait_io_or_timeout the second argument is_read, so it should be FALSE for writes (here and below)

This comment has been minimized.

Sign in to view

Copy link
@azat

azat Oct 10, 2024

Contributor

See #249 for this
try_connect= 0;
break;
default:
Expand Down Expand Up @@ -655,7 +657,10 @@ ssize_t ma_tls_read(MARIADB_TLS *ctls, const uchar* buffer, size_t length)
int error= SSL_get_error((SSL *)ctls->ssl, rc);
if (error != SSL_ERROR_WANT_READ)
break;
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, pvio->mysql->options.read_timeout) < 1)
/* To get a more precise error message than "resource temporary
unavailable" (=errno 11) after read timeout occured, we check
the socket status using a very small timeout (=5 ms) */
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, 5) < 1)
break;
}
if (rc <= 0)
Expand All @@ -676,7 +681,8 @@ ssize_t ma_tls_write(MARIADB_TLS *ctls, const uchar* buffer, size_t length)
int error= SSL_get_error((SSL *)ctls->ssl, rc);
if (error != SSL_ERROR_WANT_WRITE)
break;
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, pvio->mysql->options.write_timeout) < 1)
/* use low timeout, see ma_tls_read */
if (pvio->methods->wait_io_or_timeout(pvio, TRUE, 5) < 1)
break;
}
if (rc <= 0)
Expand Down
35 changes: 35 additions & 0 deletions unittest/libmariadb/connection.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2374,7 +2374,41 @@ static int test_parsec(MYSQL *my)
return OK;
}

/* Test for PR250 */
int test_tls_timeout(MYSQL *unused __attribute__((unused)))
{
unsigned int connect_timeout= 5;
unsigned int read_write_timeout= 10;
int rc;
time_t start, elapsed;

MYSQL *mysql= mysql_init(NULL);
mysql_ssl_set(mysql, NULL, NULL, NULL, NULL, NULL);
mysql_options(mysql, MYSQL_OPT_CONNECT_TIMEOUT, (unsigned int *)&connect_timeout);
mysql_options(mysql, MYSQL_OPT_READ_TIMEOUT, (unsigned int *)&read_write_timeout);
mysql_options(mysql, MYSQL_OPT_WRITE_TIMEOUT, (unsigned int *)&read_write_timeout);
if (!my_test_connect(mysql, hostname, username, password, schema, port, socketname, CLIENT_REMEMBER_OPTIONS, 1))
{
mysql_close(mysql);
return FAIL;
}

start= time(NULL);
rc= mysql_query(mysql, "SET @a:=SLEEP(12)");
elapsed= time(NULL) - start;
diag("elapsed: %lu", (unsigned long)elapsed);
FAIL_IF((unsigned int)elapsed > read_write_timeout + 1, "timeout ignored");

FAIL_IF(!rc, "expected timeout error");
diag("Error: %s", mysql_error(mysql));

mysql_close(mysql);
return OK;
}


struct my_tests_st my_tests[] = {
{"test_tls_timeout", test_tls_timeout, TEST_CONNECTION_NONE, 0, NULL, NULL},
{"test_parsec", test_parsec, TEST_CONNECTION_DEFAULT, 0, NULL, NULL},
{"test_conc505", test_conc505, TEST_CONNECTION_NONE, 0, NULL, NULL},
{"test_conc632", test_conc632, TEST_CONNECTION_NONE, 0, NULL, NULL},
Expand Down Expand Up @@ -2423,6 +2457,7 @@ struct my_tests_st my_tests[] = {
{"test_connection_timeout", test_connection_timeout, TEST_CONNECTION_NONE, 0, NULL, NULL},
{"test_connection_timeout2", test_connection_timeout2, TEST_CONNECTION_NONE, 0, NULL, NULL},
{"test_connection_timeout3", test_connection_timeout3, TEST_CONNECTION_NONE, 0, NULL, NULL},
{"test_tls_timeout", test_tls_timeout, TEST_CONNECTION_NONE, 0, NULL, NULL},

This comment has been minimized.

Sign in to view

Copy link
@azat

azat Oct 10, 2024

Contributor

Duplicated test?
{NULL, NULL, 0, 0, NULL, NULL}
};

Expand Down

0 comments on commit a6fd09f

Please sign in to comment.