March 2016 - tor-commits - lists.torproject.org

[tor/master] Add new channel/queue_incoming unit tests; modify channel unit tests for new clarified handling of alloc/free responsibility for queued incoming cells
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 1cdc7fddb2ed10d72f4e65e15d1af4d803a1acdb Author: Andrea Shepard <andrea(a)torproject.org> Date: Tue Mar 15 03:28:04 2016 +0000 Add new channel/queue_incoming unit tests; modify channel unit tests for new clarified handling of alloc/free responsibility for queued incoming cells --- src/test/test_channel.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 116 insertions(+), 5 deletions(-) diff --git a/src/test/test_channel.c b/src/test/test_channel.c index e11ac3f..b705ee5 100644 --- a/src/test/test_channel.c +++ b/src/test/test_channel.c @@ -25,7 +25,9 @@ extern uint64_t estimated_total_queue_size; static int test_chan_accept_cells = 0; static int test_chan_fixed_cells_recved = 0; +static cell_t * test_chan_last_seen_fixed_cell_ptr = NULL; static int test_chan_var_cells_recved = 0; +static var_cell_t * test_chan_last_seen_var_cell_ptr = NULL; static int test_cells_written = 0; static int test_destroy_not_pending_calls = 0; static int test_doesnt_want_writes_count = 0; @@ -70,6 +72,7 @@ static void test_channel_flushmux(void *arg); static void test_channel_incoming(void *arg); static void test_channel_lifecycle(void *arg); static void test_channel_multi(void *arg); +static void test_channel_queue_incoming(void *arg); static void test_channel_queue_size(void *arg); static void test_channel_write(void *arg); @@ -179,7 +182,7 @@ chan_test_cell_handler(channel_t *ch, tt_assert(ch); tt_assert(cell); - tor_free(cell); + test_chan_last_seen_fixed_cell_ptr = cell; ++test_chan_fixed_cells_recved; done: @@ -214,7 +217,7 @@ chan_test_var_cell_handler(channel_t *ch, tt_assert(ch); tt_assert(var_cell); - tor_free(var_cell); + test_chan_last_seen_var_cell_ptr = var_cell; ++test_chan_var_cells_recved; done: @@ -608,7 +611,7 @@ test_channel_dumpstats(void *arg) make_fake_cell(cell); old_count = test_chan_fixed_cells_recved; channel_queue_cell(ch, cell); - cell = NULL; + tor_free(cell); tt_int_op(test_chan_fixed_cells_recved, ==, old_count + 1); tt_assert(ch->n_bytes_recved > 0); tt_assert(ch->n_cells_recved > 0); @@ -819,7 +822,7 @@ test_channel_incoming(void *arg) make_fake_cell(cell); old_count = test_chan_fixed_cells_recved; channel_queue_cell(ch, cell); - cell = NULL; + tor_free(cell); tt_int_op(test_chan_fixed_cells_recved, ==, old_count + 1); /* Receive a variable-size cell */ @@ -827,7 +830,7 @@ test_channel_incoming(void *arg) make_fake_var_cell(var_cell); old_count = test_chan_var_cells_recved; channel_queue_var_cell(ch, var_cell); - var_cell = NULL; + tor_free(cell); tt_int_op(test_chan_var_cells_recved, ==, old_count + 1); /* Close it */ @@ -1423,6 +1426,113 @@ test_channel_queue_impossible(void *arg) } static void +test_channel_queue_incoming(void *arg) +{ + channel_t *ch = NULL; + cell_t *cell = NULL; + var_cell_t *var_cell = NULL; + int old_fixed_count, old_var_count; + + (void)arg; + + /* Mock these for duration of the test */ + MOCK(scheduler_channel_doesnt_want_writes, + scheduler_channel_doesnt_want_writes_mock); + MOCK(scheduler_release_channel, + scheduler_release_channel_mock); + + /* Accept cells to lower layer */ + test_chan_accept_cells = 1; + /* Use default overhead factor */ + test_overhead_estimate = 1.0f; + + ch = new_fake_channel(); + tt_assert(ch); + /* Start it off in OPENING */ + ch->state = CHANNEL_STATE_OPENING; + /* We'll need a cmux */ + ch->cmux = circuitmux_alloc(); + + /* Test cell handler getters */ + tt_ptr_op(channel_get_cell_handler(ch), ==, NULL); + tt_ptr_op(channel_get_var_cell_handler(ch), ==, NULL); + + /* Try to register it */ + channel_register(ch); + tt_assert(ch->registered); + + /* Open it */ + channel_change_state(ch, CHANNEL_STATE_OPEN); + tt_int_op(ch->state, ==, CHANNEL_STATE_OPEN); + + /* Assert that the incoming queue is empty */ + tt_assert(TOR_SIMPLEQ_EMPTY(&(ch->incoming_queue))); + + /* Queue an incoming fixed-length cell */ + cell = tor_malloc_zero(sizeof(cell_t)); + make_fake_cell(cell); + channel_queue_cell(ch, cell); + + /* Assert that the incoming queue has one entry */ + tt_int_op(chan_cell_queue_len(&(ch->incoming_queue)), ==, 1); + + /* Queue an incoming var cell */ + var_cell = tor_malloc_zero(sizeof(var_cell_t) + CELL_PAYLOAD_SIZE); + make_fake_var_cell(var_cell); + channel_queue_var_cell(ch, var_cell); + + /* Assert that the incoming queue has two entries */ + tt_int_op(chan_cell_queue_len(&(ch->incoming_queue)), ==, 2); + + /* + * Install cell handlers; this will drain the queue, so save the old + * cell counters first + */ + old_fixed_count = test_chan_fixed_cells_recved; + old_var_count = test_chan_var_cells_recved; + channel_set_cell_handlers(ch, + chan_test_cell_handler, + chan_test_var_cell_handler); + tt_ptr_op(channel_get_cell_handler(ch), ==, chan_test_cell_handler); + tt_ptr_op(channel_get_var_cell_handler(ch), ==, chan_test_var_cell_handler); + + /* Assert cells were received */ + tt_int_op(test_chan_fixed_cells_recved, ==, old_fixed_count + 1); + tt_int_op(test_chan_var_cells_recved, ==, old_var_count + 1); + + /* + * Assert that the pointers are different from the cells we allocated; + * when queueing cells with no incoming cell handlers installed, the + * channel layer should copy them to a new buffer, and free them after + * delivery. These pointers will have already been freed by the time + * we get here, so don't dereference them. + */ + tt_ptr_op(test_chan_last_seen_fixed_cell_ptr, !=, cell); + tt_ptr_op(test_chan_last_seen_var_cell_ptr, !=, var_cell); + + /* Assert queue is now empty */ + tt_assert(TOR_SIMPLEQ_EMPTY(&(ch->incoming_queue))); + + /* Close it; this contains an assertion that the incoming queue is empty */ + channel_mark_for_close(ch); + tt_int_op(ch->state, ==, CHANNEL_STATE_CLOSING); + chan_test_finish_close(ch); + tt_int_op(ch->state, ==, CHANNEL_STATE_CLOSED); + channel_run_cleanup(); + ch = NULL; + + done: + free_fake_channel(ch); + tor_free(cell); + tor_free(var_cell); + + UNMOCK(scheduler_channel_doesnt_want_writes); + UNMOCK(scheduler_release_channel); + + return; +} + +static void test_channel_queue_size(void *arg) { channel_t *ch = NULL; @@ -1666,6 +1776,7 @@ struct testcase_t channel_tests[] = { { "lifecycle_2", test_channel_lifecycle_2, TT_FORK, NULL, NULL }, { "multi", test_channel_multi, TT_FORK, NULL, NULL }, { "queue_impossible", test_channel_queue_impossible, TT_FORK, NULL, NULL }, + { "queue_incoming", test_channel_queue_incoming, TT_FORK, NULL, NULL }, { "queue_size", test_channel_queue_size, TT_FORK, NULL, NULL }, { "write", test_channel_write, TT_FORK, NULL, NULL }, END_OF_TESTCASES

1 0

[tor/master] Make sure channel_t queues its own copy of incoming cells
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit bd87d37a861c541afbeb660b4d8dd62df14d5b45 Author: Andrea Shepard <andrea(a)torproject.org> Date: Tue Mar 15 07:40:19 2016 +0000 Make sure channel_t queues its own copy of incoming cells --- src/or/channel.c | 38 ++++++++++++++++++++++++++++++++++---- src/or/channeltls.c | 11 +++++++++++ src/or/connection_or.c | 35 +++++++++++++++++++++++++++++++++++ src/or/connection_or.h | 1 + 4 files changed, 81 insertions(+), 4 deletions(-) diff --git a/src/or/channel.c b/src/or/channel.c index 21522a5..62a21be 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -2652,6 +2652,11 @@ channel_process_cells(channel_t *chan) /* * Process cells until we're done or find one we have no current handler * for. + * + * We must free the cells here after calling the handler, since custody + * of the buffer was given to the channel layer when they were queued; + * see comments on memory management in channel_queue_cell() and in + * channel_queue_var_cell() below. */ while (NULL != (q = TOR_SIMPLEQ_FIRST(&chan->incoming_queue))) { tor_assert(q); @@ -2669,6 +2674,7 @@ channel_process_cells(channel_t *chan) q->u.fixed.cell, chan, U64_PRINTF_ARG(chan->global_identifier)); chan->cell_handler(chan, q->u.fixed.cell); + tor_free(q->u.fixed.cell); tor_free(q); } else if (q->type == CELL_QUEUE_VAR && chan->var_cell_handler) { @@ -2681,6 +2687,7 @@ channel_process_cells(channel_t *chan) q->u.var.var_cell, chan, U64_PRINTF_ARG(chan->global_identifier)); chan->var_cell_handler(chan, q->u.var.var_cell); + tor_free(q->u.var.var_cell); tor_free(q); } else { /* Can't handle this one */ @@ -2701,6 +2708,7 @@ channel_queue_cell(channel_t *chan, cell_t *cell) { int need_to_queue = 0; cell_queue_entry_t *q; + cell_t *cell_copy = NULL; tor_assert(chan); tor_assert(cell); @@ -2728,8 +2736,19 @@ channel_queue_cell(channel_t *chan, cell_t *cell) U64_PRINTF_ARG(chan->global_identifier)); chan->cell_handler(chan, cell); } else { - /* Otherwise queue it and then process the queue if possible. */ - q = cell_queue_entry_new_fixed(cell); + /* + * Otherwise queue it and then process the queue if possible. + * + * We queue a copy, not the original pointer - it might have been on the + * stack in connection_or_process_cells_from_inbuf() (or another caller + * if we ever have a subclass other than channel_tls_t), or be freed + * there after we return. This is the uncommon case; the non-copying + * fast path occurs in the if (!need_to_queue) case above when the + * upper layer has installed cell handlers. + */ + cell_copy = tor_malloc_zero(sizeof(cell_t)); + memcpy(cell_copy, cell, sizeof(cell_t)); + q = cell_queue_entry_new_fixed(cell_copy); log_debug(LD_CHANNEL, "Queueing incoming cell_t %p for channel %p " "(global ID " U64_FORMAT ")", @@ -2755,6 +2774,7 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) { int need_to_queue = 0; cell_queue_entry_t *q; + var_cell_t *cell_copy = NULL; tor_assert(chan); tor_assert(var_cell); @@ -2783,8 +2803,18 @@ channel_queue_var_cell(channel_t *chan, var_cell_t *var_cell) U64_PRINTF_ARG(chan->global_identifier)); chan->var_cell_handler(chan, var_cell); } else { - /* Otherwise queue it and then process the queue if possible. */ - q = cell_queue_entry_new_var(var_cell); + /* + * Otherwise queue it and then process the queue if possible. + * + * We queue a copy, not the original pointer - it might have been on the + * stack in connection_or_process_cells_from_inbuf() (or another caller + * if we ever have a subclass other than channel_tls_t), or be freed + * there after we return. This is the uncommon case; the non-copying + * fast path occurs in the if (!need_to_queue) case above when the + * upper layer has installed cell handlers. + */ + cell_copy = var_cell_copy(var_cell); + q = cell_queue_entry_new_var(cell_copy); log_debug(LD_CHANNEL, "Queueing incoming var_cell_t %p for channel %p " "(global ID " U64_FORMAT ")", diff --git a/src/or/channeltls.c b/src/or/channeltls.c index c90f569..2a84514 100644 --- a/src/or/channeltls.c +++ b/src/or/channeltls.c @@ -1009,6 +1009,11 @@ channel_tls_time_process_cell(cell_t *cell, channel_tls_t *chan, int *time, * for cell types specific to the handshake for this transport protocol and * handles them, and queues all other cells to the channel_t layer, which * eventually will hand them off to command.c. + * + * The channel layer itself decides whether the cell should be queued or + * can be handed off immediately to the upper-layer code. It is responsible + * for copying in the case that it queues; we merely pass pointers through + * which we get from connection_or_process_cells_from_inbuf(). */ void @@ -1106,6 +1111,12 @@ channel_tls_handle_cell(cell_t *cell, or_connection_t *conn) * related and live below the channel_t layer, so no variable-length * cells ever get delivered in the current implementation, but I've left * the mechanism in place for future use. + * + * If we were handing them off to the upper layer, the channel_t queueing + * code would be responsible for memory management, and we'd just be passing + * pointers through from connection_or_process_cells_from_inbuf(). That + * caller always frees them after this function returns, so this function + * should never free var_cell. */ void diff --git a/src/or/connection_or.c b/src/or/connection_or.c index a967c93..9944494 100644 --- a/src/or/connection_or.c +++ b/src/or/connection_or.c @@ -488,6 +488,28 @@ var_cell_new(uint16_t payload_len) return cell; } +/** + * Copy a var_cell_t + */ + +var_cell_t * +var_cell_copy(const var_cell_t *src) +{ + var_cell_t *copy = NULL; + size_t size = 0; + + if (src != NULL) { + size = STRUCT_OFFSET(var_cell_t, payload) + src->payload_len; + copy = tor_malloc_zero(size); + copy->payload_len = src->payload_len; + copy->command = src->command; + copy->circ_id = src->circ_id; + memcpy(copy->payload, src->payload, copy->payload_len); + } + + return copy; +} + /** Release all space held by <b>cell</b>. */ void var_cell_free(var_cell_t *cell) @@ -2060,6 +2082,19 @@ connection_or_process_cells_from_inbuf(or_connection_t *conn) { var_cell_t *var_cell; + /* + * Note on memory management for incoming cells: below the channel layer, + * we shouldn't need to consider its internal queueing/copying logic. It + * is safe to pass cells to it on the stack or on the heap, but in the + * latter case we must be sure we free them later. + * + * The incoming cell queue code in channel.c will (in the common case) + * decide it can pass them to the upper layer immediately, in which case + * those functions may run directly on the cell pointers we pass here, or + * it may decide to queue them, in which case it will allocate its own + * buffer and copy the cell. + */ + while (1) { log_debug(LD_OR, TOR_SOCKET_T_FORMAT": starting, inbuf_datalen %d " diff --git a/src/or/connection_or.h b/src/or/connection_or.h index 3877fd5..0bd8567 100644 --- a/src/or/connection_or.h +++ b/src/or/connection_or.h @@ -97,6 +97,7 @@ void cell_pack(packed_cell_t *dest, const cell_t *src, int wide_circ_ids); int var_cell_pack_header(const var_cell_t *cell, char *hdr_out, int wide_circ_ids); var_cell_t *var_cell_new(uint16_t payload_len); +var_cell_t *var_cell_copy(const var_cell_t *src); void var_cell_free(var_cell_t *cell); /** DOCDOC */

1 0

[tor/master] changes file for bug18570
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 72ebf4160412f64fb6ae0cd97dd89d01d89c075a Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:19:07 2016 -0400 changes file for bug18570 --- changes/bug18570 | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/changes/bug18570 b/changes/bug18570 new file mode 100644 index 0000000..04f72f4 --- /dev/null +++ b/changes/bug18570 @@ -0,0 +1,7 @@ + o Minor bugfixes (correctness): + - Fix a bad memory handling bug that would occur if we had queued + a cell on a channel's incoming queue. Fortunately, we can't actually + queue a cell like that as our code is constructed today, but it's best + to avoid this kind of error, even if there isn't any code that triggers + it today. Fixes bug 18570; bugfix on 0.2.4.4-alpha. +

1 0

[tor/master] Merge branch 'bug18570_027'
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit cb3f9bc2d48e8c3f4847643c03e082d394d33168 Merge: a42938c 72ebf41 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:20:16 2016 -0400 Merge branch 'bug18570_027' changes/bug18570 | 7 +++ src/or/channel.c | 38 +++++++++++++-- src/or/channeltls.c | 11 +++++ src/or/connection_or.c | 35 ++++++++++++++ src/or/connection_or.h | 1 + src/test/test_channel.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 204 insertions(+), 9 deletions(-)

1 0

[tor/master] changes file for 18600
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 70024ea6c4df8b2fe0c07fe5519d2c9683fc833b Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:33:58 2016 -0400 changes file for 18600 --- changes/bug18600 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/changes/bug18600 b/changes/bug18600 new file mode 100644 index 0000000..bf53bfe --- /dev/null +++ b/changes/bug18600 @@ -0,0 +1,3 @@ + o Minor bugfixes (logging): + - Scrub service in from "unrecognized service ID" log messages. + Fixes bug 18600; bugfix on 0.2.4.11-alpha.

1 0

[tor/master] Merge remote-tracking branch 'special/bug18600'
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit 13eb120beaff5e006c0eb9a3fc8563c5df16a328 Merge: cb3f9bc 2c057c2 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Mar 21 10:32:39 2016 -0400 Merge remote-tracking branch 'special/bug18600' src/or/rendservice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

1 0

[tor/master] test_options.c: assert that TransProxyType is tested
by nickm＠torproject.org 21 Mar '16

21 Mar '16

commit a42938c07670162863decc952b4d73681d9302d6 Author: Steven Chamberlain <steven(a)pyro.eu.org> Date: Fri Mar 18 13:17:18 2016 +0000 test_options.c: assert that TransProxyType is tested If a new platform defines USE_TRANSPARENT, ensure that a test runs for its TransProxyType. --- src/test/test_options.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/test_options.c b/src/test/test_options.c index 9ea5718..bd00e61 100644 --- a/src/test/test_options.c +++ b/src/test/test_options.c @@ -1102,6 +1102,9 @@ test_options_validate__transproxy(void *ignored) tt_assert(!msg); #endif + // Assert that a test has run for some TransProxyType + tt_assert(tdata); + #else tdata = get_options_test_data("TransPort 127.0.0.1:555\n");

1 0

[translation/torbutton-torbuttonproperties] Update translations for torbutton-torbuttonproperties
by translation＠torproject.org 21 Mar '16

21 Mar '16

commit eeaf3097a50ebed86f59c9a0ec7c4df2f38ef3fb Author: Translation commit bot <translation(a)torproject.org> Date: Mon Mar 21 12:45:57 2016 +0000 Update translations for torbutton-torbuttonproperties --- es/torbutton.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/es/torbutton.properties b/es/torbutton.properties index 319f5e7..a29234b 100644 --- a/es/torbutton.properties +++ b/es/torbutton.properties @@ -76,4 +76,4 @@ profileProblemTitle=Problema del perfil %S profileReadOnly=No puede ejecutar %S desde un sistema de ficheros de solo-lectura. Copie %S a otra ubicación antes de intentar usarlo. profileReadOnlyMac=No puede ejecutar %S desde un sistema de ficheros de solo-lectura. Copie %S a su escritorio o la carpeta de aplicaciones antes de intentar usarlo. profileAccessDenied=%S no tiene permiso para acceder al perfil. Ajuste los permisos de su sistema de ficheros e inténtelo de nuevo. -profileMigrationFailed=Migration of your existing %S profile failed.\nNew settings will be used. +profileMigrationFailed=Ha ocurrido un error en la migración de su perfil %S. \nSe usarán los nuevos ajustes.

1 0

[translation/tor-launcher-properties_completed] Update translations for tor-launcher-properties_completed
by translation＠torproject.org 21 Mar '16

21 Mar '16

commit 61f84b38a7a8759ddd7867208ee05c71194d8d3a Author: Translation commit bot <translation(a)torproject.org> Date: Mon Mar 21 12:45:38 2016 +0000 Update translations for tor-launcher-properties_completed --- es/torlauncher.properties | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/es/torlauncher.properties b/es/torlauncher.properties index 42780e5..5283bd1 100644 --- a/es/torlauncher.properties +++ b/es/torlauncher.properties @@ -1,4 +1,4 @@ -### Copyright (c) 2014, The Tor Project, Inc. +### Copyright (c) 2016, The Tor Project, Inc. ### See LICENSE for licensing information. torlauncher.error_title=Arranque de Tor @@ -13,8 +13,8 @@ torlauncher.tor_bootstrap_failed_details=%1$S falló (%2$S). torlauncher.unable_to_start_tor=Imposible iniciar Tor.\n\n%S torlauncher.tor_missing=No se encuentra el archivo ejecutable de Tor. -torlauncher.torrc_missing=No se encuentra el archivo "torrc" -torlauncher.datadir_missing=El directorio de datos de Tor no existe. +torlauncher.torrc_missing=No existe el fichero torcc y no puede ser creado. +torlauncher.datadir_missing=El directorio de datos de Tor no existe y no puede ser creado torlauncher.password_hash_missing=Fallo al obtener la contraseña cifrada. torlauncher.failed_to_get_settings=No se pudo recuperar las preferencias de Tor.\n\n%S

1 0

[translation/tor-launcher-properties] Update translations for tor-launcher-properties
by translation＠torproject.org 21 Mar '16

21 Mar '16

commit 22c387d67ba05addbbb41607b3d5637ba4844e95 Author: Translation commit bot <translation(a)torproject.org> Date: Mon Mar 21 12:45:33 2016 +0000 Update translations for tor-launcher-properties --- es/torlauncher.properties | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/es/torlauncher.properties b/es/torlauncher.properties index fa78ce9..5283bd1 100644 --- a/es/torlauncher.properties +++ b/es/torlauncher.properties @@ -13,8 +13,8 @@ torlauncher.tor_bootstrap_failed_details=%1$S falló (%2$S). torlauncher.unable_to_start_tor=Imposible iniciar Tor.\n\n%S torlauncher.tor_missing=No se encuentra el archivo ejecutable de Tor. -torlauncher.torrc_missing=The torrc file is missing and could not be created. -torlauncher.datadir_missing=The Tor data directory does not exist and could not be created. +torlauncher.torrc_missing=No existe el fichero torcc y no puede ser creado. +torlauncher.datadir_missing=El directorio de datos de Tor no existe y no puede ser creado torlauncher.password_hash_missing=Fallo al obtener la contraseña cifrada. torlauncher.failed_to_get_settings=No se pudo recuperar las preferencias de Tor.\n\n%S

1 0