[sr-dev] git:andrei/tcp_tls_changes: tls: fix initial state error handling
Andrei Pelinescu-Onciul
andrei at iptel.org
Sat Jun 19 17:13:29 CEST 2010
Module: sip-router
Branch: andrei/tcp_tls_changes
Commit: 42b8ccc2b8a68a874c5b89cfa45d470c364b6bdc
URL: http://git.sip-router.org/cgi-bin/gitweb.cgi/sip-router/?a=commit;h=42b8ccc2b8a68a874c5b89cfa45d470c364b6bdc
Author: Andrei Pelinescu-Onciul <andrei at iptel.org>
Committer: Andrei Pelinescu-Onciul <andrei at iptel.org>
Date: Sat Jun 19 17:03:45 2010 +0200
tls: fix initial state error handling
tls_connect() and tls_accept() non openssl related errors
(*error == SSL_ERROR_NONE) were treated as "normal" openssl
errors. This could result in silently dropped packets in low
memory conditions, instead of dropping the whole underlying tcp
connection.
---
modules/tls/tls_server.c | 68 ++++++++++++++++++++++++++++++++++-----------
1 files changed, 51 insertions(+), 17 deletions(-)
diff --git a/modules/tls/tls_server.c b/modules/tls/tls_server.c
index be661de..121a016 100644
--- a/modules/tls/tls_server.c
+++ b/modules/tls/tls_server.c
@@ -226,9 +226,13 @@ static void tls_dump_cert_info(char* s, X509* cert)
* @param c - tcp connection with tls (extra_data must be a filled
* tcp_extra_data structure). The state must be S_TLS_ACCEPTING.
* @param error set to the error reason (SSL_ERROR_*).
+ * Note that it can be SSL_ERROR_NONE while the return is < 0
+ * ("internal" error, not at the SSL level, see below).
* @return >=1 on success, 0 and <0 on error. 0 means the underlying SSL
- * connection was closed/shutdown. < 0 is also returned for
- * WANT_READ or WANT_WRITE
+ * connection was closed/shutdown. < 0 is returned for any
+ * SSL_ERROR (including WANT_READ or WANT_WRITE), but also
+ * for internal non SSL related errors (in this case -2 is
+ * returned and error==SSL_ERROR_NONE).
*
*/
int tls_accept(struct tcp_connection *c, int* error)
@@ -239,7 +243,8 @@ int tls_accept(struct tcp_connection *c, int* error)
struct tls_extra_data* tls_c;
int tls_log;
- if (LOW_MEM_NEW_CONNECTION_TEST()){
+ *error = SSL_ERROR_NONE;
+ if (unlikely(LOW_MEM_NEW_CONNECTION_TEST())){
ERR("tls: ssl bug #1491 workaround: not enough memory for safe"
" operation: %lu\n", shm_available());
goto err;
@@ -250,7 +255,6 @@ int tls_accept(struct tcp_connection *c, int* error)
if (unlikely(tls_c->state != S_TLS_ACCEPTING)) {
BUG("Invalid connection state %d (bug in TLS code)\n", tls_c->state);
- /* Not critical */
goto err;
}
ret = SSL_accept(ssl);
@@ -283,18 +287,23 @@ int tls_accept(struct tcp_connection *c, int* error)
}
return ret;
err:
- return -1;
+ /* internal non openssl related errors */
+ return -2;
}
-/** wrapper around SSL_connect, usin SSL return convention.
+/** wrapper around SSL_connect, using SSL return convention.
* It will also log critical errors and certificate debugging info.
* @param c - tcp connection with tls (extra_data must be a filled
* tcp_extra_data structure). The state must be S_TLS_CONNECTING.
* @param error set to the error reason (SSL_ERROR_*).
+ * Note that it can be SSL_ERROR_NONE while the return is < 0
+ * ("internal" error, not at the SSL level, see below).
* @return >=1 on success, 0 and <0 on error. 0 means the underlying SSL
- * connection was closed/shutdown. < 0 is also returned for
- * WANT_READ or WANT_WRITE
+ * connection was closed/shutdown. < 0 is returned for any
+ * SSL_ERROR (including WANT_READ or WANT_WRITE), but also
+ * for internal non SSL related errors (in this case -2 is
+ * returned and error==SSL_ERROR_NONE).
*
*/
int tls_connect(struct tcp_connection *c, int* error)
@@ -305,7 +314,8 @@ int tls_connect(struct tcp_connection *c, int* error)
struct tls_extra_data* tls_c;
int tls_log;
- if (LOW_MEM_NEW_CONNECTION_TEST()){
+ *error = SSL_ERROR_NONE;
+ if (unlikely(LOW_MEM_NEW_CONNECTION_TEST())){
ERR("tls: ssl bug #1491 workaround: not enough memory for safe"
" operation: %lu\n", shm_available());
goto err;
@@ -314,10 +324,8 @@ int tls_connect(struct tcp_connection *c, int* error)
tls_c=(struct tls_extra_data*)c->extra_data;
ssl=tls_c->ssl;
- *error = SSL_ERROR_NONE;
if (unlikely(tls_c->state != S_TLS_CONNECTING)) {
BUG("Invalid connection state %d (bug in TLS code)\n", tls_c->state);
- /* Not critical */
goto err;
}
ret = SSL_connect(ssl);
@@ -352,7 +360,8 @@ int tls_connect(struct tcp_connection *c, int* error)
}
return ret;
err:
- return -1;
+ /* internal non openssl related errors */
+ return -2;
}
@@ -598,16 +607,24 @@ redo_wr:
n = SSL_write(ssl, buf + offs, len - offs);
if (unlikely(n <= 0))
ssl_error = SSL_get_error(ssl, n);
- } else
+ } else {
+ /* tls_connect failed/needs more IO */
+ if (unlikely(n < 0 && ssl_error == SSL_ERROR_NONE))
+ goto error;
err_src = "TLS connect:";
+ }
} else if (unlikely(tls_c->state == S_TLS_ACCEPTING)) {
n = tls_accept(c, &ssl_error);
if (unlikely(n>=1)) {
n = SSL_write(ssl, buf + offs, len - offs);
if (unlikely(n <= 0))
ssl_error = SSL_get_error(ssl, n);
- } else
+ } else {
+ /* tls_accept failed/needs more IO */
+ if (unlikely(n < 0 && ssl_error == SSL_ERROR_NONE))
+ goto error;
err_src = "TLS accept:";
+ }
} else {
n = SSL_write(ssl, buf + offs, len - offs);
if (unlikely(n <= 0))
@@ -630,6 +647,8 @@ redo_wr:
if (unlikely(n <= 0)){
switch(ssl_error) {
case SSL_ERROR_NONE:
+ BUG("unexpected SSL_ERROR_NONE for n=%d\n", n);
+ goto error;
break;
case SSL_ERROR_ZERO_RETURN:
/* SSL EOF */
@@ -908,6 +927,11 @@ redo_read:
if (unlikely(n>=1)) {
n = SSL_read(ssl, r->pos, bytes_free);
} else {
+ /* tls_connect failed/needs more IO */
+ if (unlikely(n < 0 && ssl_error == SSL_ERROR_NONE)) {
+ lock_release(&c->write_lock);
+ goto error;
+ }
err_src = "TLS connect:";
goto ssl_read_skipped;
}
@@ -916,6 +940,11 @@ redo_read:
if (unlikely(n>=1)) {
n = SSL_read(ssl, r->pos, bytes_free);
} else {
+ /* tls_accept failed/needs more IO */
+ if (unlikely(n < 0 && ssl_error == SSL_ERROR_NONE)) {
+ lock_release(&c->write_lock);
+ goto error;
+ }
err_src = "TLS accept:";
goto ssl_read_skipped;
}
@@ -954,6 +983,10 @@ ssl_read_skipped:
lock_release(&c->write_lock);
switch(ssl_error) {
case SSL_ERROR_NONE:
+ if (unlikely(n < 0)) {
+ BUG("unexpected SSL_ERROR_NONE for n=%d\n", n);
+ goto error;
+ }
break;
case SSL_ERROR_ZERO_RETURN:
/* SSL EOF */
@@ -967,7 +1000,8 @@ ssl_read_skipped:
if (unlikely(rd.pos != rd.used)) {
/* data still in the read buffer */
BUG("SSL_ERROR_WANT_READ but data still in"
- " the rbio (%d bytes)\n", rd.used - rd.pos);
+ " the rbio (%p, %d bytes at %d)\n", rd.buf,
+ rd.used - rd.pos, rd.pos);
goto bug;
}
if (unlikely((*flags & (RD_CONN_EOF | RD_CONN_SHORT_READ)) == 0))
@@ -1029,9 +1063,9 @@ ssl_read_skipped:
BUG("unexpected value (n = %d)\n", n);
else {
if (unlikely(n < bytes_free))
- BUG("read buffer not exhausted (rbio still has %d bytes,"
+ BUG("read buffer not exhausted (rbio %p still has %d bytes,"
"last SSL_read %d / %d)\n",
- rd.used - rd.pos, n, bytes_free);
+ rd.buf, rd.used - rd.pos, n, bytes_free);
/* n <= bytes_free */
/* queue read data if not fully consumed by SSL_read()
* (very unlikely situation)
More information about the sr-dev
mailing list