Skip to content

Fix stale CASE exchange blocking new session handshake #407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jonlil wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix stale CASE exchange blocking new session handshake #407

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30dd689
Fix stale CASE exchange blocking new session handshake
jonlil Apr 11, 2026
4d7e717
Fix panic in remove_exch when exchange already evicted
jonlil Apr 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions rs-matter/src/transport/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,33 +312,38 @@ impl Session {

let exch_index = self.get_exch_for_rx(&rx_header.proto);
if let Some(exch_index) = exch_index {
let exch = unwrap!(self.exchanges[exch_index].as_mut());
// If we receive a new session request (CASESigma1/PBKDFParamRequest) on an
// existing exchange, the peer has restarted the handshake. Remove the stale
// exchange (or mark as dropped if MRP is pending) and create a fresh one.
if MessageMeta::from(&rx_header.proto).is_new_session() {
warn!("New session request on existing exchange — peer restarted handshake, evicting stale exchange");
self.remove_exch(exch_index);
// Fall through to create a new exchange below
Comment on lines +318 to +321
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Instead of manually setting the dropped state, consider using self.remove_exch(exch_index). This method is more idiomatic as it attempts to cleanly remove the exchange from the session's exchange table, or marks it as dropped if there are pending MRP operations (ACKs or retransmissions). This also increases the chance of freeing a slot in the exchange table for the new handshake if the stale exchange can be removed immediately.

Suggested change
if MessageMeta::from(&rx_header.proto).is_new_session() {
warn!("New session request on existing exchange — peer restarted handshake, evicting stale exchange");
let exch = unwrap!(self.exchanges[exch_index].as_mut());
exch.role.set_dropped_state();
// Fall through to create a new exchange below
if MessageMeta::from(&rx_header.proto).is_new_session() {
warn!("New session request on existing exchange — peer restarted handshake, evicting stale exchange");
self.remove_exch(exch_index);
// Fall through to create a new exchange below

Copy link
Copy Markdown
Contributor Author

@jonlil jonlil Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion — updated to use remove_exch() which either clears the slot immediately or marks as Dropped if MRP is pending. This is cleaner and frees the slot for the new exchange when possible.

} else {
let exch = unwrap!(self.exchanges[exch_index].as_mut());

exch.post_recv(&rx_header.plain, &rx_header.proto, epoch)?;
exch.post_recv(&rx_header.plain, &rx_header.proto, epoch)?;

Ok(false)
} else {
if !rx_header.proto.is_initiator()
|| !MessageMeta::from(&rx_header.proto).is_new_exchange()
{
// Do not create a new exchange if the peer is not an initiator, or if
// the packet is NOT a candidate for a new exchange
// (i.e. it is a standalone ACK or a SC status response)
Err(ErrorCode::NoExchange)?;
return Ok(false);
}
Comment on lines +322 to 328
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a significant routing issue here. If get_exch_for_rx returns a stale exchange that was marked as dropped (but not removed from the table), this else block will be executed for subsequent packets in the handshake (e.g., CASESigma3). This will cause the packet to be processed by the stale exchange instead of the new one created for the restarted handshake. To fully address this, get_exch_for_rx should be updated to ignore exchanges in the dropped state, ensuring that the transport can find the active exchange even if a stale one with the same ID still exists in the table.

Copy link
Copy Markdown
Contributor Author

@jonlil jonlil Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed this is a valid concern. With the updated code using remove_exch(), the slot is cleared immediately in the common case (no pending MRP at CASESigma1 stage). This means get_exch_for_rx won't find the stale exchange on subsequent packets.

In the rare edge case where MRP is pending and the exchange is only marked as Dropped (not removed), the stale exchange could theoretically shadow the new one. However, at the CASESigma1 stage no application data has been exchanged yet, so there should be no pending retransmissions or ACKs to delay removal.

}

if let Some(exch_index) =
self.add_exch(rx_header.proto.exch_id, Role::Responder(Default::default()))
{
// unwrap is safe as we just created the exchange
let exch = unwrap!(self.exchanges[exch_index].as_mut());
// Create a new exchange for this message (new session request, or no existing exchange)
if !rx_header.proto.is_initiator() || !MessageMeta::from(&rx_header.proto).is_new_exchange()
{
Err(ErrorCode::NoExchange)?;
}

exch.post_recv(&rx_header.plain, &rx_header.proto, epoch)?;
if let Some(exch_index) =
self.add_exch(rx_header.proto.exch_id, Role::Responder(Default::default()))
{
let exch = unwrap!(self.exchanges[exch_index].as_mut());

Ok(true)
} else {
Err(ErrorCode::NoSpaceExchanges)?
}
exch.post_recv(&rx_header.plain, &rx_header.proto, epoch)?;

Ok(true)
} else {
Err(ErrorCode::NoSpaceExchanges)?
}
}

Expand Down Expand Up @@ -469,7 +474,10 @@ impl Session {
}

pub(crate) fn remove_exch(&mut self, index: usize) -> bool {
let exchange = unwrap!(self.exchanges[index].as_mut());
let Some(exchange) = self.exchanges[index].as_mut() else {
// Already removed (e.g. evicted by stale-exchange handling before Drop ran)
return true;
};
let exchange_id = ExchangeId::new(self.id, index);

if exchange.mrp.is_retrans_pending() {
Expand Down
Loading