evie/rs-matter
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #82 from ivmarkov/main

Browse source 
Do not hold on to RefCell borrows across await points
This commit is contained in:
Kedar Sovani 2023-08-01 14:00:49 +05:30 • committed by GitHub
commit 50f18dbbee
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 210 additions and 227 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
.github/workflows/ci.yml vendored
View file

@ -10,7 +10,7 @@ on:
workflow_dispatch: workflow_dispatch:
env: env:
RUST_TOOLCHAIN: nightly-2023-07-01 RUST_TOOLCHAIN: nightly
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
CARGO_TERM_COLOR: always CARGO_TERM_COLOR: always

2
rs-matter/src/mdns/builtin.rs
View file

@ -208,7 +208,6 @@ impl<'a> MdnsService<'a> {
select(&mut broadcast, &mut respond).await.unwrap() select(&mut broadcast, &mut respond).await.unwrap()
} }
#[allow(clippy::await_holding_refcell_ref)]
async fn broadcast(&self, tx_pipe: &Pipe<'_>) -> Result<(), Error> { async fn broadcast(&self, tx_pipe: &Pipe<'_>) -> Result<(), Error> {
loop { loop {
select( select(
@ -258,7 +257,6 @@ impl<'a> MdnsService<'a> {
} }
} }
#[allow(clippy::await_holding_refcell_ref)]
async fn respond(&self, rx_pipe: &Pipe<'_>, tx_pipe: &Pipe<'_>) -> Result<(), Error> { async fn respond(&self, rx_pipe: &Pipe<'_>, tx_pipe: &Pipe<'_>) -> Result<(), Error> {
loop { loop {
{ {

104
rs-matter/src/secure_channel/case.rs
View file

@ -87,7 +87,6 @@ impl<'a> Case<'a> {
self.handle_casesigma3(exchange, rx, tx, &mut session).await self.handle_casesigma3(exchange, rx, tx, &mut session).await
} }
#[allow(clippy::await_holding_refcell_ref)]
async fn handle_casesigma3( async fn handle_casesigma3(
&mut self, &mut self,
exchange: &mut Exchange<'_>, exchange: &mut Exchange<'_>,
@ -97,23 +96,11 @@ impl<'a> Case<'a> {
) -> Result<(), Error> { ) -> Result<(), Error> {
rx.check_proto_opcode(OpCode::CASESigma3 as _)?; rx.check_proto_opcode(OpCode::CASESigma3 as _)?;
let status = {
let fabric_mgr = self.fabric_mgr.borrow(); let fabric_mgr = self.fabric_mgr.borrow();
let fabric = fabric_mgr.get_fabric(case_session.local_fabric_idx)?; let fabric = fabric_mgr.get_fabric(case_session.local_fabric_idx)?;
if fabric.is_none() { if let Some(fabric) = fabric {
drop(fabric_mgr);
complete_with_status(
exchange,
tx,
common::SCStatusCodes::NoSharedTrustRoots,
None,
)
.await?;
return Ok(());
}
// Safe to unwrap here
let fabric = fabric.unwrap();
let root = get_root_node_struct(rx.as_slice())?; let root = get_root_node_struct(rx.as_slice())?;
let encrypted = root.find_tag(1)?.slice()?; let encrypted = root.find_tag(1)?.slice()?;
@ -125,7 +112,8 @@ impl<'a> Case<'a> {
let decrypted = &mut decrypted[..encrypted.len()]; let decrypted = &mut decrypted[..encrypted.len()];
decrypted.copy_from_slice(encrypted); decrypted.copy_from_slice(encrypted);
let len = Case::get_sigma3_decryption(fabric.ipk.op_key(), case_session, decrypted)?; let len =
Case::get_sigma3_decryption(fabric.ipk.op_key(), case_session, decrypted)?;
let decrypted = &decrypted[..len]; let decrypted = &decrypted[..len];
let root = get_root_node_struct(decrypted)?; let root = get_root_node_struct(decrypted)?;
@ -145,26 +133,17 @@ impl<'a> Case<'a> {
if let Err(e) = Case::validate_certs(fabric, &initiator_noc, initiator_icac_mut) { if let Err(e) = Case::validate_certs(fabric, &initiator_noc, initiator_icac_mut) {
error!("Certificate Chain doesn't match: {}", e); error!("Certificate Chain doesn't match: {}", e);
complete_with_status(exchange, tx, common::SCStatusCodes::InvalidParameter, None) SCStatusCodes::InvalidParameter
.await?; } else if let Err(e) = Case::validate_sigma3_sign(
return Ok(());
}
if Case::validate_sigma3_sign(
d.initiator_noc.0, d.initiator_noc.0,
d.initiator_icac.map(|a| a.0), d.initiator_icac.map(|a| a.0),
&initiator_noc, &initiator_noc,
d.signature.0, d.signature.0,
case_session, case_session,
) ) {
.is_err() error!("Sigma3 Signature doesn't match: {}", e);
{ SCStatusCodes::InvalidParameter
error!("Sigma3 Signature doesn't match"); } else {
complete_with_status(exchange, tx, common::SCStatusCodes::InvalidParameter, None)
.await?;
return Ok(());
}
// Only now do we add this message to the TT Hash // Only now do we add this message to the TT Hash
let mut peer_catids: NocCatIds = Default::default(); let mut peer_catids: NocCatIds = Default::default();
initiator_noc.get_cat_ids(&mut peer_catids); initiator_noc.get_cat_ids(&mut peer_catids);
@ -179,18 +158,19 @@ impl<'a> Case<'a> {
)?; )?;
// TODO: Handle NoSpace // TODO: Handle NoSpace
exchange.with_session_mgr_mut(|sess_mgr| sess_mgr.clone_session(&clone_data))?; exchange
.with_session_mgr_mut(|sess_mgr| sess_mgr.clone_session(&clone_data))?;
complete_with_status( SCStatusCodes::SessionEstablishmentSuccess
exchange, }
tx, } else {
SCStatusCodes::SessionEstablishmentSuccess, SCStatusCodes::NoSharedTrustRoots
None, }
) };
.await
complete_with_status(exchange, tx, status, None).await
} }
#[allow(clippy::await_holding_refcell_ref)]
async fn handle_casesigma1( async fn handle_casesigma1(
&mut self, &mut self,
exchange: &mut Exchange<'_>, exchange: &mut Exchange<'_>,
@ -255,23 +235,13 @@ impl<'a> Case<'a> {
const MAX_ENCRYPTED_SIZE: usize = 800; const MAX_ENCRYPTED_SIZE: usize = 800;
let mut encrypted = alloc!([0; MAX_ENCRYPTED_SIZE]); let mut encrypted = alloc!([0; MAX_ENCRYPTED_SIZE]);
let encrypted_len = {
let mut signature = alloc!([0u8; crypto::EC_SIGNATURE_LEN_BYTES]); let mut signature = alloc!([0u8; crypto::EC_SIGNATURE_LEN_BYTES]);
let fabric_found = {
let fabric_mgr = self.fabric_mgr.borrow(); let fabric_mgr = self.fabric_mgr.borrow();
let fabric = fabric_mgr.get_fabric(case_session.local_fabric_idx)?; let fabric = fabric_mgr.get_fabric(case_session.local_fabric_idx)?;
if fabric.is_none() { if let Some(fabric) = fabric {
drop(fabric_mgr);
complete_with_status(
exchange,
tx,
common::SCStatusCodes::NoSharedTrustRoots,
None,
)
.await?;
return Ok(());
}
#[cfg(feature = "alloc")] #[cfg(feature = "alloc")]
let signature_mut = &mut *signature; let signature_mut = &mut *signature;
@ -279,7 +249,7 @@ impl<'a> Case<'a> {
let signature_mut = &mut signature; let signature_mut = &mut signature;
let sign_len = Case::get_sigma2_sign( let sign_len = Case::get_sigma2_sign(
fabric.unwrap(), fabric,
&case_session.our_pub_key, &case_session.our_pub_key,
&case_session.peer_pub_key, &case_session.peer_pub_key,
signature_mut, signature_mut,
@ -292,15 +262,15 @@ impl<'a> Case<'a> {
#[cfg(not(feature = "alloc"))] #[cfg(not(feature = "alloc"))]
let encrypted_mut = &mut encrypted; let encrypted_mut = &mut encrypted;
Case::get_sigma2_encryption( let encrypted_len = Case::get_sigma2_encryption(
fabric.unwrap(), fabric,
self.rand, self.rand,
&our_random, &our_random,
case_session, case_session,
signature, signature,
encrypted_mut, encrypted_mut,
)? )?;
};
let encrypted = &encrypted[0..encrypted_len]; let encrypted = &encrypted[0..encrypted_len];
// Generate our Response Body // Generate our Response Body
@ -318,7 +288,23 @@ impl<'a> Case<'a> {
case_session.tt_hash.update(tx.as_mut_slice())?; case_session.tt_hash.update(tx.as_mut_slice())?;
true
} else {
false
}
};
if fabric_found {
exchange.exchange(tx, rx).await exchange.exchange(tx, rx).await
} else {
complete_with_status(
exchange,
tx,
common::SCStatusCodes::NoSharedTrustRoots,
None,
)
.await
}
} }
fn get_session_clone_data( fn get_session_clone_data(
@ -515,7 +501,7 @@ impl<'a> Case<'a> {
fabric: &Fabric, fabric: &Fabric,
rand: Rand, rand: Rand,
our_random: &[u8], our_random: &[u8],
case_session: &mut CaseSession, case_session: &CaseSession,
signature: &[u8], signature: &[u8],
out: &mut [u8], out: &mut [u8],
) -> Result<usize, Error> { ) -> Result<usize, Error> {

8
rs-matter/src/secure_channel/crypto_mbedtls.rs
View file

@ -186,7 +186,7 @@ impl CryptoSpake2 {
let (Z, V) = Self::get_ZV_as_verifier( let (Z, V) = Self::get_ZV_as_verifier(
&self.w0, &self.w0,
&self.L, &self.L,
&mut self.M, &self.M,
&X, &X,
&self.xy, &self.xy,
&self.order, &self.order,
@ -228,7 +228,7 @@ impl CryptoSpake2 {
fn get_ZV_as_prover( fn get_ZV_as_prover(
w0: &Mpi, w0: &Mpi,
w1: &Mpi, w1: &Mpi,
N: &mut EcPoint, N: &EcPoint,
Y: &EcPoint, Y: &EcPoint,
x: &Mpi, x: &Mpi,
order: &Mpi, order: &Mpi,
@ -264,7 +264,7 @@ impl CryptoSpake2 {
fn get_ZV_as_verifier( fn get_ZV_as_verifier(
w0: &Mpi, w0: &Mpi,
L: &EcPoint, L: &EcPoint,
M: &mut EcPoint, M: &EcPoint,
X: &EcPoint, X: &EcPoint,
y: &Mpi, y: &Mpi,
order: &Mpi, order: &Mpi,
@ -292,7 +292,7 @@ impl CryptoSpake2 {
Ok((Z, V)) Ok((Z, V))
} }
fn invert(group: &mut EcGroup, num: &EcPoint) -> Result<EcPoint, mbedtls::Error> { fn invert(group: &EcGroup, num: &EcPoint) -> Result<EcPoint, mbedtls::Error> {
let p = group.p()?; let p = group.p()?;
let num_y = num.y()?; let num_y = num.y()?;
let inverted_num_y = p.sub(&num_y)?; let inverted_num_y = p.sub(&num_y)?;

18
rs-matter/src/secure_channel/pake.rs
View file

@ -213,7 +213,6 @@ impl<'a> Pake<'a> {
} }
#[allow(non_snake_case)] #[allow(non_snake_case)]
#[allow(clippy::await_holding_refcell_ref)]
async fn handle_pasepake1( async fn handle_pasepake1(
&mut self, &mut self,
exchange: &mut Exchange<'_>, exchange: &mut Exchange<'_>,
@ -224,6 +223,7 @@ impl<'a> Pake<'a> {
rx.check_proto_opcode(OpCode::PASEPake1 as _)?; rx.check_proto_opcode(OpCode::PASEPake1 as _)?;
self.update_timeout(exchange, tx, false).await?; self.update_timeout(exchange, tx, false).await?;
{
let pase = self.pase.borrow(); let pase = self.pase.borrow();
let session = pase.session.as_ref().ok_or(ErrorCode::NoSession)?; let session = pase.session.as_ref().ok_or(ErrorCode::NoSession)?;
@ -244,12 +244,11 @@ impl<'a> Pake<'a> {
cb: OctetStr(&cB), cb: OctetStr(&cB),
}; };
resp.to_tlv(&mut tw, TagType::Anonymous)?; resp.to_tlv(&mut tw, TagType::Anonymous)?;
}
drop(pase);
exchange.exchange(tx, rx).await exchange.exchange(tx, rx).await
} }
#[allow(clippy::await_holding_refcell_ref)]
async fn handle_pbkdfparamrequest( async fn handle_pbkdfparamrequest(
&mut self, &mut self,
exchange: &mut Exchange<'_>, exchange: &mut Exchange<'_>,
@ -260,6 +259,7 @@ impl<'a> Pake<'a> {
rx.check_proto_opcode(OpCode::PBKDFParamRequest as _)?; rx.check_proto_opcode(OpCode::PBKDFParamRequest as _)?;
self.update_timeout(exchange, tx, true).await?; self.update_timeout(exchange, tx, true).await?;
{
let pase = self.pase.borrow(); let pase = self.pase.borrow();
let session = pase.session.as_ref().ok_or(ErrorCode::NoSession)?; let session = pase.session.as_ref().ok_or(ErrorCode::NoSession)?;
@ -299,13 +299,11 @@ impl<'a> Pake<'a> {
resp.to_tlv(&mut tw, TagType::Anonymous)?; resp.to_tlv(&mut tw, TagType::Anonymous)?;
spake2p.set_context(rx.as_slice(), tx.as_mut_slice())?; spake2p.set_context(rx.as_slice(), tx.as_mut_slice())?;
}
drop(pase);
exchange.exchange(tx, rx).await exchange.exchange(tx, rx).await
} }
#[allow(clippy::await_holding_refcell_ref)]
async fn update_timeout( async fn update_timeout(
&mut self, &mut self,
exchange: &mut Exchange<'_>, exchange: &mut Exchange<'_>,
@ -314,6 +312,7 @@ impl<'a> Pake<'a> {
) -> Result<(), Error> { ) -> Result<(), Error> {
self.check_session(exchange, tx).await?; self.check_session(exchange, tx).await?;
let status = {
let mut pase = self.pase.borrow_mut(); let mut pase = self.pase.borrow_mut();
if pase if pase
@ -325,7 +324,7 @@ impl<'a> Pake<'a> {
pase.timeout = None; pase.timeout = None;
} }
let status = if let Some(sd) = pase.timeout.as_mut() { if let Some(sd) = pase.timeout.as_mut() {
if &sd.exch_id != exchange.id() { if &sd.exch_id != exchange.id() {
info!("Other PAKE session in progress"); info!("Other PAKE session in progress");
Some(SCStatusCodes::Busy) Some(SCStatusCodes::Busy)
@ -337,13 +336,14 @@ impl<'a> Pake<'a> {
} else { } else {
error!("PAKE session not found or expired"); error!("PAKE session not found or expired");
Some(SCStatusCodes::SessionNotFound) Some(SCStatusCodes::SessionNotFound)
}
}; };
if let Some(status) = status { if let Some(status) = status {
drop(pase);
complete_with_status(exchange, tx, status, None).await complete_with_status(exchange, tx, status, None).await
} else { } else {
let mut pase = self.pase.borrow_mut();
pase.timeout = Some(Timeout::new(exchange, pase.epoch)); pase.timeout = Some(Timeout::new(exchange, pase.epoch));
Ok(()) Ok(())

1
rs-matter/src/transport/core.rs
View file

@ -397,7 +397,6 @@ impl<'a> Matter<'a> {
} }
#[inline(always)] #[inline(always)]
#[cfg_attr(feature = "nightly", allow(clippy::await_holding_refcell_ref))] // Fine because of the async mutex
pub async fn handle_exchange<H>( pub async fn handle_exchange<H>(
&self, &self,
tx_buf: &mut [u8; MAX_TX_BUF_SIZE], tx_buf: &mut [u8; MAX_TX_BUF_SIZE],