Skip to content

fix(lading): surface user-config parsing failures as Error variants #1902

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

Draft
goxberry wants to merge 1 commit into
base: goxberry/expect-cleanup-lading-bins-drop-quarantines
Choose a base branch
from
Draft

fix(lading): surface user-config parsing failures as Error variants #1902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 8 additions & 12 deletions lading/src/bin/payloadtool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,6 @@ fn generate_and_check(
}

#[expect(clippy::too_many_lines)]
#[expect(
clippy::expect_used,
reason = "FIXME: maximum_prebuild_cache_size_bytes is user-supplied config; a zero value should surface as a recoverable error rather than panicking. Tracked for follow-up."
)]
fn check_generator(config: &generator::Config, args: &Args) -> Result<Option<Fingerprint>> {
match &config.inner {
generator::Inner::FileGen(g) => {
Expand All @@ -398,26 +394,26 @@ fn check_generator(config: &generator::Config, args: &Args) -> Result<Option<Fin
};
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(cache_size.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
generate_and_check(variant, seed, total_bytes, max_block_size, args)
}
generator::Inner::UnixDatagram(g) => {
let max_block_size = UDP_PACKET_LIMIT_BYTES;
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(g.maximum_prebuild_cache_size_bytes.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
generate_and_check(&g.variant, g.seed, total_bytes, max_block_size, args)
}
generator::Inner::Tcp(g) => {
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(g.maximum_prebuild_cache_size_bytes.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
generate_and_check(&g.variant, g.seed, total_bytes, g.maximum_block_size, args)
}
generator::Inner::Udp(g) => {
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(g.maximum_prebuild_cache_size_bytes.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
let max_block_size = UDP_PACKET_LIMIT_BYTES;
generate_and_check(&g.variant, g.seed, total_bytes, max_block_size, args)
}
Expand All @@ -431,7 +427,7 @@ fn check_generator(config: &generator::Config, args: &Args) -> Result<Option<Fin
};
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(max_prebuild_cache_size_bytes.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
generate_and_check(variant, g.seed, total_bytes, g.maximum_block_size, args)
}
generator::Inner::SplunkHec(_) => {
Expand All @@ -451,19 +447,19 @@ fn check_generator(config: &generator::Config, args: &Args) -> Result<Option<Fin
generator::Inner::Grpc(g) => {
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(g.maximum_prebuild_cache_size_bytes.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
generate_and_check(&g.variant, g.seed, total_bytes, g.maximum_block_size, args)
}
generator::Inner::UnixStream(g) => {
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(g.maximum_prebuild_cache_size_bytes.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
generate_and_check(&g.variant, g.seed, total_bytes, g.maximum_block_size, args)
}
generator::Inner::PassthruFile(g) => {
#[expect(clippy::cast_possible_truncation)]
let total_bytes = NonZeroU32::new(g.maximum_prebuild_cache_size_bytes.as_u128() as u32)
.expect("Non-zero max prebuild cache size");
.ok_or_else(|| anyhow!("maximum_prebuild_cache_size_bytes must be non-zero"))?;
generate_and_check(&g.variant, g.seed, total_bytes, g.maximum_block_size, args)
}
generator::Inner::ProcessTree(_) => {
Expand Down
38 changes: 27 additions & 11 deletions lading/src/generator/grpc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ pub enum Error {
#[source]
source: Box<tonic::Status>,
},
/// `config.target_uri` is not a valid URI, or is missing the required
/// path-and-query component.
#[error("invalid target_uri {uri}: {reason}")]
InvalidTargetUri {
/// User-supplied URI string.
uri: String,
/// Cause of rejection (URI parse error or missing path).
reason: String,
},
}

impl From<tonic::Status> for Error {
Expand Down Expand Up @@ -185,17 +194,16 @@ impl Grpc {
///
/// # Errors
///
/// Creation will fail if the underlying governor capacity exceeds u32.
/// Creation will fail if the underlying governor capacity exceeds u32, or
/// returns [`Error::InvalidTargetUri`] when `config.target_uri` fails to
/// parse or has no path-and-query component (e.g. `http://host` with no
/// `/service/Method` suffix).
///
/// # Panics
///
/// Function will panic if user has passed zero values for any byte
/// values. Sharp corners.
#[expect(clippy::cast_possible_truncation)]
#[expect(
clippy::expect_used,
reason = "FIXME: config.target_uri is user-supplied; parsing and the required path_and_query check should surface as Error variants instead of panicking. Tracked for follow-up."
)]
pub fn new(
general: General,
config: Config,
Expand Down Expand Up @@ -227,12 +235,20 @@ impl Grpc {
)?,
};

let target_uri =
uri::Uri::try_from(config.target_uri.clone()).expect("target_uri must be valid");
let rpc_path = target_uri
.path_and_query()
.cloned()
.expect("target_uri should have an RPC path");
let target_uri = uri::Uri::try_from(config.target_uri.clone()).map_err(|source| {
Error::InvalidTargetUri {
uri: config.target_uri.clone(),
reason: source.to_string(),
}
})?;
let rpc_path =
target_uri
.path_and_query()
.cloned()
.ok_or_else(|| Error::InvalidTargetUri {
uri: config.target_uri.clone(),
reason: "missing path-and-query component".to_string(),
})?;
Ok(Self {
target_uri,
rpc_path,
Expand Down
31 changes: 19 additions & 12 deletions lading/src/generator/tcp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,18 @@ pub enum Error {
#[source]
source: Box<std::io::Error>,
},
/// Failed to resolve `config.addr` to a socket address.
#[error("Failed to resolve TCP address {addr}: {source}")]
AddrParse {
/// User-supplied address string.
addr: String,
/// Underlying resolver error.
#[source]
source: Box<std::io::Error>,
},
/// `config.addr` resolved to an empty set of socket addresses.
#[error("TCP address {0} resolved to no socket addresses")]
EmptyAddrResolution(String),
}

#[derive(Debug)]
Expand All @@ -124,17 +136,9 @@ impl Tcp {
///
/// # Errors
///
/// Creation will fail if the underlying governor capacity exceeds u32.
///
/// # Panics
///
/// Function will panic if user has passed zero values for any byte
/// values. Sharp corners.
/// Creation will fail if the underlying governor capacity exceeds u32, or
/// if `config.addr` cannot be resolved to a socket address.
#[expect(clippy::cast_possible_truncation)]
#[expect(
clippy::expect_used,
reason = "FIXME: config.addr is user-supplied; socket address parsing failures should surface as Error variants instead of panicking. Tracked for follow-up."
)]
pub fn new(
general: General,
config: &Config,
Expand All @@ -158,9 +162,12 @@ impl Tcp {
let addr = config
.addr
.to_socket_addrs()
.expect("could not convert to socket")
.map_err(|source| Error::AddrParse {
addr: config.addr.clone(),
source: Box::new(source),
})?
.next()
.expect("could not convert to socket addr");
.ok_or_else(|| Error::EmptyAddrResolution(config.addr.clone()))?;

let concurrency = ConcurrencyStrategy::new(
NonZeroU16::new(config.parallel_connections),
Expand Down
16 changes: 6 additions & 10 deletions lading/src/generator/tcp_rr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ pub enum Error {
/// Invalid configuration.
#[error("invalid config: {0}")]
Config(String),
/// `config.addr` is not a valid IP address.
#[error("invalid addr: {0}")]
InvalidAddr(#[from] std::net::AddrParseError),
}

#[derive(Debug)]
Expand Down Expand Up @@ -123,15 +126,8 @@ impl TcpRr {
///
/// # Errors
///
/// Returns an error if a worker thread panics.
///
/// # Panics
///
/// Panics if `addr` cannot be resolved to a socket address.
#[expect(
clippy::expect_used,
reason = "FIXME: config.addr is user-supplied; parse failure should surface as an Error variant instead of panicking. Tracked for follow-up."
)]
/// Returns an error if `config.addr` is not a valid IP address or if a
/// worker thread panics.
pub async fn spin(self) -> Result<(), Error> {
if self.config.threads > self.config.flows {
return Err(Error::Config(format!(
Expand All @@ -140,7 +136,7 @@ impl TcpRr {
)));
}

let ip: IpAddr = self.config.addr.parse().expect("invalid addr");
let ip: IpAddr = self.config.addr.parse()?;
let data_addr = SocketAddr::new(ip, self.config.data_port);
let control_addr = SocketAddr::new(ip, self.config.control_port);

Expand Down
31 changes: 19 additions & 12 deletions lading/src/generator/udp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,18 @@ pub enum Error {
#[source]
source: Box<std::io::Error>,
},
/// Failed to resolve `config.addr` to a socket address.
#[error("Failed to resolve UDP address {addr}: {source}")]
AddrParse {
/// User-supplied address string.
addr: String,
/// Underlying resolver error.
#[source]
source: Box<std::io::Error>,
},
/// `config.addr` resolved to an empty set of socket addresses.
#[error("UDP address {0} resolved to no socket addresses")]
EmptyAddrResolution(String),
}

#[derive(Debug)]
Expand All @@ -135,17 +147,9 @@ impl Udp {
///
/// # Errors
///
/// Creation will fail if the underlying governor capacity exceeds u32.
///
/// # Panics
///
/// Function will panic if user has passed zero values for any byte
/// values. Sharp corners.
/// Creation will fail if the underlying governor capacity exceeds u32, or
/// if `config.addr` cannot be resolved to a socket address.
#[expect(clippy::cast_possible_truncation)]
#[expect(
clippy::expect_used,
reason = "FIXME: config.addr is user-supplied; socket address parsing failures should surface as Error variants instead of panicking. Tracked for follow-up."
)]
pub fn new(
general: General,
config: &Config,
Expand All @@ -169,9 +173,12 @@ impl Udp {
let addr = config
.addr
.to_socket_addrs()
.expect("could not convert to socket")
.map_err(|source| Error::AddrParse {
addr: config.addr.clone(),
source: Box::new(source),
})?
.next()
.expect("could not convert to socket addr");
.ok_or_else(|| Error::EmptyAddrResolution(config.addr.clone()))?;

let concurrency = ConcurrencyStrategy::new(
NonZeroU16::new(config.parallel_connections),
Expand Down
Loading