Skip to content

Commit

Permalink
fix(coap): Apply check and clippy lints
Browse files Browse the repository at this point in the history
  • Loading branch information
chrysn committed Oct 30, 2024
1 parent c784e76 commit 7d0603d
Show file tree
Hide file tree
Showing 7 changed files with 90 additions and 59 deletions.
2 changes: 1 addition & 1 deletion clippy.toml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Require SAFETY docs, as well as a few other lints, for private items
check-private-items = true

doc-valid-idents = ["MHz", "GHz", "THz", ".."]
doc-valid-idents = ["CoAP", "MHz", "GHz", "THz", ".."]
6 changes: 2 additions & 4 deletions examples/coap/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
#![feature(impl_trait_in_assoc_type)]
#![feature(used_with_arg)]

use riot_rs::{debug::log::*, network};

// because coapcore depends on it temporarily
extern crate alloc;
use static_alloc::Bump;
Expand All @@ -14,7 +12,7 @@ static A: Bump<[u8; 1 << 16]> = Bump::uninit();

#[riot_rs::task(autostart)]
async fn coap_run() {
use coap_handler_implementations::{HandlerBuilder, ReportingHandlerBuilder};
use coap_handler_implementations::HandlerBuilder;

let log = None;
let buffer = scroll_ring::Buffer::<512>::default();
Expand Down Expand Up @@ -54,7 +52,7 @@ async fn run_client_operations(mut stdout: impl core::fmt::Write) {

// shame
let addr = "10.42.0.1:1234";
let demoserver = addr.clone().parse().unwrap();
let demoserver = addr.parse().unwrap();

use coap_request::Stack;
writeln!(stdout, "Sending GET to {}...", addr).unwrap();
Expand Down
1 change: 1 addition & 0 deletions src/lib/coapcore/src/oluru.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
//! indicate numeric values (including of priorities, where "high" corresponds to "small" and "low"
//! to "large").
#![forbid(unsafe_code)]
#![expect(clippy::indexing_slicing, reason = "module is scheduled for overhaul")]

use arrayvec::ArrayVec;

Expand Down
58 changes: 31 additions & 27 deletions src/lib/coapcore/src/seccontext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ impl COwn {
}
}

impl Into<lakers::ConnId> for COwn {
fn into(self) -> lakers::ConnId {
lakers::ConnId::from_int_raw(self.0)
impl From<COwn> for lakers::ConnId {
fn from(cown: COwn) -> Self {
lakers::ConnId::from_int_raw(cown.0)
}
}

Expand Down Expand Up @@ -106,9 +106,9 @@ impl AifStaticRest {
.is_some_and(|o| o.value() == b"stdout")
&& uri_path_options.next().is_none()
{
return self.may_use_stdout;
self.may_use_stdout
} else {
return true;
true
}
}
}
Expand All @@ -134,6 +134,10 @@ impl<Crypto: lakers::Crypto> Default for SecContextState<Crypto> {
}

#[derive(Debug)]
#[expect(
clippy::large_enum_variant,
reason = "requiring more memory during connection setup is expected, but the complexity of an inhmogenous pool is currently impractical"
)]
enum SecContextStage<Crypto: lakers::Crypto> {
Empty,

Expand Down Expand Up @@ -479,22 +483,21 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
return Err(Own(CoAPError::method_not_allowed()));
}

let first_byte = request
let (first_byte, edhoc_m1) = request
.payload()
.get(0)
.split_first()
.ok_or_else(CoAPError::bad_request)?;
let starts_with_true = first_byte == &0xf5;

if starts_with_true {
info!("Processing incoming EDHOC message 1");
let message_1 =
&lakers::EdhocMessageBuffer::new_from_slice(&request.payload()[1..])
.map_err(too_small)?;
&lakers::EdhocMessageBuffer::new_from_slice(edhoc_m1).map_err(too_small)?;

let (responder, c_i, ead_1) = lakers::EdhocResponder::new(
(self.crypto_factory)(),
&self.own_identity.1,
self.own_identity.0.clone(),
self.own_identity.1,
*self.own_identity.0,
)
.process_message_1(message_1)
.map_err(render_error)?;
Expand All @@ -512,12 +515,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
.filter_map(|entry| entry.corresponding_cown())
// C_R does not only need to be unique, it also must not be identical
// to C_I
.chain(
COwn::from_kid(c_i.as_slice())
.as_slice()
.into_iter()
.cloned(),
),
.chain(COwn::from_kid(c_i.as_slice()).as_slice().iter().cloned()),
);

