Skip to content

Commit 5628ba8

Browse files
committed
fix(parse)!: Use u16 as our size type the parse traits
BREAKING_CHANGE: swap `usize` to `u16` in parse traits DPDK uses `u16` where etherparse uses `usize`. Both answers are reasonable in the context of packet processing. That said, we need to be compatible with both of them. The only practical answer is to switch the parse traits to use `u16`. Any alternative requires constant downcast checks throughout the rest of the code base. Signed-off-by: Daniel Noland <[email protected]>
1 parent e66b71c commit 5628ba8

File tree

12 files changed

+406
-262
lines changed

12 files changed

+406
-262
lines changed

net/src/eth/mod.rs

+36-21
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ pub enum EthError {
4242

4343
impl Eth {
4444
/// The length (in bytes) of an [`Eth`] header
45-
pub const HEADER_LEN: usize = 14;
45+
#[allow(clippy::unwrap_used)] // trivially safe const eval
46+
pub const HEADER_LEN: NonZero<u16> = NonZero::new(14).unwrap();
4647

4748
/// Create a new [Eth] header.
4849
#[must_use]
@@ -100,7 +101,10 @@ impl Eth {
100101
impl Parse for Eth {
101102
type Error = EthError;
102103

103-
fn parse(buf: &[u8]) -> Result<(Self, NonZero<usize>), ParseError<Self::Error>> {
104+
fn parse(buf: &[u8]) -> Result<(Self, NonZero<u16>), ParseError<Self::Error>> {
105+
if buf.len() > u16::MAX as usize {
106+
return Err(ParseError::BufferTooLong(buf.len()));
107+
}
104108
let (inner, rest) = Ethernet2Header::from_slice(buf).map_err(|e| {
105109
let expected = NonZero::new(e.required_len).unwrap_or_else(|| unreachable!());
106110
ParseError::Length(LengthError {
@@ -114,7 +118,9 @@ impl Parse for Eth {
114118
rest = rest.len(),
115119
buf = buf.len()
116120
);
117-
let consumed = NonZero::new(buf.len() - rest.len()).ok_or_else(|| unreachable!())?;
121+
#[allow(clippy::cast_possible_truncation)] // buffer length bounded above
122+
let consumed =
123+
NonZero::new((buf.len() - rest.len()) as u16).ok_or_else(|| unreachable!())?;
118124
let new = Self(inner);
119125
// integrity check for ethernet header
120126
new.destination()
@@ -132,12 +138,16 @@ impl Parse for Eth {
132138
impl DeParse for Eth {
133139
type Error = ();
134140

135-
fn size(&self) -> NonZero<usize> {
136-
NonZero::new(self.0.header_len()).unwrap_or_else(|| unreachable!())
141+
fn size(&self) -> NonZero<u16> {
142+
#[allow(clippy::cast_possible_truncation)] // Eth headers have fixed length
143+
NonZero::new(self.0.header_len() as u16).unwrap_or_else(|| unreachable!())
137144
}
138145

139-
fn deparse(&self, buf: &mut [u8]) -> Result<NonZero<usize>, DeParseError<Self::Error>> {
146+
fn deparse(&self, buf: &mut [u8]) -> Result<NonZero<u16>, DeParseError<Self::Error>> {
140147
let len = buf.len();
148+
if buf.len() > u16::MAX as usize {
149+
return Err(DeParseError::BufferTooLong(len));
150+
}
141151
let unused = self.0.write_to_slice(buf).map_err(|e| {
142152
let expected = NonZero::new(e.required_len).unwrap_or_else(|| unreachable!());
143153
DeParseError::Length(LengthError {
@@ -150,7 +160,8 @@ impl DeParse for Eth {
150160
"unused.len() >= buf.len() ({unused} >= {len})",
151161
unused = unused.len(),
152162
);
153-
let consumed = NonZero::new(len - unused.len()).ok_or_else(|| unreachable!())?;
163+
#[allow(clippy::cast_possible_truncation)] // buffer len upper bounded already
164+
let consumed = NonZero::new((len - unused.len()) as u16).ok_or_else(|| unreachable!())?;
154165
Ok(consumed)
155166
}
156167
}
@@ -231,75 +242,79 @@ mod contract {
231242
#[allow(clippy::unwrap_used, clippy::expect_used)] // valid in test code for unreachable cases
232243
#[cfg(test)]
233244
mod test {
245+
const HEADER_LEN_USIZE: usize = Eth::HEADER_LEN.get() as usize;
234246
use crate::eth::{DestinationMacAddressError, Eth, EthError, SourceMacAddressError};
235-
use crate::parse::{DeParse, Parse, ParseError};
247+
use crate::parse::{DeParse, IntoNonZeroUSize, Parse, ParseError};
236248

237249
#[test]
238250
fn parse_back() {
239251
bolero::check!().with_type().for_each(|eth: &Eth| {
240252
assert!(eth.source().inner().valid_src().is_ok());
241253
assert!(eth.destination().inner().valid_dst().is_ok());
242-
let mut buf = [0u8; Eth::HEADER_LEN];
254+
let mut buf = [0u8; HEADER_LEN_USIZE];
243255
eth.deparse(&mut buf).unwrap();
244256
let (eth2, consumed) = Eth::parse(&buf).unwrap();
245257
assert_eq!(eth, &eth2);
246-
assert_eq!(consumed.get(), Eth::HEADER_LEN);
258+
assert_eq!(consumed, Eth::HEADER_LEN);
247259
});
248260
}
249261

250262
fn parse_buffer_of_fixed_length<const LEN: usize>(buf: &[u8; LEN]) {
251263
let outcome = Eth::parse(buf);
252264
match outcome {
253265
Ok((eth, consumed)) => {
254-
assert!(buf.len() >= Eth::HEADER_LEN);
255-
assert_eq!(consumed.get(), Eth::HEADER_LEN);
266+
assert!(buf.len() >= Eth::HEADER_LEN.into_non_zero_usize().get());
267+
assert_eq!(consumed, Eth::HEADER_LEN);
256268
assert!(eth.source().inner().valid_src().is_ok());
257269
assert!(eth.destination().inner().valid_dst().is_ok());
258-
let mut buf2 = [0u8; 14];
270+
let mut buf2 = [0u8; HEADER_LEN_USIZE];
259271
eth.deparse(&mut buf2).unwrap();
260272
let (eth2, consumed2) = Eth::parse(&buf2).unwrap();
261273
assert_eq!(eth, eth2);
262-
assert_eq!(consumed2.get(), Eth::HEADER_LEN);
274+
assert_eq!(consumed2, Eth::HEADER_LEN);
263275
}
264276
Err(ParseError::Length(e)) => {
265-
assert_eq!(e.expected.get(), Eth::HEADER_LEN);
277+
assert_eq!(e.expected, Eth::HEADER_LEN.into_non_zero_usize());
266278
assert_eq!(e.actual, buf.len());
267-
assert!(buf.len() < Eth::HEADER_LEN);
279+
assert!(buf.len() < Eth::HEADER_LEN.into_non_zero_usize().get());
268280
}
269281
Err(ParseError::Invalid(
270282
EthError::InvalidDestination(DestinationMacAddressError::ZeroDestination(z))
271283
| EthError::InvalidSource(SourceMacAddressError::ZeroSource(z)),
272284
)) => {
273-
assert!(buf.len() >= Eth::HEADER_LEN);
285+
assert!(buf.len() >= Eth::HEADER_LEN.into_non_zero_usize().get());
274286
assert!(z.is_zero());
275287
}
276288
Err(ParseError::Invalid(EthError::InvalidSource(
277289
SourceMacAddressError::MulticastSource(m),
278290
))) => {
279-
assert!(buf.len() >= Eth::HEADER_LEN);
291+
assert!(buf.len() >= Eth::HEADER_LEN.into_non_zero_usize().get());
280292
assert!(m.is_multicast());
281293
}
294+
Err(ParseError::BufferTooLong(e)) => {
295+
assert_eq!(e, buf.len());
296+
}
282297
}
283298
}
284299

285300
#[test]
286301
fn parse_arbitrary_bytes() {
287302
bolero::check!()
288303
.with_type()
289-
.for_each(parse_buffer_of_fixed_length::<{ Eth::HEADER_LEN }>);
304+
.for_each(parse_buffer_of_fixed_length::<{ HEADER_LEN_USIZE }>);
290305
}
291306

292307
#[test]
293308
fn parse_prop_test_buffer_too_short() {
294309
bolero::check!()
295310
.with_type()
296-
.for_each(parse_buffer_of_fixed_length::<{ Eth::HEADER_LEN - 1 }>);
311+
.for_each(parse_buffer_of_fixed_length::<{ HEADER_LEN_USIZE - 1 }>);
297312
}
298313

299314
#[test]
300315
fn parse_prop_test_excess_buffer() {
301316
bolero::check!()
302317
.with_type()
303-
.for_each(parse_buffer_of_fixed_length::<{ Eth::HEADER_LEN + 1 }>);
318+
.for_each(parse_buffer_of_fixed_length::<{ HEADER_LEN_USIZE + 1 }>);
304319
}
305320
}

net/src/icmp4/mod.rs

+22-9
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33

44
//! `ICMPv4` header type and logic.
55
6-
use crate::parse::{DeParse, DeParseError, LengthError, Parse, ParseError, ParsePayload, Reader};
6+
use crate::parse::{
7+
DeParse, DeParseError, IntoNonZeroUSize, LengthError, Parse, ParseError, ParsePayload, Reader,
8+
};
79
use etherparse::Icmpv4Header;
810
use std::num::NonZero;
911

@@ -18,7 +20,10 @@ pub struct Icmp4(Icmpv4Header);
1820
impl Parse for Icmp4 {
1921
type Error = LengthError;
2022

21-
fn parse(buf: &[u8]) -> Result<(Self, NonZero<usize>), ParseError<Self::Error>> {
23+
fn parse(buf: &[u8]) -> Result<(Self, NonZero<u16>), ParseError<Self::Error>> {
24+
if buf.len() > u16::MAX as usize {
25+
return Err(ParseError::BufferTooLong(buf.len()));
26+
}
2227
let (inner, rest) = Icmpv4Header::from_slice(buf).map_err(|e| {
2328
let expected = NonZero::new(e.required_len).unwrap_or_else(|| unreachable!());
2429
ParseError::Length(LengthError {
@@ -32,27 +37,30 @@ impl Parse for Icmp4 {
3237
rest = rest.len(),
3338
buf = buf.len()
3439
);
35-
let consumed = NonZero::new(buf.len() - rest.len()).ok_or_else(|| unreachable!())?;
40+
#[allow(clippy::cast_possible_truncation)] // checked above
41+
let consumed =
42+
NonZero::new((buf.len() - rest.len()) as u16).ok_or_else(|| unreachable!())?;
3643
Ok((Self(inner), consumed))
3744
}
3845
}
3946

4047
impl DeParse for Icmp4 {
4148
type Error = ();
4249

43-
fn size(&self) -> NonZero<usize> {
44-
NonZero::new(self.0.header_len()).unwrap_or_else(|| unreachable!())
50+
fn size(&self) -> NonZero<u16> {
51+
#[allow(clippy::cast_possible_truncation)] // header length bounded
52+
NonZero::new(self.0.header_len() as u16).unwrap_or_else(|| unreachable!())
4553
}
4654

47-
fn deparse(&self, buf: &mut [u8]) -> Result<NonZero<usize>, DeParseError<Self::Error>> {
55+
fn deparse(&self, buf: &mut [u8]) -> Result<NonZero<u16>, DeParseError<Self::Error>> {
4856
let len = buf.len();
49-
if len < self.size().get() {
57+
if len < self.size().into_non_zero_usize().get() {
5058
return Err(DeParseError::Length(LengthError {
51-
expected: self.size(),
59+
expected: self.size().into_non_zero_usize(),
5260
actual: len,
5361
}));
5462
}
55-
buf[..self.size().get()].copy_from_slice(&self.0.to_bytes());
63+
buf[..self.size().into_non_zero_usize().get()].copy_from_slice(&self.0.to_bytes());
5664
Ok(self.size())
5765
}
5866
}
@@ -81,6 +89,9 @@ mod contract {
8189
Ok((icmp4, _)) => icmp4,
8290
Err(ParseError::Length(l)) => unreachable!("{:?}", l),
8391
Err(ParseError::Invalid(e)) => unreachable!("{:?}", e),
92+
Err(ParseError::BufferTooLong(_)) => {
93+
unreachable!()
94+
}
8495
};
8596
Some(icmp4)
8697
}
@@ -104,11 +115,13 @@ mod test {
104115
Err(DeParseError::Invalid(())) => {
105116
unreachable!()
106117
}
118+
Err(DeParseError::BufferTooLong(_)) => unreachable!(),
107119
};
108120
let (parsed, bytes_read) = match Icmp4::parse(&buffer) {
109121
Ok((parsed, bytes_read)) => (parsed, bytes_read),
110122
Err(ParseError::Invalid(e)) => unreachable!("{e:?}"),
111123
Err(ParseError::Length(l)) => unreachable!("{l:?}"),
124+
Err(ParseError::BufferTooLong(_)) => unreachable!(),
112125
};
113126
assert_eq!(input, &parsed);
114127
assert_eq!(bytes_written, bytes_read);

net/src/icmp6/mod.rs

+17-9
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33

44
//! `ICMPv6` header type and logic.
55
6-
use crate::parse::{DeParse, DeParseError, LengthError, Parse, ParseError, ParsePayload, Reader};
6+
use crate::parse::{
7+
DeParse, DeParseError, IntoNonZeroUSize, LengthError, Parse, ParseError, ParsePayload, Reader,
8+
};
79
use etherparse::Icmpv6Header;
810
use std::num::NonZero;
911

@@ -17,7 +19,10 @@ pub struct Icmp6(Icmpv6Header);
1719
impl Parse for Icmp6 {
1820
type Error = LengthError;
1921

20-
fn parse(buf: &[u8]) -> Result<(Self, NonZero<usize>), ParseError<Self::Error>> {
22+
fn parse(buf: &[u8]) -> Result<(Self, NonZero<u16>), ParseError<Self::Error>> {
23+
if buf.len() > u16::MAX as usize {
24+
return Err(ParseError::BufferTooLong(buf.len()));
25+
}
2126
let (inner, rest) = Icmpv6Header::from_slice(buf).map_err(|e| {
2227
let expected = NonZero::new(e.required_len).unwrap_or_else(|| unreachable!());
2328
ParseError::Length(LengthError {
@@ -31,7 +36,9 @@ impl Parse for Icmp6 {
3136
rest = rest.len(),
3237
buf = buf.len()
3338
);
34-
let consumed = NonZero::new(buf.len() - rest.len()).ok_or_else(|| unreachable!())?;
39+
#[allow(clippy::cast_possible_truncation)] // buffer length bounded above
40+
let consumed =
41+
NonZero::new((buf.len() - rest.len()) as u16).ok_or_else(|| unreachable!())?;
3542
Ok((Self(inner), consumed))
3643
}
3744
}
@@ -48,19 +55,20 @@ impl ParsePayload for Icmp6 {
4855
impl DeParse for Icmp6 {
4956
type Error = ();
5057

51-
fn size(&self) -> NonZero<usize> {
52-
NonZero::new(self.0.header_len()).unwrap_or_else(|| unreachable!())
58+
fn size(&self) -> NonZero<u16> {
59+
#[allow(clippy::cast_possible_truncation)] // header size bounded
60+
NonZero::new(self.0.header_len() as u16).unwrap_or_else(|| unreachable!())
5361
}
5462

55-
fn deparse(&self, buf: &mut [u8]) -> Result<NonZero<usize>, DeParseError<Self::Error>> {
63+
fn deparse(&self, buf: &mut [u8]) -> Result<NonZero<u16>, DeParseError<Self::Error>> {
5664
let len = buf.len();
57-
if len < self.size().get() {
65+
if len < self.size().into_non_zero_usize().get() {
5866
return Err(DeParseError::Length(LengthError {
59-
expected: self.size(),
67+
expected: self.size().into_non_zero_usize(),
6068
actual: len,
6169
}));
6270
}
63-
buf[..self.size().get()].copy_from_slice(&self.0.to_bytes());
71+
buf[..self.size().into_non_zero_usize().get()].copy_from_slice(&self.0.to_bytes());
6472
Ok(self.size())
6573
}
6674
}

net/src/ip_auth/mod.rs

+7-2
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ pub struct IpAuth(Box<IpAuthHeader>);
2222
impl Parse for IpAuth {
2323
type Error = etherparse::err::ip_auth::HeaderSliceError;
2424

25-
fn parse(buf: &[u8]) -> Result<(Self, NonZero<usize>), ParseError<Self::Error>> {
25+
fn parse(buf: &[u8]) -> Result<(Self, NonZero<u16>), ParseError<Self::Error>> {
26+
if buf.len() > u16::MAX as usize {
27+
return Err(ParseError::BufferTooLong(buf.len()));
28+
}
2629
let (inner, rest) = IpAuthHeader::from_slice(buf)
2730
.map(|(h, rest)| (Box::new(h), rest))
2831
.map_err(ParseError::Invalid)?;
@@ -32,7 +35,9 @@ impl Parse for IpAuth {
3235
rest = rest.len(),
3336
buf = buf.len()
3437
);
35-
let consumed = NonZero::new(buf.len() - rest.len()).ok_or_else(|| unreachable!())?;
38+
#[allow(clippy::cast_possible_truncation)] // buffer length bounded above
39+
let consumed =
40+
NonZero::new((buf.len() - rest.len()) as u16).ok_or_else(|| unreachable!())?;
3641
Ok((Self(inner), consumed))
3742
}
3843
}

0 commit comments

Comments
 (0)