Skip to content

Commit 62efd96

Browse files
authored
Fix segfault when decoding mixed network message types in batched frames (eclipse-zenoh#1156)
* Added test to reproduce segfault conditions. * Ensure _z_network_message_t is cleared before decoding message. Also removed extraneous _z_msg_query_clear().
1 parent 9e54f7d commit 62efd96

File tree

3 files changed

+133
-2
lines changed

3 files changed

+133
-2
lines changed

src/protocol/codec/network.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -630,6 +630,7 @@ z_result_t _z_network_message_encode(_z_wbuf_t *wbf, const _z_network_message_t
630630
z_result_t _z_network_message_decode(_z_network_message_t *msg, _z_zbuf_t *zbf, _z_arc_slice_t *arcs,
631631
uintptr_t mapping) {
632632
uint8_t *header;
633+
*msg = (_z_network_message_t){0};
633634
_Z_RETURN_IF_ERR(_z_uint8_decode_as_ref(&header, zbf));
634635
switch (_Z_MID(*header)) {
635636
case _Z_MID_N_DECLARE: {

src/session/queryable.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -257,8 +257,7 @@ z_result_t _z_trigger_queryables(_z_transport_common_t *transport, _z_msg_query_
257257
_Z_SET_IF_OK(ret, _z_query_move_data(_Z_RC_IN_VAL(&query), &msgq->_ext_value, &qle_infos.ke, &msgq->_parameters,
258258
&transport->_session, qid, &msgq->_ext_attachment, anyke, &msgq->_ext_info));
259259
_Z_CLEAN_RETURN_IF_ERR(ret, _z_wireexpr_clear(q_key); _z_msg_query_clear(msgq);
260-
_z_queryable_cache_data_clear(&qle_infos); _z_query_rc_drop(&query);
261-
_z_msg_query_clear(msgq))
260+
_z_queryable_cache_data_clear(&qle_infos); _z_query_rc_drop(&query))
262261

263262
_Z_RC_IN_VAL(&query)->_is_local = is_local;
264263
// Parse session_queryable svec

tests/z_msgcodec_test.c

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1461,6 +1461,13 @@ _z_n_msg_push_t gen_push(void) {
14611461
};
14621462
}
14631463

1464+
static _z_network_message_t gen_push_message(void) {
1465+
return (_z_network_message_t){
1466+
._tag = _Z_N_PUSH,
1467+
._body._push = gen_push(),
1468+
};
1469+
}
1470+
14641471
void assert_eq_push(const _z_n_msg_push_t *left, const _z_n_msg_push_t *right) {
14651472
assert_eq_timestamp(&left->_timestamp, &right->_timestamp);
14661473
assert_eq_keyexpr(&left->_key, &right->_key);
@@ -1516,6 +1523,13 @@ _z_n_msg_request_t gen_request(void) {
15161523
return request;
15171524
}
15181525

1526+
static _z_network_message_t gen_request_message(void) {
1527+
return (_z_network_message_t){
1528+
._tag = _Z_N_REQUEST,
1529+
._body._request = gen_request(),
1530+
};
1531+
}
1532+
15191533
void assert_eq_request(const _z_n_msg_request_t *left, const _z_n_msg_request_t *right) {
15201534
assert(left->_rid == right->_rid);
15211535
assert_eq_keyexpr(&left->_key, &right->_key);
@@ -1580,6 +1594,13 @@ _z_n_msg_response_t gen_response(void) {
15801594
return ret;
15811595
}
15821596

1597+
static _z_network_message_t gen_response_message(void) {
1598+
return (_z_network_message_t){
1599+
._tag = _Z_N_RESPONSE,
1600+
._body._response = gen_response(),
1601+
};
1602+
}
1603+
15831604
void assert_eq_response(const _z_n_msg_response_t *left, const _z_n_msg_response_t *right) {
15841605
assert_eq_keyexpr(&left->_key, &right->_key);
15851606
assert_eq_timestamp(&left->_ext_timestamp, &right->_ext_timestamp);
@@ -1618,6 +1639,14 @@ void response_message(void) {
16181639
}
16191640

16201641
_z_n_msg_response_final_t gen_response_final(void) { return (_z_n_msg_response_final_t){._request_id = gen_zint()}; }
1642+
1643+
static _z_network_message_t gen_response_final_message(void) {
1644+
return (_z_network_message_t){
1645+
._tag = _Z_N_RESPONSE_FINAL,
1646+
._body._response_final = gen_response_final(),
1647+
};
1648+
}
1649+
16211650
void assert_eq_response_final(const _z_n_msg_response_final_t *left, const _z_n_msg_response_final_t *right) {
16221651
assert(left->_request_id == right->_request_id);
16231652
}
@@ -2132,6 +2161,104 @@ void test_buffer_too_small(void) {
21322161
assert(deserialized_len == SIZE_MAX);
21332162
}
21342163

2164+
static _z_network_message_t make_specific_net_msg(uint8_t which) {
2165+
switch (which) {
2166+
case 0:
2167+
return gen_declare_message();
2168+
case 1:
2169+
return gen_push_message();
2170+
case 2:
2171+
return gen_request_message();
2172+
case 3:
2173+
return gen_response_message();
2174+
case 4:
2175+
return gen_response_final_message();
2176+
case 5:
2177+
return gen_interest_message();
2178+
default:
2179+
assert(false && "Invalid network message selector");
2180+
}
2181+
2182+
return (_z_network_message_t){0};
2183+
}
2184+
2185+
static const char *net_msg_name(uint8_t which) {
2186+
switch (which) {
2187+
case 0:
2188+
return "DECLARE";
2189+
case 1:
2190+
return "PUSH";
2191+
case 2:
2192+
return "INTEREST";
2193+
case 3:
2194+
return "RESPONSE";
2195+
case 4:
2196+
return "RESPONSE_FINAL";
2197+
case 5:
2198+
return "REQUEST";
2199+
default:
2200+
return "???";
2201+
}
2202+
}
2203+
2204+
// Encode exactly 2 messages (A then B) into one buffer, then decode them sequentially reusing the SAME decoded object.
2205+
static void network_message_decode_pair_reuse(uint8_t a, uint8_t b, bool check_contents) {
2206+
printf("\n>> Pair %s(%u) -> %s(%u) (check: %s)\n", net_msg_name(a), a, net_msg_name(b), b,
2207+
check_contents ? "true" : "false");
2208+
2209+
_z_wbuf_t wbf = gen_wbuf(UINT16_MAX);
2210+
2211+
_z_network_message_t expected[2];
2212+
expected[0] = make_specific_net_msg(a);
2213+
expected[1] = make_specific_net_msg(b);
2214+
2215+
assert(_z_network_message_encode(&wbf, &expected[0]) == _Z_RES_OK);
2216+
assert(_z_network_message_encode(&wbf, &expected[1]) == _Z_RES_OK);
2217+
2218+
_z_zbuf_t zbf = _z_wbuf_to_zbuf(&wbf);
2219+
2220+
_z_network_message_t decoded = {0};
2221+
_z_arc_slice_t arcs = _z_arc_slice_empty();
2222+
2223+
for (int i = 0; i < 2; i++) {
2224+
_z_n_msg_clear(&decoded);
2225+
2226+
z_result_t r = _z_network_message_decode(&decoded, &zbf, &arcs, _Z_KEYEXPR_MAPPING_LOCAL);
2227+
assert(r == _Z_RES_OK);
2228+
2229+
if (check_contents) {
2230+
assert_eq_net_msg(&expected[i], &decoded);
2231+
}
2232+
}
2233+
2234+
_z_n_msg_clear(&decoded);
2235+
_z_n_msg_clear(&expected[0]);
2236+
_z_n_msg_clear(&expected[1]);
2237+
2238+
_z_zbuf_clear(&zbf);
2239+
_z_wbuf_clear(&wbf);
2240+
}
2241+
2242+
// 6x6 matrix: test all A->B transitions, one invocation per pair
2243+
static void network_message_decode_pairwise_matrix(bool check_contents, size_t itr) {
2244+
enum { TYPES = 6 };
2245+
2246+
printf(
2247+
"\n>> Network pairwise matrix "
2248+
"(2 messages per run), check: %s, iterations: %zu\n",
2249+
check_contents ? "true" : "false", itr);
2250+
2251+
for (size_t it = 0; it < itr; it++) {
2252+
printf("\n-- MATRIX ITERATION %zu --\n", it);
2253+
2254+
for (uint8_t a = 0; a < TYPES; a++) {
2255+
for (uint8_t b = 0; b < TYPES; b++) {
2256+
network_message_decode_pair_reuse(a, b, check_contents);
2257+
}
2258+
}
2259+
}
2260+
}
2261+
21352262
/*=============================*/
21362263
/* Main */
21372264
/*=============================*/
@@ -2192,6 +2319,10 @@ int main(void) {
21922319
test_crc_mismatch();
21932320
test_buffer_too_small();
21942321

2322+
// Ensure that we can decode a sequence of messages reusing the same decoded object
2323+
network_message_decode_pairwise_matrix(false, 1000);
2324+
network_message_decode_pairwise_matrix(true, 1000);
2325+
21952326
return 0;
21962327
}
21972328

0 commit comments

Comments
 (0)