debug!("Entries in pool:");
Expand Down Expand Up @@ -559,10 +557,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
// isn't processable, it's unlikely that another one would come up and be.
let mut taken = self
.pool
.lookup(
|c| c.corresponding_cown() == Some(kid),
|matched| core::mem::replace(matched, Default::default()),
)
.lookup(|c| c.corresponding_cown() == Some(kid), core::mem::take)
// following RFC8613 Section 8.2 item 2.2
// FIXME unauthorized (unreleased in coap-message-utils)
.ok_or_else(CoAPError::bad_request)?;
Expand All @@ -589,6 +584,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
} = taken
{
debug_assert_eq!(c_r, kid, "State was looked up by KID");
#[allow(clippy::indexing_slicing, reason = "slice fits by construction")]
let msg_3 = lakers::EdhocMessageBuffer::new_from_slice(&payload[..cutoff])
.map_err(|e| Own(too_small(e)))?;

Expand Down Expand Up @@ -653,7 +649,13 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
let oscore_salt = responder.edhoc_exporter(1u8, &[], 8); // label is 1
let oscore_secret = &oscore_secret[..16];
let oscore_salt = &oscore_salt[..8];
debug!("OSCORE secret: {:?}...", &oscore_secret[..5]);
#[allow(
clippy::indexing_slicing,
reason = "secret necessarily contains more than 40 bits"
)]
{
debug!("OSCORE secret: {:?}...", &oscore_secret[..5]);
}
debug!("OSCORE salt: {:?}", &oscore_salt);

let sender_id = c_i.as_slice();
Expand All @@ -665,8 +667,8 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler

let immutables = liboscore::PrimitiveImmutables::derive(
hkdf,
&oscore_secret,
&oscore_salt,
oscore_secret,
oscore_salt,
None,
aead,
sender_id,
Expand Down Expand Up @@ -740,6 +742,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
let oscore_option = oscore_option.unwrap();
let oscore_option = liboscore::OscoreOption::parse(&oscore_option)
.map_err(|_| CoAPError::bad_option(coap_numbers::option::OSCORE))?;
#[allow(clippy::indexing_slicing, reason = "slice fits by construction")]
allocated_message
.set_payload(&payload[front_trim_payload..])
.unwrap();
Expand Down Expand Up @@ -795,7 +798,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
) -> Result<(), Self::BuildResponseError<M>> {
use OrInner::{Inner, Own};

Ok(match req {
match req {
Own(EdhocResponse::OkSend2(c_r)) => {
// FIXME: Why does the From<O> not do the map_err?
response.set_code(
Expand All @@ -807,7 +810,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
|matched| {
// temporary default will not live long (and may be only constructed if
// prepare_message_2 fails)
let taken = core::mem::replace(matched, Default::default());
let taken = core::mem::take(matched);
let SecContextState {
protocol_stage:
SecContextStage::EdhocResponderProcessedM1 {
Expand Down Expand Up @@ -960,6 +963,7 @@ impl<'a, H: coap_handler::Handler, Crypto: lakers::Crypto> coap_handler::Handler
M::Code::new(coap_numbers::code::UNAUTHORIZED).map_err(|x| Own(x.into()))?,
);
}
})
};
Ok(())
}
}
16 changes: 8 additions & 8 deletions src/riot-rs-coap/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,11 @@
// Moving work from https://github.com/embassy-rs/embassy/pull/2519 in here for the time being
mod udp_nal;

use coap_handler_implementations::{HandlerBuilder, ReportingHandlerBuilder};
use coap_handler_implementations::ReportingHandlerBuilder;
use coapcore::seccontext;
use critical_section::Mutex;
use embassy_net::udp::{PacketMetadata, UdpSocket};
use embassy_sync::once_lock::OnceLock;
use riot_rs_debug::log::*;
use riot_rs_debug::log::info;
use riot_rs_embassy::sendcell::SendCell;
use static_cell::StaticCell;

Expand All @@ -35,6 +34,11 @@ static CLIENT: OnceLock<
///
/// This can only be run once, as it sets up a system wide CoAP handler.
pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Reporting) -> ! {
use hexlit::hex;
const R: &[u8] = &hex!("72cc4761dbd4c78f758931aa589d348d1ef874a7e303ede2f140dcf3e6aa4aac");

static COAP: StaticCell<embedded_nal_coap::CoAPShared<CONCURRENT_REQUESTS>> = StaticCell::new();

let stack = riot_rs_embassy::network::network_stack().await.unwrap();

// FIXME trim to CoAP requirements
Expand All @@ -50,7 +54,6 @@ pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Report
&mut tx_meta,
&mut tx_buffer,
);
use embedded_nal_async::UnconnectedUdp;

info!("Starting up CoAP server");

Expand All @@ -61,8 +64,6 @@ pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Report
.await
.unwrap();

use hexlit::hex;
const R: &[u8] = &hex!("72cc4761dbd4c78f758931aa589d348d1ef874a7e303ede2f140dcf3e6aa4aac");
let own_identity = (
&lakers::CredentialRPK::new(lakers::EdhocMessageBuffer::new_from_slice(&hex!("A2026008A101A5010202410A2001215820BBC34960526EA4D32E940CAD2A234148DDC21791A12AFBCBAC93622046DD44F02258204519E257236B2A0CE2023F0931F1F386CA7AFDA64FCDE0108C224C51EABF6072")).expect("Credential should be small enough")).expect("Credential should be processable"),
R,
Expand All @@ -77,8 +78,7 @@ pub async fn coap_run(handler: impl coap_handler::Handler + coap_handler::Report

info!("Server is ready.");

static COAP: StaticCell<embedded_nal_coap::CoAPShared<CONCURRENT_REQUESTS>> = StaticCell::new();
let coap = COAP.init_with(|| embedded_nal_coap::CoAPShared::new());
let coap = COAP.init_with(embedded_nal_coap::CoAPShared::new);
let (client, server) = coap.split();
CLIENT
.init(SendCell::new_async(client).await)
Expand Down
36 changes: 26 additions & 10 deletions src/riot-rs-coap/src/udp_nal/mod.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
//! UDP sockets usable through [embedded_nal_async]
//! UDP sockets usable through [`embedded_nal_async`]
//!
//! The full [embedded_nal_async::UdpStack] is *not* implemented at the moment: As its API allows
//! arbitrary creation of movable sockets, embassy's [udp::UdpSocket] type could only be crated if
//! The full [`embedded_nal_async::UdpStack`] is *not* implemented at the moment: As its API allows
//! arbitrary creation of movable sockets, embassy's [`udp::UdpSocket`] type could only be crated if
//! the NAL stack had a pre-allocated pool of sockets with their respective buffers. Nothing rules
//! out such a type, but at the moment, only the bound or connected socket types are implemented
//! with their own constructors from an embassy [crate::Stack] -- for many applications, those are
//! with their own constructors from an embassy [`crate::Stack`] -- for many applications, those are
//! useful enough. (FIXME: Given we construct from Socket, Stack could really be implemented on
//! `Cell<Option<Socket>>` by `.take()`ing, couldn't it?)
//!
//! The constructors of the various socket types mimic the UdpStack's socket creation functions,
//! The constructors of the various socket types mimic the [`UdpStack`]'s socket creation functions,
//! but take an owned (uninitialized) Socket instead of a shared stack.
//!
//! No `bind_single` style constructor is currently provided. FIXME: Not sure we have all the
Expand All @@ -25,6 +25,10 @@ mod util;
pub use util::Error;
use util::{is_unspec_ip, sockaddr_nal2smol, sockaddr_smol2nal};

#[expect(
dead_code,
reason = "pub item is being prepared for embedded-nal-async where it will be reachable publicly"
)]
pub struct ConnectedUdp<'a> {
remote: IpEndpoint,
// The local port is stored in the socket, as it gets bound. This value is populated lazily:
Expand All @@ -38,13 +42,19 @@ pub struct ConnectedUdp<'a> {

/// A UDP socket that has been bound locally (either to a unique address or just to a port)
///
/// Its operations are accessible through the [nal::UnconnectedUdp] trait.
/// Its operations are accessible through the [`nal::UnconnectedUdp`] trait.
pub struct UnconnectedUdp<'a> {
socket: udp::UdpSocket<'a>,
}

