From a74b684c48f31967a314ded6d3ca1bc7ada65bcd Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sun, 4 Oct 2026 23:59:29 +0000 Subject: [PATCH] fix(lib): keep a refused inbound packet endpoint-owned instead of leaking it (bug-1918) INVOKE_MSG_HANDLER stored NULL into ep->fsm_in.pkt after every msg handler. Handlers that take the packet clear the slot themselves (vtask_handle_msg, peer_ctrl_msg_handler); one that refuses it (-EINVAL / -ENOMEM) leaves it there, and the NULL store dropped the only reference. The slot is now left as the handler leaves it: a refused packet is reused for the next message or freed by endpoint_fini. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/include/netio.h | 11 ++-- src/lib/testsuite/test_vtask_pkt_handoff.c | 59 ++++++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/src/include/netio.h b/src/include/netio.h index abb9992e..fc41db7e 100644 --- a/src/include/netio.h +++ b/src/include/netio.h @@ -141,8 +141,8 @@ typedef struct endpoint_t { * via the vtask dispatcher, so the vtask_enq/-1 ref pair does not cover this * call). A handler whose own logic drops ep's last reference (e.g. a stale * vote reply driving raft_cb_become_follower -> endpoint_unref on the very ep - * currently being read) frees ep out from under the trailing pkt=NULL write - * below, a heap-use-after-free. Hold a ref for the duration of the call, same + * currently being read) frees ep out from under the macro's own trailing use + * of it, a heap-use-after-free. Hold a ref for the duration of the call, same * pattern as the vtask dispatch path, so the handler's own unref cannot drop * the count to zero until this macro is done touching ep. */ #define INVOKE_MSG_HANDLER(ep) \ @@ -150,9 +150,12 @@ typedef struct endpoint_t { TRACE("%s msg handler %p", __func__, (ep)->msg_handler); \ if ((ep)->msg_handler != NULL) { \ endpoint_ref(ep); \ + /* A handler that takes the packet clears ep->fsm_in.pkt itself \ + * (vtask_handle_msg, peer_ctrl_msg_handler). One that refuses it \ + * (early -EINVAL/-ENOMEM) leaves it endpoint-owned: the next \ + * message reuses it and endpoint_fini frees it. Overwriting the \ + * slot here leaked every refused packet (bug-1918). */ \ (ep)->msg_handler(ep); \ - /* passing ownership of packet to upper layer */ \ - (ep)->fsm_in.pkt = NULL; \ endpoint_unref(ep); \ } \ } while (0) diff --git a/src/lib/testsuite/test_vtask_pkt_handoff.c b/src/lib/testsuite/test_vtask_pkt_handoff.c index b05e76a4..5e59a303 100644 --- a/src/lib/testsuite/test_vtask_pkt_handoff.c +++ b/src/lib/testsuite/test_vtask_pkt_handoff.c @@ -189,9 +189,68 @@ static void test_handoff_rejects_incomplete_fsm(void) vtaskmgr_fini(&mgr); } +/* bug-1918: a msg handler that refuses the packet (an early -EINVAL/-ENOMEM + * exit, e.g. handle_msg with no owner context or vtask_handle_msg failing to + * allocate its task) leaves it in ep->fsm_in.pkt. INVOKE_MSG_HANDLER used to + * overwrite the slot with NULL after every call, leaking that packet; it must + * stay endpoint-owned so the next message reuses it or endpoint_fini frees it. + * A handler that takes the packet clears the slot itself (vtask_handle_msg, + * peer_ctrl_msg_handler), so the macro has nothing to clear. */ +static int refusing_msg_handler(endpoint_t* ep) +{ + (void)ep; + return -ENOMEM; +} + +static int taking_msg_handler(endpoint_t* ep) +{ + io_packet_t* pkt = ep->fsm_in.pkt; + + ep->fsm_in.pkt = NULL; + free_packet(pkt); + free(pkt); + return 0; +} + +static void test_refused_packet_stays_endpoint_owned(void) +{ + endpoint_t ep; + io_packet_t* pkt; + + memset(&ep, 0, sizeof(ep)); + TEST_ASSERT_TRUE(endpoint_init(&ep, AF_INET, SOCK_STREAM, 0, 0) >= 0); + + pkt = encode_nop(); + arm_inbound_done(&ep, pkt); + ep.msg_handler = (msg_handler_t)refusing_msg_handler; + INVOKE_MSG_HANDLER(&ep); + TEST_ASSERT_EQUAL_PTR_MESSAGE( + pkt, ep.fsm_in.pkt, + "bug-1918: a packet the handler refused must stay in the endpoint " + "slot (it was dropped from the slot and leaked)"); + + /* A handler that takes the packet leaves the slot empty. */ + ep.fsm_in.pkt = NULL; + free_packet(pkt); + free(pkt); + arm_inbound_done(&ep, encode_nop()); + ep.msg_handler = (msg_handler_t)taking_msg_handler; + INVOKE_MSG_HANDLER(&ep); + TEST_ASSERT_NULL(ep.fsm_in.pkt); + + /* Refused again: endpoint_fini frees the endpoint-owned packet. */ + arm_inbound_done(&ep, encode_nop()); + ep.msg_handler = (msg_handler_t)refusing_msg_handler; + INVOKE_MSG_HANDLER(&ep); + TEST_ASSERT_NOT_NULL(ep.fsm_in.pkt); + endpoint_fini(&ep); + TEST_ASSERT_NULL(ep.fsm_in.pkt); +} + void test_vtask_pkt_handoff(void) { RUN_TEST(test_endpoint_fini_after_vtask_freed_pkt); RUN_TEST(test_handoff_detaches_before_task_runs); RUN_TEST(test_handoff_rejects_incomplete_fsm); + RUN_TEST(test_refused_packet_stays_endpoint_owned); }