From 120305c7f3a9a5103e103fa557221bff2a8082b8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 17 Apr 2025 21:15:30 -0400 Subject: [PATCH] Change relay_msg_t to _not_ hold a copy of the message. Previously we had to memdup every time we parsed a relay_msg_t; but that's unnecessary, since (most) every time we use it, we have a longer-lived cell object. This _did_ require some hacking in relay_msg_copy, but I think the gain in simplicity is worth it. --- src/core/or/relay.c | 16 ++--- src/core/or/relay_msg.c | 107 ++++++++++++++++++++------------- src/core/or/relay_msg.h | 3 + src/core/or/relay_msg_st.h | 10 +-- src/test/test_cell_formats.c | 17 ++++-- src/test/test_circuitbuild.c | 3 +- src/test/test_circuitpadding.c | 5 +- src/test/test_conflux_cell.c | 9 +-- src/test/test_relaycell.c | 17 +++--- 9 files changed, 107 insertions(+), 80 deletions(-) diff --git a/src/core/or/relay.c b/src/core/or/relay.c index 5fdf5715bb..7a79bb3888 100644 --- a/src/core/or/relay.c +++ b/src/core/or/relay.c @@ -265,14 +265,13 @@ circuit_receive_relay_cell(cell_t *cell, circuit_t *circ, * the SENDME if need be. */ sendme_record_received_cell_digest(circ, layer_hint); - // TODO #41051: This also doesn't need to copy! - relay_msg_t *msg = relay_msg_decode_cell(format, cell); - - if (msg == NULL) { + relay_msg_t msg_buf; + if (relay_msg_decode_cell_in_place(format, cell, &msg_buf) < 0) { log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "Received undecodable relay cell"); return -END_CIRC_REASON_TORPROTOCOL; } + const relay_msg_t *msg = &msg_buf; if (circ->purpose == CIRCUIT_PURPOSE_PATH_BIAS_TESTING) { if (pathbias_check_probe_response(circ, msg) == -1) { @@ -281,7 +280,6 @@ circuit_receive_relay_cell(cell_t *cell, circuit_t *circ, /* We need to drop this cell no matter what to avoid code that expects * a certain purpose (such as the hidserv code). */ - relay_msg_free(msg); return 0; } @@ -294,7 +292,6 @@ circuit_receive_relay_cell(cell_t *cell, circuit_t *circ, log_fn(LOG_PROTOCOL_WARN, LD_PROTOCOL, "connection_edge_process_relay_cell (away from origin) " "failed."); - relay_msg_free(msg); return reason; } } @@ -311,11 +308,9 @@ circuit_receive_relay_cell(cell_t *cell, circuit_t *circ, log_warn(LD_OR, "connection_edge_process_relay_cell (at origin) failed."); } - relay_msg_free(msg); return reason; } } - relay_msg_free(msg); return 0; } @@ -636,10 +631,7 @@ relay_send_command_from_edge_,(streamid_t stream_id, circuit_t *orig_circ, msg.command = relay_command; msg.stream_id = stream_id; msg.length = payload_len; - // This cast is mildly naughty, but ultimately harmless: - // There is no need to copy the payload here, so long as we don't - // free it. - msg.body = (uint8_t *) payload; + msg.body = (const uint8_t *) payload; msg_body_len = msg.length; if (relay_msg_encode_cell(cell_format, &msg, &cell) < 0) { diff --git a/src/core/or/relay_msg.c b/src/core/or/relay_msg.c index 330bbf714c..458331f634 100644 --- a/src/core/or/relay_msg.c +++ b/src/core/or/relay_msg.c @@ -32,7 +32,6 @@ relay_msg_free_(relay_msg_t *msg) if (!msg) { return; } - tor_free(msg->body); tor_free(msg); } @@ -42,7 +41,6 @@ void relay_msg_clear(relay_msg_t *msg) { tor_assert(msg); - tor_free(msg->body); memset(msg, 0, sizeof(*msg)); } @@ -59,34 +57,41 @@ relay_msg_clear(relay_msg_t *msg) #define V1_PAYLOAD_OFFSET_NO_STREAM_ID 19 #define V1_PAYLOAD_OFFSET_WITH_STREAM_ID 21 -/** Allocate a new relay message and copy the content of the given message. */ +/** Allocate a new relay message and copy the content of the given message. + * + * This message allocation _will_ own its body, even if the original did not. + **/ relay_msg_t * relay_msg_copy(const relay_msg_t *msg) { - relay_msg_t *new = tor_malloc_zero(sizeof(*msg)); + void *alloc = tor_malloc_zero(sizeof(relay_msg_t) + msg->length); + relay_msg_t *new_msg = alloc; + uint8_t *body = ((uint8_t*)alloc) + sizeof(relay_msg_t); - memcpy(new, msg, sizeof(*msg)); - new->body = tor_memdup_nulterm(msg->body, msg->length); - memcpy(new->body, msg->body, new->length); + memcpy(new_msg, msg, sizeof(*msg)); + new_msg->body = body; + memcpy(body, msg->body, msg->length); - return new; + return new_msg; } /** Set a relay message data into the given message. Useful for stack allocated - * messages. */ + * messages. + * + * Note that the resulting relay_msg will have a reference to + * 'payload', which must not be changed while this message is in use. + **/ void relay_msg_set(const uint8_t relay_cell_proto, const uint8_t cmd, const streamid_t stream_id, const uint8_t *payload, const uint16_t payload_len, relay_msg_t *msg) { - // TODO #41051: Should this free msg->body? (void) relay_cell_proto; msg->command = cmd; msg->stream_id = stream_id; msg->length = payload_len; - msg->body = tor_malloc_zero(msg->length); - memcpy(msg->body, payload, msg->length); + msg->body = payload; } /* Add random bytes to the unused portion of the payload, to foil attacks @@ -170,14 +175,14 @@ encode_v1_cell(const relay_msg_t *msg, return 0; } -/** Try to decode 'cell' into a newly allocated V0 relay message. +/** Try to decode 'cell' into a V0 relay message. * - * Return NULL on error. + * Return 0 on success, -1 on error. */ -static relay_msg_t * -decode_v0_cell(const cell_t *cell) +static int +decode_v0_cell(const cell_t *cell, relay_msg_t *out) { - relay_msg_t *out = tor_malloc_zero(sizeof(relay_msg_t)); + memset(out, 0, sizeof(relay_msg_t)); out->is_relay_early = (cell->command == CELL_RELAY_EARLY); const uint8_t *body = cell->payload; @@ -186,30 +191,28 @@ decode_v0_cell(const cell_t *cell) out->length = ntohs(get_uint16(body + V0_LEN_OFFSET)); if (out->length > CELL_PAYLOAD_SIZE - RELAY_HEADER_SIZE_V0) { - goto err; + return -1; } - out->body = tor_memdup_nulterm(body + V0_PAYLOAD_OFFSET, out->length); + out->body = body + V0_PAYLOAD_OFFSET; - return out; - err: - relay_msg_free(out); - return NULL; + return 0; } -/** Try to decode 'cell' into a newly allocated V0 relay message. +/** Try to decode 'cell' into a V1 relay message. * - * Return NULL on error. + * Return 0 on success, -1 on error.= */ -static relay_msg_t * -decode_v1_cell(const cell_t *cell) +static int +decode_v1_cell(const cell_t *cell, relay_msg_t *out) { - relay_msg_t *out = tor_malloc_zero(sizeof(relay_msg_t)); + memset(out, 0, sizeof(relay_msg_t)); out->is_relay_early = (cell->command == CELL_RELAY_EARLY); const uint8_t *body = cell->payload; out->command = get_uint8(body + V1_CMD_OFFSET); if (! is_known_relay_command(out->command)) - goto err; + return -1; + out->length = ntohs(get_uint16(body + V1_LEN_OFFSET)); size_t payload_offset; if (relay_cmd_expects_streamid_in_v1(out->command)) { @@ -220,13 +223,10 @@ decode_v1_cell(const cell_t *cell) } if (out->length > CELL_PAYLOAD_SIZE - payload_offset) - goto err; - out->body = tor_memdup_nulterm(body + payload_offset, out->length); + return -1; + out->body = body + payload_offset; - return out; - err: - relay_msg_free(out); - return NULL; + return 0; } /** * Encode 'msg' into 'cell' according to the rules of 'format'. @@ -262,19 +262,42 @@ relay_msg_encode_cell(relay_cell_fmt_t format, * Decode 'cell' (which must be RELAY or RELAY_EARLY) into a newly allocated * 'relay_msg_t'. * - * Return NULL on error. + * Note that the resulting relay_msg_t will have a reference to 'cell'. + * Do not change 'cell' while the resulting message is still in use! + * + * Return -1 on error, and 0 on success. */ -relay_msg_t * -relay_msg_decode_cell(relay_cell_fmt_t format, - const cell_t *cell) +int +relay_msg_decode_cell_in_place(relay_cell_fmt_t format, + const cell_t *cell, + relay_msg_t *msg_out) { switch (format) { case RELAY_CELL_FORMAT_V0: - return decode_v0_cell(cell); + return decode_v0_cell(cell, msg_out); case RELAY_CELL_FORMAT_V1: - return decode_v1_cell(cell); + return decode_v1_cell(cell, msg_out); default: tor_fragile_assert(); - return NULL; + return -1; + } +} + +/** + * As relay_msg_decode_cell_in_place, but allocate a new relay_msg_t + * on success. + * + * Return NULL on error. + */ +relay_msg_t * +relay_msg_decode_cell(relay_cell_fmt_t format, + const cell_t *cell) +{ + relay_msg_t *msg = tor_malloc(sizeof(relay_msg_t)); + if (relay_msg_decode_cell_in_place(format, cell, msg) < 0) { + relay_msg_free(msg); + return NULL; + } else { + return msg; } } diff --git a/src/core/or/relay_msg.h b/src/core/or/relay_msg.h index 3670bc5e92..ab99e11d69 100644 --- a/src/core/or/relay_msg.h +++ b/src/core/or/relay_msg.h @@ -24,6 +24,9 @@ void relay_msg_set(const uint8_t relay_cell_proto, const uint8_t cmd, int relay_msg_encode_cell(relay_cell_fmt_t format, const relay_msg_t *msg, cell_t *cell_out) ATTR_WUR; +int relay_msg_decode_cell_in_place(relay_cell_fmt_t format, + const cell_t *cell, + relay_msg_t *msg_out) ATTR_WUR; relay_msg_t *relay_msg_decode_cell( relay_cell_fmt_t format, const cell_t *cell) ATTR_WUR; diff --git a/src/core/or/relay_msg_st.h b/src/core/or/relay_msg_st.h index 5b59adc0b2..9cf27305e9 100644 --- a/src/core/or/relay_msg_st.h +++ b/src/core/or/relay_msg_st.h @@ -27,11 +27,11 @@ typedef struct relay_msg_t { streamid_t stream_id; /* Indicate if this is a message from a relay early cell. */ bool is_relay_early; - /* Message body of a relay message. */ - // TODO #41051: This is an owned copy of the body. - // It might be better to turn this into a non-owned pointer into - // the cell body, if we can, to save a copy. - uint8_t *body; + /* Message body of a relay message. + * + * NOTE that this struct does not own the body; instead, this is a pointer + * into a different object. */ + const uint8_t *body; } relay_msg_t; #endif /* !defined(TOR_RELAY_MSG_ST_H) */ diff --git a/src/test/test_cell_formats.c b/src/test/test_cell_formats.c index 9bf7042787..126aad1a3c 100644 --- a/src/test/test_cell_formats.c +++ b/src/test/test_cell_formats.c @@ -1189,13 +1189,15 @@ test_cfmt_relay_msg_encoding_simple(void *arg) cell_t cell; char *mem_op_hex_tmp = NULL; int r; + uint8_t body[100]; /* Simple message: Data, fits easily in cell. */ msg1 = tor_malloc_zero(sizeof(relay_msg_t)); msg1->command = RELAY_COMMAND_DATA; msg1->stream_id = 0x250; msg1->length = 11; - msg1->body = tor_memdup("hello world", 11); + msg1->body = body; + strlcpy((char*)body, "hello world", sizeof(body)); r = relay_msg_encode_cell(RELAY_CELL_FORMAT_V0, msg1, &cell); tt_int_op(r, OP_EQ, 0); @@ -1229,7 +1231,8 @@ test_cfmt_relay_msg_encoding_simple(void *arg) msg1->command = RELAY_COMMAND_SENDME; msg1->stream_id = 0; msg1->length = 20; - msg1->body = tor_memdup("hello i am a tag....", 20); + msg1->body = body; + strlcpy((char *)body, "hello i am a tag....", sizeof(body)); r = relay_msg_encode_cell(RELAY_CELL_FORMAT_V0, msg1, &cell); tt_int_op(r, OP_EQ, 0); @@ -1325,13 +1328,14 @@ test_cfmt_relay_cell_padding(void *arg) { (void)arg; relay_msg_t *msg1 = NULL; + uint8_t buf[500]; // Longer than it needs to be. + memset(buf, 0xff, sizeof(buf)); /* Simple message; we'll adjust the length and encode it. */ msg1 = tor_malloc_zero(sizeof(relay_msg_t)); msg1->command = RELAY_COMMAND_DATA; msg1->stream_id = 0x250; - msg1->body = tor_malloc(500); // Longer than it needs to be. - memset(msg1->body, 0xff, 500); + msg1->body = buf; // Empty message msg1->length = 0; @@ -1402,12 +1406,13 @@ test_cfmt_relay_msg_encoding_error(void *arg) relay_msg_t *msg1 = NULL; int r; cell_t cell; + uint8_t buf[500]; // Longer than it needs to be. + memset(buf, 0xff, sizeof(buf)); msg1 = tor_malloc_zero(sizeof(relay_msg_t)); msg1->command = RELAY_COMMAND_DATA; msg1->stream_id = 0x250; - msg1->body = tor_malloc(500); // Longer than it needs to be. - memset(msg1->body, 0xff, 500); + msg1->body = buf; tor_capture_bugs_(5); // Too long for v0. diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c index 9bebcc2873..4db86d4423 100644 --- a/src/test/test_circuitbuild.c +++ b/src/test/test_circuitbuild.c @@ -1295,7 +1295,8 @@ test_circuit_extend(void *arg) setup_full_capture_of_logs(LOG_INFO); msg->command = RELAY_COMMAND_EXTEND2; - msg->body = tor_memdup("xyz", 3); + uint8_t body[3] = "xyz"; + msg->body = body; #ifndef ALL_BUGS_ARE_FATAL /* Circuit must be non-NULL */ diff --git a/src/test/test_circuitpadding.c b/src/test/test_circuitpadding.c index a16a169cdf..ddc11f7408 100644 --- a/src/test/test_circuitpadding.c +++ b/src/test/test_circuitpadding.c @@ -1349,8 +1349,9 @@ test_circuitpadding_wronghop(void *arg) /* 3. Garbled negotiated cell */ memset(&msg, 0, sizeof(msg)); - msg.body = tor_malloc_zero(99); - memset(msg.body, 0xff, 99); + uint8_t buf[99]; + memset(buf, 0xff, 99); + msg.body = buf; ret = circpad_handle_padding_negotiated(client_side, &msg, TO_ORIGIN_CIRCUIT(client_side)->cpath->next); tt_int_op(ret, OP_EQ, -1); diff --git a/src/test/test_conflux_cell.c b/src/test/test_conflux_cell.c index 7e02f54764..2527ba1611 100644 --- a/src/test/test_conflux_cell.c +++ b/src/test/test_conflux_cell.c @@ -24,6 +24,8 @@ test_link(void *arg) conflux_cell_link_t link; conflux_cell_link_t *decoded_link = NULL; relay_msg_t msg; + uint8_t buf0[RELAY_PAYLOAD_SIZE_MAX]; + uint8_t buf1[RELAY_PAYLOAD_SIZE_MAX]; (void) arg; @@ -36,16 +38,15 @@ test_link(void *arg) crypto_rand((char *) link.nonce, sizeof(link.nonce)); - msg.body = tor_malloc(500); - ssize_t cell_len = build_link_cell(&link, msg.body); + ssize_t cell_len = build_link_cell(&link, buf0); tt_int_op(cell_len, OP_GT, 0); msg.length = cell_len; + msg.body = buf0; decoded_link = conflux_cell_parse_link(&msg); tt_assert(decoded_link); - uint8_t buf[RELAY_PAYLOAD_SIZE]; - ssize_t enc_cell_len = build_link_cell(decoded_link, buf); + ssize_t enc_cell_len = build_link_cell(decoded_link, buf1); tt_int_op(cell_len, OP_EQ, enc_cell_len); /* Validate the original link object with the decoded one. */ diff --git a/src/test/test_relaycell.c b/src/test/test_relaycell.c index ed5732b1f3..1c409346ef 100644 --- a/src/test/test_relaycell.c +++ b/src/test/test_relaycell.c @@ -173,15 +173,15 @@ fake_entry_conn(origin_circuit_t *oncirc, streamid_t id) return entryconn; } -#define PACK_CELL(id, cmd, body_s) do { \ +#define PACK_CELL(id, cmd, body_s) do { \ len_tmp = sizeof(body_s)-1; \ relay_msg_free(msg); \ msg = tor_malloc_zero(sizeof(relay_msg_t)); \ msg->command = (cmd); \ msg->stream_id = (id); \ - msg->body = tor_malloc(len_tmp); \ + msg->body = cell_buf; \ msg->length = len_tmp; \ - memcpy(msg->body, (body_s), len_tmp); \ + memcpy(cell_buf, (body_s), len_tmp); \ } while (0) #define ASSERT_COUNTED_BW() do { \ tt_int_op(circ->n_delivered_read_circ_bw, OP_EQ, delivered+msg->length); \ @@ -206,6 +206,7 @@ subtest_circbw_halfclosed(origin_circuit_t *circ, streamid_t init_id) entry_connection_t *entryconn4=NULL; int delivered = circ->n_delivered_read_circ_bw; int overhead = circ->n_overhead_read_circ_bw; + uint8_t cell_buf[RELAY_PAYLOAD_SIZE_MAX]; /* Make new entryconns */ entryconn2 = fake_entry_conn(circ, init_id); @@ -678,6 +679,7 @@ test_circbw_relay(void *arg) origin_circuit_t *circ; int delivered = 0; int overhead = 0; + uint8_t cell_buf[RELAY_PAYLOAD_SIZE_MAX]; (void)arg; @@ -708,10 +710,8 @@ test_circbw_relay(void *arg) /* Properly formatted connect cell: counted */ PACK_CELL(1, RELAY_COMMAND_CONNECTED, "Data1234"); - tor_free(msg->body); - msg->body = tor_malloc(500); tor_addr_parse(&addr, "30.40.50.60"); - msg->length = connected_cell_format_payload(msg->body, + msg->length = connected_cell_format_payload(cell_buf, &addr, 1024); connection_edge_process_relay_cell(msg, TO_CIRCUIT(circ), edgeconn, circ->cpath); @@ -882,14 +882,15 @@ test_relaycell_resolved(void *arg) int r; or_options_t *options = get_options_mutable(); size_t len_tmp; + uint8_t cell_buf[RELAY_PAYLOAD_SIZE_MAX]; #define SET_CELL(s) do { \ relay_msg_free(msg); \ msg = tor_malloc_zero(sizeof(relay_msg_t)); \ len_tmp = sizeof(s) - 1; \ - msg->body = tor_malloc_zero(len_tmp); \ + msg->body = cell_buf; \ msg->length = len_tmp; \ - memcpy(msg->body, s, len_tmp); \ + memcpy(cell_buf, s, len_tmp); \ } while (0) #define MOCK_RESET() do { \ srm_ncalls = mum_ncalls = 0; \ -- 2.47.2