#[allow(
dead_code,
clippy::unused_async,
clippy::missing_errors_doc,
reason = "pub item is being prepared for embedded-nal-async where it will be reachable publicly"
)]
impl<'a> ConnectedUdp<'a> {
/// Create a ConnectedUdp by assigning it a remote and a concrete local address
/// Create a [`ConnectedUdp`] by assigning it a remote and a concrete local address
///
/// ## Prerequisites
///
Expand All @@ -67,8 +77,8 @@ impl<'a> ConnectedUdp<'a> {
})
}

/// Create a ConnectedUdp by assigning it a remote and a local address (the latter may happen
/// lazily)
/// Create a [`ConnectedUdp`] by assigning it a remote and a local address (the latter may
/// happen lazily)
///
/// ## Prerequisites
///
Expand All @@ -80,8 +90,14 @@ impl<'a> ConnectedUdp<'a> {
}
}

#[allow(
dead_code,
clippy::unused_async,
clippy::missing_errors_doc,
reason = "pub item is being prepared for embedded-nal-async where it will be reachable publicly"
)]
impl<'a> UnconnectedUdp<'a> {
/// Create an UnconnectedUdp.
/// Create an [`UnconnectedUdp`].
///
/// The `local` address may be anything from fully specified (address and port) to fully
/// unspecified (port 0, all-zeros address).
Expand Down
Loading

0 comments on commit 7d0603d

Please sign in to comment.