From b4429307899afd637ae33e8e6a1400f30b57085e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 11 Dec 2012 17:46:12 -0500 Subject: [PATCH 1/2] Fix serious breakage in connection_handle_write_impl When we first implemented TLS, we assumed in conneciton_handle_write that a TOR_TLS_WANT_WRITE from flush_buf_tls meant that nothing had been written. But when we moved our buffers to a ring buffer implementation back in 0.1.0.5-rc (!), we broke that invariant: it's possible that some bytes have been written but nothing. That's bad. It means that if we do a sequence of TLS writes that ends with a WANTWRITE, we don't notice that we flushed any bytes, and we don't (I think) decrement buckets. Fixes bug 7708; bugfix on 0.1.0.5-rc --- changes/bug7708 | 5 +++++ src/or/connection.c | 15 +++++++++++++-- 2 files changed, 18 insertions(+), 2 deletions(-) create mode 100644 changes/bug7708 diff --git a/changes/bug7708 b/changes/bug7708 new file mode 100644 index 0000000000..e272adf227 --- /dev/null +++ b/changes/bug7708 @@ -0,0 +1,5 @@ + o Major bugfixes: + - When a TLS write is partially successful but incomplete, remember + that the flushed part has been flushed, and notice that bytes were + actually written. Reported and fixed pseudonymously. Fixes bug + 7708; bugfix on Tor 0.1.0.5-rc. diff --git a/src/or/connection.c b/src/or/connection.c index eac9c4f32b..3e6341b69f 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -3168,6 +3168,7 @@ connection_handle_write_impl(connection_t *conn, int force) ssize_t max_to_write; time_t now = approx_time(); size_t n_read = 0, n_written = 0; + int dont_stop_writing = 0; tor_assert(!connection_is_listener(conn)); @@ -3220,6 +3221,7 @@ connection_handle_write_impl(connection_t *conn, int force) if (connection_speaks_cells(conn) && conn->state > OR_CONN_STATE_PROXY_HANDSHAKING) { or_connection_t *or_conn = TO_OR_CONN(conn); + size_t initial_size; if (conn->state == OR_CONN_STATE_TLS_HANDSHAKING || conn->state == OR_CONN_STATE_TLS_CLIENT_RENEGOTIATING) { connection_stop_writing(conn); @@ -3235,6 +3237,7 @@ connection_handle_write_impl(connection_t *conn, int force) } /* else open, or closing */ + initial_size = buf_datalen(conn->outbuf); result = flush_buf_tls(or_conn->tls, conn->outbuf, max_to_write, &conn->outbuf_flushlen); @@ -3257,7 +3260,8 @@ connection_handle_write_impl(connection_t *conn, int force) case TOR_TLS_WANTWRITE: log_debug(LD_NET,"wanted write."); /* we're already writing */ - return 0; + dont_stop_writing = 1; + break; case TOR_TLS_WANTREAD: /* Make sure to avoid a loop if the receive buckets are empty. */ log_debug(LD_NET,"wanted read."); @@ -3279,6 +3283,12 @@ connection_handle_write_impl(connection_t *conn, int force) tor_tls_get_n_raw_bytes(or_conn->tls, &n_read, &n_written); log_debug(LD_GENERAL, "After TLS write of %d: %ld read, %ld written", result, (long)n_read, (long)n_written); + /* So we notice bytes were written even on error */ + /* XXXX024 This cast is safe since we can never write INT_MAX bytes in a + * single set of TLS operations. But it looks kinda ugly. If we refactor + * the *_buf_tls functions, we should make them return ssize_t or size_t + * or something. */ + result = (int)(initial_size-buf_datalen(conn->outbuf)); } else { CONN_LOG_PROTECT(conn, result = flush_buf(conn->s, conn->outbuf, @@ -3313,7 +3323,8 @@ connection_handle_write_impl(connection_t *conn, int force) connection_mark_for_close(conn); } - if (!connection_wants_to_flush(conn)) { /* it's done flushing */ + if (!connection_wants_to_flush(conn) && + !dont_stop_writing) { /* it's done flushing */ if (connection_finished_flushing(conn) < 0) { /* already marked */ return -1; From 690ea9e8cf3b9c047484a6aea30fa94650fb66c8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 30 Jan 2013 17:07:32 -0500 Subject: [PATCH 2/2] Clarify documentation of connection_finished_flushing --- src/or/connection.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 3e6341b69f..10fee1248b 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -3872,8 +3872,9 @@ connection_flushed_some(connection_t *conn) return r; } -/** We just finished flushing bytes from conn-\>outbuf, and there - * are no more bytes remaining. +/** We just finished flushing bytes to the appropriately low network layer, + * and there are no more bytes remaining in conn-\>outbuf, conn-\>bev, or + * conn-\>tls to be flushed. * * This function just passes conn to the connection-specific * connection_*_finished_flushing() function.