From 269f33bc21964635cde1c752387f186d279091a4 Mon Sep 17 00:00:00 2001 From: yavuzhaliloglu Date: Thu, 30 Apr 2026 11:45:25 +0300 Subject: [PATCH 1/3] Fix Q-Block re-entrancy in rate-limiting pacing --- include/coap3/coap_session_internal.h | 7 +++ src/coap_block.c | 62 +++++++++++++++++++++++---- src/coap_net.c | 2 +- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/include/coap3/coap_session_internal.h b/include/coap3/coap_session_internal.h index 4d52b2eb80..3fed8e27d7 100644 --- a/include/coap3/coap_session_internal.h +++ b/include/coap3/coap_session_internal.h @@ -261,6 +261,13 @@ struct coap_session_t { uint64_t rl_ticks_per_packet; /**< If not 0, rate limit NON to ticks per packet */ coap_tick_t last_tx; /**< Last time a ratelimited packet is sent */ uint8_t is_rate_limiting; /**< Currently NON rate limiting */ + uint8_t is_doing_q_block; /**< Currently processing a Q-Block response + handler (e.g. 4.08 retransmit, 2.31 + Continue triggering next MAX_PAYLOADS + set). Forces coap_send_internal() to + still apply pacing even though + is_rate_limiting is set by an outer + wait loop in a recursive io_process. */ uint32_t ping_failed; /**< Ping failure count */ }; diff --git a/src/coap_block.c b/src/coap_block.c index ecf634d4a5..80a302e50a 100644 --- a/src/coap_block.c +++ b/src/coap_block.c @@ -2245,6 +2245,16 @@ coap_block_check_q_block1_xmit(coap_session_t *session, coap_tick_t now, coap_ti int ret = 0; *tim_rem = COAP_MAX_DELAY_TICKS; + if (session->is_doing_q_block) { + /* + * A Q-Block send is already in flight on this session (e.g. an outer + * coap_send_q_blocks() iterating with rate-limit pacing, or a 4.08 / + * 2.31 response handler). Skip the timer-driven next-MAX_PAYLOAD-set + * push so we do not start an overlapping coap_send_q_blocks() for the + * same lg_xmit, which would re-send already-queued blocks. + */ + return ret; + } LL_FOREACH_SAFE(session->lg_xmit, lg_xmit, q) { coap_tick_t non_timeout = lg_xmit->non_timeout_random_ticks; @@ -2265,7 +2275,9 @@ coap_block_check_q_block1_xmit(coap_session_t *session, coap_tick_t now, coap_ti block.num = (uint32_t)(lg_xmit->offset / chunk); block.m = lg_xmit->offset + chunk < lg_xmit->data_info->length; block.szx = lg_xmit->blk_size; + session->is_doing_q_block = 1; coap_send_q_blocks(session, lg_xmit, block, lg_xmit->sent_pdu, COAP_SEND_SKIP_PDU); + session->is_doing_q_block = 0; if (*tim_rem > non_timeout) { *tim_rem = non_timeout; ret = 1; @@ -2309,6 +2321,11 @@ coap_block_check_q_block2_xmit(coap_session_t *session, coap_tick_t now, coap_ti int ret = 0; *tim_rem = (coap_tick_t)-1; + if (session->is_doing_q_block) { + /* See coap_block_check_q_block1_xmit() — skip while a Q-Block send is + already in flight on this session to avoid overlapping sends. */ + return ret; + } LL_FOREACH_SAFE(session->lg_xmit, lg_xmit, q) { coap_tick_t non_timeout = lg_xmit->non_timeout_random_ticks; @@ -2329,8 +2346,11 @@ coap_block_check_q_block2_xmit(coap_session_t *session, coap_tick_t now, coap_ti block.num = (uint32_t)(lg_xmit->offset / chunk); block.m = lg_xmit->offset + chunk < lg_xmit->data_info->length; block.szx = lg_xmit->blk_size; - if (block.num == (uint32_t)lg_xmit->last_block) + if (block.num == (uint32_t)lg_xmit->last_block) { + session->is_doing_q_block = 1; coap_send_q_blocks(session, lg_xmit, block, lg_xmit->sent_pdu, COAP_SEND_SKIP_PDU); + session->is_doing_q_block = 0; + } if (*tim_rem > non_timeout) { *tim_rem = non_timeout; ret = 1; @@ -3908,8 +3928,14 @@ coap_handle_response_send_block(coap_session_t *session, coap_pdu_t *sent, #if COAP_Q_BLOCK_SUPPORT if (lg_xmit->option == COAP_OPTION_Q_BLOCK1 && pdu->type == COAP_MESSAGE_NON) { - if (coap_send_q_block1(session, block, pdu, - COAP_SEND_INC_PDU) == COAP_INVALID_MID) + coap_mid_t q_mid; + /* Save/restore — see 4.08 handler below for rationale. */ + uint8_t saved_q_block = session->is_doing_q_block; + + session->is_doing_q_block = 1; + q_mid = coap_send_q_block1(session, block, pdu, COAP_SEND_INC_PDU); + session->is_doing_q_block = saved_q_block; + if (q_mid == COAP_INVALID_MID) goto fail_body; return 1; } else if (coap_send_internal(session, pdu, NULL) == COAP_INVALID_MID) @@ -3990,15 +4016,25 @@ coap_handle_response_send_block(coap_session_t *session, coap_pdu_t *sent, uint64_t token; uint8_t ltoken[8]; size_t ltoken_length; + /* + * Save/restore is_doing_q_block: this handler can run nested + * inside an outer coap_send_q_blocks() (timer-driven next + * MAX_PAYLOAD set, etc.) that already set the flag. Plain + * set-then-clear would zero the outer's flag on our exit and + * let coap_block_check_q_block1_xmit() spawn an overlapping + * coap_send_q_blocks() while the outer is still iterating. + */ + uint8_t saved_q_block = session->is_doing_q_block; + session->is_doing_q_block = 1; for (i = 0; (bp < data + length) && i < COAP_MAX_PAYLOADS(session); i++) { if ((*bp & 0xc0) != 0x00) /* uint(value) */ - goto fail_cbor; + goto fail_cbor_q_block; block.num = derive_cbor_value(&bp, data + length - bp); coap_log_debug("Q-Block1: Missing block %d\n", block.num); if (block.num > (1 << 20) -1) - goto fail_cbor; + goto fail_cbor_q_block; block.m = (block.num + 1) * chunk < lg_xmit->data_info->length; block.szx = lg_xmit->blk_size; @@ -4007,8 +4043,10 @@ coap_handle_response_send_block(coap_session_t *session, coap_pdu_t *sent, ltoken_length = coap_encode_var_safe8(ltoken, sizeof(token), token); pdu = coap_pdu_duplicate_lkd(lg_xmit->sent_pdu, session, ltoken_length, ltoken, NULL, COAP_BOOL_FALSE); - if (!pdu) + if (!pdu) { + session->is_doing_q_block = saved_q_block; goto fail_body; + } coap_update_option(pdu, lg_xmit->option, coap_encode_var_safe(buf, sizeof(buf), @@ -4021,12 +4059,20 @@ coap_handle_response_send_block(coap_session_t *session, coap_pdu_t *sent, lg_xmit->data_info->length, lg_xmit->data_info->data, block.num, - block.szx)) + block.szx)) { + session->is_doing_q_block = saved_q_block; goto fail_body; - if (coap_send_internal(session, pdu, NULL) == COAP_INVALID_MID) + } + if (coap_send_internal(session, pdu, NULL) == COAP_INVALID_MID) { + session->is_doing_q_block = saved_q_block; goto fail_body; + } } + session->is_doing_q_block = saved_q_block; return 1; +fail_cbor_q_block: + session->is_doing_q_block = saved_q_block; + goto fail_cbor; } fail_cbor: coap_log_info("Invalid application/missing-blocks+cbor-seq\n"); diff --git a/src/coap_net.c b/src/coap_net.c index edd4364ce8..7fc48c497e 100644 --- a/src/coap_net.c +++ b/src/coap_net.c @@ -2044,7 +2044,7 @@ coap_send_internal(coap_session_t *session, coap_pdu_t *pdu, coap_pdu_t *request if (pdu->type == COAP_MESSAGE_NON && session->rl_ticks_per_packet) { coap_tick_t now; - if (!session->is_rate_limiting) { + if (!session->is_rate_limiting || session->is_doing_q_block) { coap_ticks(&now); while (1) { uint32_t timeout_ms; From b63a789e84b3a0c70deb3817dc83586408697efc Mon Sep 17 00:00:00 2001 From: yavuz <77507602+yavuzhaliloglu@users.noreply.github.com> Date: Thu, 30 Apr 2026 13:05:40 +0300 Subject: [PATCH 2/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- include/coap3/coap_session_internal.h | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/include/coap3/coap_session_internal.h b/include/coap3/coap_session_internal.h index 3fed8e27d7..dc09d92874 100644 --- a/include/coap3/coap_session_internal.h +++ b/include/coap3/coap_session_internal.h @@ -261,13 +261,15 @@ struct coap_session_t { uint64_t rl_ticks_per_packet; /**< If not 0, rate limit NON to ticks per packet */ coap_tick_t last_tx; /**< Last time a ratelimited packet is sent */ uint8_t is_rate_limiting; /**< Currently NON rate limiting */ - uint8_t is_doing_q_block; /**< Currently processing a Q-Block response - handler (e.g. 4.08 retransmit, 2.31 - Continue triggering next MAX_PAYLOADS - set). Forces coap_send_internal() to - still apply pacing even though - is_rate_limiting is set by an outer - wait loop in a recursive io_process. */ + uint8_t is_doing_q_block; /**< Q-Block send/response processing is in + progress on this session (e.g. 4.08 + retransmit, 2.31 Continue triggering + next MAX_PAYLOADS set, or timer-driven + Q-Block restart sends). Forces + coap_send_internal() to still apply + pacing even though is_rate_limiting is + set by an outer wait loop in a + recursive io_process. */ uint32_t ping_failed; /**< Ping failure count */ }; From 80416e1d7d8b4e9d03ecb37bbf572baefe25c96e Mon Sep 17 00:00:00 2001 From: yavuzhaliloglu Date: Thu, 30 Apr 2026 13:02:56 +0300 Subject: [PATCH 3/3] Save and restore is_rate_limiting in pacing loop to prevent re-entrancy issue When coap_send_internal() is called recursively from a Q-Block response handler (4.08, 2.31), the inner pacing loop would clear is_rate_limiting to 0 on exit, clobbering the outer frame's state. Save the previous value at entry and restore it at exit so nested invocations don't corrupt the outer wait loop's guard. Addresses Copilot review feedback on PR #1973. --- src/coap_net.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coap_net.c b/src/coap_net.c index 7fc48c497e..4267994fd4 100644 --- a/src/coap_net.c +++ b/src/coap_net.c @@ -2058,9 +2058,10 @@ coap_send_internal(coap_session_t *session, coap_pdu_t *pdu, coap_pdu_t *request if (timeout_ms == 0) { timeout_ms = COAP_IO_NO_WAIT; } + uint8_t prev_rate_limiting = session->is_rate_limiting; session->is_rate_limiting = 1; coap_io_process_lkd(session->context, timeout_ms); - session->is_rate_limiting = 0; + session->is_rate_limiting = prev_rate_limiting; coap_ticks(&now); } session->last_tx = now;