commit 120305c7f3a9a5103e103fa557221bff2a8082b8
parent 89d870d32816c32a220f59debc1d8f3469e97a9a
Author: Nick Mathewson <nickm@torproject.org>
Date: Thu, 17 Apr 2025 21:15:30 -0400
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.
Diffstat:
9 files changed, 107 insertions(+), 80 deletions(-)
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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; \
