chore(lints): enforce per-crate lint policy and add build profiles
Give every member crate its own `[lints.clippy]` (`pedantic`, `todo`, `unwrap_used`) and `[lints.rust]` block, add explicit `release` and `production` profiles to the workspace manifest, and fix the code so that `just clippy` -- `cargo clippy --workspace --all-targets --all-features -- -D warnings` -- passes with zero warnings. Declaring the lints is the easy half and on its own it was actively harmful: `unsafe_code = "forbid"` cannot be overridden from source, so it broke `lanparty-gateway`, `lanparty-client-tap`, and `lanparty-client-route`, which all need OS FFI, and the workspace no longer compiled on Linux at all. The rest of the workspace produced roughly 120 denied lints. A lint policy nobody can satisfy is worse than no policy, so this commit makes the tree actually clean. Unsafe policy: the three FFI crates use `unsafe_code = "deny"`, and the one module in each that holds the FFI (`gateway::packet`, `client_tap::windows`, `client_route::windows`) opts back in with a documented `#![allow(unsafe_code)]`. Every other crate keeps `"forbid"`. `deny` was chosen over dropping the lint so that `unsafe` outside those modules is still a hard error, and over per-block allows because the FFI is dense enough that per-block attributes would drown the code. Build profiles: `release` stays optimized but debuggable (debug info, debug assertions, overflow checks, no LTO, incremental) so that a bug reproduced at close-to-real speed still panics loudly and gives a usable backtrace. `production` inherits from it and turns all of that off, adding fat LTO and a single codegen unit, and is what ships. Note the consequence: `cargo build --release` binaries are now slower than before this commit and must not be shipped; use `just build-production`. Code changes made to satisfy the lints, grouped by kind: - `# Errors` sections on every public fallible function, and `# Panics` on `RoomRegistry::new`. The Windows-only implementations and their non-Windows `bail!` stubs are documented in parallel so the crates stay clean when built for Windows too, which CI on Linux cannot check. - Panicking paths removed rather than documented where the panic was only an unreachable invariant: `OverlayHeader::decode` and `declared_payload_len` take a fixed-size prefix via `first_chunk` instead of `try_into().expect(...)`, and `RoomRegistry::join` returns an `InternalError` reject instead of `expect`ing the room it just inserted. - Tests keep using `unwrap`/`unwrap_err`: each crate root carries `#![cfg_attr(test, allow(clippy::unwrap_used))]`. The lint is about production code; an `expect` message per assertion buys nothing. - Lossy casts replaced with `try_from` where a real conversion was happening (datagram-size negotiation, control-frame length prefix, MTU clamping). In `gateway::packet` the cast lints are allowed module-wide instead: those casts move values between libc's C types and ours after the value is already known to fit, so fallible conversions would only add unreachable branches. - Signature changes, all on private or crate-internal items except one: `RoomRegistry::join` now takes `&EndpointHello` (public, it never consumed the hello), `Room::join` and `reject_control_error` likewise. `Room`'s `room_id` field is now `id`. - `#[allow(clippy::too_many_lines)]` with a reason on three end-to-end test scenarios and on `bridge_until_shutdown`, whose `select!` loop mutates state shared by every arm; splitting it would hide that. - Mechanical fixes from `cargo clippy --fix`: `map_or_else`, `let...else`, inline format args, backticks in doc comments, `Duration::from_mins`. No runtime behavior changes: the only observable differences are the error text when a relay room lookup fails immediately after insertion (previously a panic) and the profile rename described above. Also documents the lint policy and the profile table in README.md, whose build section now points at the `just` recipes it should have used all along. Test Plan: - `just clippy` -- passes, zero warnings - `just test` -- 186 tests pass, 0 failed - `cargo check --profile production --workspace --all-features` -- passes - `just fmt` -- clean - Not verified: the Windows-only code paths and their doc comments, which need a Windows target to compile. Refs: https://doc.rust-lang.org/cargo/reference/profiles.html Refs: https://rust-lang.github.io/rust-clippy/master/index.html#missing_errors_doc Refs: https://doc.rust-lang.org/reference/attributes/diagnostics.html#lint-check-attributes
This commit is contained in:
@@ -6,3 +6,11 @@ edition.workspace = true
|
||||
[dependencies]
|
||||
serde.workspace = true
|
||||
thiserror.workspace = true
|
||||
|
||||
[lints.clippy]
|
||||
pedantic = { level = "warn", priority = -1 }
|
||||
todo = "warn"
|
||||
unwrap_used = "warn"
|
||||
|
||||
[lints.rust]
|
||||
unsafe_code = "forbid"
|
||||
|
||||
@@ -12,6 +12,13 @@ pub struct EthernetFrame<'a> {
|
||||
}
|
||||
|
||||
impl<'a> EthernetFrame<'a> {
|
||||
/// Borrows `bytes` as an Ethernet frame after checking that it is long
|
||||
/// enough to contain a complete Ethernet header.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`ProtoError::EthernetFrameTooShort`] if `bytes` is shorter than
|
||||
/// [`MIN_ETHERNET_FRAME_LEN`].
|
||||
pub fn parse(bytes: &'a [u8]) -> Result<Self, ProtoError> {
|
||||
if bytes.len() < MIN_ETHERNET_FRAME_LEN {
|
||||
return Err(ProtoError::EthernetFrameTooShort {
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
//! This crate intentionally contains no socket, TAP, QUIC, or OS-specific
|
||||
//! behavior. It is the small contract that the Windows client, Linux gateway,
|
||||
//! and relay must all agree on.
|
||||
#![cfg_attr(test, allow(clippy::unwrap_used))]
|
||||
|
||||
mod ethernet;
|
||||
mod mac;
|
||||
|
||||
@@ -91,8 +91,8 @@ impl From<MacAddr> for [u8; 6] {
|
||||
|
||||
impl fmt::Display for MacAddr {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
let [a, b, c, d, e, g] = self.0;
|
||||
write!(f, "{a:02x}:{b:02x}:{c:02x}:{d:02x}:{e:02x}:{g:02x}")
|
||||
let [o0, o1, o2, o3, o4, o5] = self.0;
|
||||
write!(f, "{o0:02x}:{o1:02x}:{o2:02x}:{o3:02x}:{o4:02x}:{o5:02x}")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -17,6 +17,14 @@ pub enum MtuError {
|
||||
},
|
||||
}
|
||||
|
||||
/// Returns the largest TAP MTU that still fits into the negotiated QUIC
|
||||
/// datagram budget, after subtracting overlay and Ethernet framing overhead
|
||||
/// plus `safety_margin`.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`MtuError::DatagramTooSmall`] if the budget cannot even carry
|
||||
/// [`MIN_USEFUL_TAP_MTU`] bytes of payload on top of that overhead.
|
||||
pub fn max_tap_mtu_for_datagram(
|
||||
quic_max_datagram_size: usize,
|
||||
safety_margin: usize,
|
||||
@@ -34,6 +42,13 @@ pub fn max_tap_mtu_for_datagram(
|
||||
Ok(quic_max_datagram_size - overhead)
|
||||
}
|
||||
|
||||
/// Returns the TAP MTU to configure: [`DEFAULT_TAP_MTU`], clamped down when the
|
||||
/// negotiated QUIC datagram budget cannot carry it.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`MtuError::DatagramTooSmall`] if the budget is too small to be
|
||||
/// useful at all; see [`max_tap_mtu_for_datagram`].
|
||||
pub fn recommended_tap_mtu(quic_max_datagram_size: usize) -> Result<usize, MtuError> {
|
||||
let max = max_tap_mtu_for_datagram(quic_max_datagram_size, DEFAULT_DATAGRAM_SAFETY_MARGIN)?;
|
||||
|
||||
|
||||
@@ -19,6 +19,12 @@ impl FrameType {
|
||||
self as u8
|
||||
}
|
||||
|
||||
/// Decodes the on-wire frame type discriminant.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`ProtoError::UnknownFrameType`] for a discriminant this
|
||||
/// protocol version does not define.
|
||||
pub const fn from_u8(value: u8) -> Result<Self, ProtoError> {
|
||||
match value {
|
||||
1 => Ok(Self::Ethernet),
|
||||
@@ -39,6 +45,13 @@ pub struct OverlayHeader {
|
||||
}
|
||||
|
||||
impl OverlayHeader {
|
||||
/// Builds a header for a payload of `payload_len` bytes.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`ProtoError::UnsupportedFlags`] if `flags` sets any bit that is
|
||||
/// still reserved, or [`ProtoError::PayloadTooLarge`] if `payload_len` does
|
||||
/// not fit into the 16-bit wire field.
|
||||
pub fn new(
|
||||
frame_type: FrameType,
|
||||
room_id: u64,
|
||||
@@ -99,35 +112,49 @@ impl OverlayHeader {
|
||||
bytes
|
||||
}
|
||||
|
||||
/// Decodes the fixed-size header prefix of a received datagram.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`ProtoError::DatagramTooShort`] if `bytes` is shorter than
|
||||
/// [`OVERLAY_HEADER_LEN`], [`ProtoError::BadMagic`] or
|
||||
/// [`ProtoError::UnsupportedVersion`] if the datagram is not a datagram of
|
||||
/// this protocol version, [`ProtoError::UnsupportedFlags`] if a reserved
|
||||
/// flag bit is set, or [`ProtoError::UnknownFrameType`] for an unknown
|
||||
/// frame type.
|
||||
pub fn decode(bytes: &[u8]) -> Result<Self, ProtoError> {
|
||||
if bytes.len() < OVERLAY_HEADER_LEN {
|
||||
// Taking the header as a fixed-size array up front makes every field
|
||||
// read below a statically bounds-checked array index, so this function
|
||||
// has no panicking path.
|
||||
let Some(header) = bytes.first_chunk::<OVERLAY_HEADER_LEN>() else {
|
||||
return Err(ProtoError::DatagramTooShort {
|
||||
actual: bytes.len(),
|
||||
minimum: OVERLAY_HEADER_LEN,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
let magic = u32::from_be_bytes(bytes[0..4].try_into().expect("header magic slice length"));
|
||||
let magic = u32::from_be_bytes([header[0], header[1], header[2], header[3]]);
|
||||
if magic != OVERLAY_MAGIC {
|
||||
return Err(ProtoError::BadMagic { actual: magic });
|
||||
}
|
||||
|
||||
let version = bytes[4];
|
||||
let version = header[4];
|
||||
if version != OVERLAY_VERSION {
|
||||
return Err(ProtoError::UnsupportedVersion { actual: version });
|
||||
}
|
||||
|
||||
let flags = u16::from_be_bytes(bytes[18..20].try_into().expect("flags slice length"));
|
||||
let flags = u16::from_be_bytes([header[18], header[19]]);
|
||||
validate_overlay_flags(flags)?;
|
||||
|
||||
Ok(Self {
|
||||
frame_type: FrameType::from_u8(bytes[5])?,
|
||||
room_id: u64::from_be_bytes(bytes[6..14].try_into().expect("room id slice length")),
|
||||
peer_id: u32::from_be_bytes(bytes[14..18].try_into().expect("peer id slice length")),
|
||||
frame_type: FrameType::from_u8(header[5])?,
|
||||
room_id: u64::from_be_bytes([
|
||||
header[6], header[7], header[8], header[9], header[10], header[11], header[12],
|
||||
header[13],
|
||||
]),
|
||||
peer_id: u32::from_be_bytes([header[14], header[15], header[16], header[17]]),
|
||||
flags,
|
||||
payload_len: u16::from_be_bytes(
|
||||
bytes[20..22].try_into().expect("payload len slice length"),
|
||||
),
|
||||
payload_len: u16::from_be_bytes([header[20], header[21]]),
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -139,6 +166,12 @@ pub struct OverlayPacket<'a> {
|
||||
}
|
||||
|
||||
impl<'a> OverlayPacket<'a> {
|
||||
/// Pairs a decoded header with its payload.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`ProtoError::PayloadLengthMismatch`] if `payload` is not
|
||||
/// exactly as long as the header declares.
|
||||
pub fn new(header: OverlayHeader, payload: &'a [u8]) -> Result<Self, ProtoError> {
|
||||
let declared = usize::from(header.payload_len);
|
||||
|
||||
@@ -193,6 +226,11 @@ fn validate_overlay_flags(flags: u16) -> Result<(), ProtoError> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Encodes a complete overlay datagram: header followed by `payload`.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Propagates every error of [`OverlayHeader::new`].
|
||||
pub fn encode_datagram(
|
||||
frame_type: FrameType,
|
||||
room_id: u64,
|
||||
@@ -207,6 +245,13 @@ pub fn encode_datagram(
|
||||
Ok(datagram)
|
||||
}
|
||||
|
||||
/// Checks an encoded datagram against the QUIC datagram size the peers
|
||||
/// negotiated.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`ProtoError::DatagramExceedsBudget`] if `datagram_len` exceeds
|
||||
/// `max_datagram_size`.
|
||||
pub fn validate_datagram_budget(
|
||||
datagram_len: usize,
|
||||
max_datagram_size: usize,
|
||||
@@ -221,6 +266,12 @@ pub fn validate_datagram_budget(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Decodes a received overlay datagram, borrowing its payload from `bytes`.
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Propagates every error of [`OverlayHeader::decode`] and
|
||||
/// [`OverlayPacket::new`].
|
||||
pub fn decode_datagram(bytes: &[u8]) -> Result<OverlayPacket<'_>, ProtoError> {
|
||||
let header = OverlayHeader::decode(bytes)?;
|
||||
let payload = &bytes[OVERLAY_HEADER_LEN..];
|
||||
|
||||
@@ -14,7 +14,6 @@ const IPV6_NEXT_HEADER_HOP_BY_HOP: u8 = 0;
|
||||
const IPV6_NEXT_HEADER_ROUTING: u8 = 43;
|
||||
const IPV6_NEXT_HEADER_FRAGMENT: u8 = 44;
|
||||
const IPV6_NEXT_HEADER_AH: u8 = 51;
|
||||
const IPV6_NEXT_HEADER_NO_NEXT: u8 = 59;
|
||||
const IPV6_NEXT_HEADER_DESTINATION_OPTIONS: u8 = 60;
|
||||
const IPV6_NEXT_HEADER_ICMPV6: u8 = 58;
|
||||
const DHCPV4_SERVER_PORT: u16 = 67;
|
||||
@@ -34,6 +33,9 @@ pub enum EthernetSafetyDrop {
|
||||
Ipv6Fragment,
|
||||
}
|
||||
|
||||
/// Returns why a frame coming from the physical LAN must not be forwarded into
|
||||
/// the tunnel, or `None` if it may be forwarded.
|
||||
#[must_use]
|
||||
pub fn gateway_lan_safety_drop_reason(frame: EthernetFrame<'_>) -> Option<EthernetSafetyDrop> {
|
||||
if !frame.source().is_valid_unicast() {
|
||||
return Some(EthernetSafetyDrop::InvalidSourceMac);
|
||||
@@ -42,6 +44,12 @@ pub fn gateway_lan_safety_drop_reason(frame: EthernetFrame<'_>) -> Option<Ethern
|
||||
common_safety_drop_reason(frame)
|
||||
}
|
||||
|
||||
/// Returns why a frame received from a remote tunnel client must not be
|
||||
/// injected into the physical LAN, or `None` if it may be injected.
|
||||
///
|
||||
/// This is stricter than [`gateway_lan_safety_drop_reason`]: remote peers must
|
||||
/// not be able to act as a VLAN trunk, DHCP server, or IPv6 router on the LAN.
|
||||
#[must_use]
|
||||
pub fn remote_client_safety_drop_reason(frame: EthernetFrame<'_>) -> Option<EthernetSafetyDrop> {
|
||||
if let Some(drop_reason) = common_safety_drop_reason(frame) {
|
||||
return Some(drop_reason);
|
||||
@@ -93,9 +101,9 @@ fn is_vlan_tagged_frame(frame: EthernetFrame<'_>) -> bool {
|
||||
}
|
||||
|
||||
fn is_link_local_control_destination(mac: MacAddr) -> bool {
|
||||
let [a, b, c, d, e, f] = mac.octets();
|
||||
let [prefix @ .., last] = mac.octets();
|
||||
|
||||
[a, b, c, d, e] == [0x01, 0x80, 0xc2, 0x00, 0x00] && f <= 0x0f
|
||||
prefix == [0x01, 0x80, 0xc2, 0x00, 0x00] && last <= 0x0f
|
||||
}
|
||||
|
||||
fn is_dhcp_server_reply(frame: EthernetFrame<'_>) -> bool {
|
||||
@@ -190,7 +198,6 @@ fn ipv6_upper_layer_payload_offset(ipv6: &[u8], expected_next_header: u8) -> Opt
|
||||
loop {
|
||||
match next_header {
|
||||
next_header if next_header == expected_next_header => return Some(offset),
|
||||
IPV6_NEXT_HEADER_NO_NEXT => return None,
|
||||
IPV6_NEXT_HEADER_HOP_BY_HOP
|
||||
| IPV6_NEXT_HEADER_ROUTING
|
||||
| IPV6_NEXT_HEADER_DESTINATION_OPTIONS => {
|
||||
@@ -216,6 +223,9 @@ fn ipv6_upper_layer_payload_offset(ipv6: &[u8], expected_next_header: u8) -> Opt
|
||||
return None;
|
||||
}
|
||||
}
|
||||
// Any other next-header value is either an upper-layer protocol we
|
||||
// were not asked about or the "no next header" terminator (59), so
|
||||
// the header we are looking for is not in this packet.
|
||||
_ => return None,
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user