Skip to content
This repository was archived by the owner on Apr 13, 2026. It is now read-only.

Commit 13a67bd

Browse files
unclesp1d3runclesp1d3r
committed
fix: resolve PR review findings for overflow, port exhaustion, and dead code
- Fix u16 overflow in VLAN range total calculation by using u32 (C1) - Return errors on NAT/VPN port exhaustion instead of silent duplicates (C2, C3) - Remove unused --template CLI flag and update shell completion snapshots (H2) - Remove dead configure_terminal function from validate command (H3) - Use clap conflicts_with for --count/--vlan-range mutual exclusion (H5) - Replace unwrap() on user-provided paths with proper error handling (H6) - Add division-by-zero guards in PerformanceMetrics methods (H10) Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
1 parent d1417eb commit 13a67bd

11 files changed

Lines changed: 75 additions & 53 deletions

‎src/cli/commands/generate.rs‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,10 @@ fn execute_csv_generation(args: &GenerateArgs, global: &GlobalArgs) -> Result<()
183183
let vlan_ranges = crate::cli::parse_vlan_range(vlan_range_str)
184184
.map_err(crate::model::ConfigError::validation)?;
185185

186-
let total_vlans: u16 = vlan_ranges.iter().map(|(start, end)| end - start + 1).sum();
186+
let total_vlans: u32 = vlan_ranges
187+
.iter()
188+
.map(|(start, end)| (*end - *start + 1) as u32)
189+
.sum();
187190

188191
if !global.quiet {
189192
println!(
@@ -351,10 +354,16 @@ fn execute_csv_generation(args: &GenerateArgs, global: &GlobalArgs) -> Result<()
351354
));
352355

353356
// Write firewall rules to separate CSV file
354-
let firewall_output = output_file.with_file_name(format!(
355-
"{}_firewall_rules.csv",
356-
output_file.file_stem().unwrap().to_str().unwrap()
357-
));
357+
let stem = output_file
358+
.file_stem()
359+
.and_then(|s| s.to_str())
360+
.ok_or_else(|| {
361+
crate::model::ConfigError::invalid_parameter(
362+
"output",
363+
"Output file path must have a valid filename",
364+
)
365+
})?;
366+
let firewall_output = output_file.with_file_name(format!("{stem}_firewall_rules.csv"));
358367

359368
write_firewall_rules_csv(&firewall_rules, &firewall_output)
360369
.with_context(|| format!("Failed to write firewall rules to {:?}", firewall_output))?;
@@ -395,7 +404,10 @@ fn execute_xml_generation(args: &GenerateArgs, global: &GlobalArgs) -> Result<()
395404
let vlan_ranges = crate::cli::parse_vlan_range(vlan_range_str)
396405
.map_err(crate::model::ConfigError::validation)?;
397406

398-
let total_vlans: u16 = vlan_ranges.iter().map(|(start, end)| end - start + 1).sum();
407+
let total_vlans: u32 = vlan_ranges
408+
.iter()
409+
.map(|(start, end)| (*end - *start + 1) as u32)
410+
.sum();
399411

400412
if !global.quiet {
401413
println!(

‎src/cli/commands/validate.rs‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,6 @@ use std::path::Path;
1414

1515
/// Execute validation with global arguments
1616
pub fn execute_with_global(args: ValidateArgs, global: &GlobalArgs) -> Result<()> {
17-
// Apply global settings
18-
configure_terminal(global);
19-
2017
execute(args, global)
2118
}
2219

@@ -180,13 +177,6 @@ fn determine_format(input: &Path, format: &ValidationFormat) -> Result<Validatio
180177
}
181178
}
182179

183-
/// Configure terminal output based on global settings
184-
fn configure_terminal(global: &GlobalArgs) {
185-
// Colors are automatically disabled by checking env::var("NO_COLOR").is_ok()
186-
// and env::var("TERM") == "dumb" in the progress bar and console styling code
187-
let _ = global; // Suppress unused parameter warnings
188-
}
189-
190180
/// Create a progress bar with consistent styling
191181
fn create_progress_bar(message: &str) -> ProgressBar {
192182
let pb = ProgressBar::new_spinner();

‎src/cli/mod.rs‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ pub struct GenerateArgs {
208208
pub firewall_rule_complexity: String,
209209

210210
/// VLAN range specification (e.g., "100-150" or "10,20,30-40")
211-
#[arg(long)]
211+
#[arg(long, conflicts_with = "count")]
212212
pub vlan_range: Option<String>,
213213

214214
/// Number of VPN configurations to generate
@@ -222,10 +222,6 @@ pub struct GenerateArgs {
222222
/// WAN assignment strategy for VLANs
223223
#[arg(long, value_enum)]
224224
pub wan_assignments: Option<WanAssignmentStrategy>,
225-
226-
/// Custom XML template file
227-
#[arg(long)]
228-
pub template: Option<PathBuf>,
229225
}
230226

231227
impl GenerateArgs {
@@ -244,11 +240,6 @@ impl GenerateArgs {
244240
self.validate_vlan_range(vlan_range)?;
245241
}
246242

247-
// Validate conflicts between count and vlan_range
248-
if self.vlan_range.is_some() && self.count != 10 {
249-
return Err("Cannot specify both --count and --vlan-range. Use --vlan-range to specify exact VLANs or --count for auto-generated ranges.".to_string());
250-
}
251-
252243
Ok(())
253244
}
254245

@@ -257,9 +248,9 @@ impl GenerateArgs {
257248
let ranges = parse_vlan_range(vlan_range)
258249
.map_err(|e| format!("Invalid VLAN range format '{}': {}", vlan_range, e))?;
259250

260-
let total_vlans = ranges.iter().map(|r| r.1 - r.0 + 1).sum::<u16>();
251+
let total_vlans: u32 = ranges.iter().map(|r| (r.1 - r.0 + 1) as u32).sum();
261252

262-
if matches!(self.format, OutputFormat::Xml) && total_vlans > MAX_UNIQUE_VLAN_IDS {
253+
if matches!(self.format, OutputFormat::Xml) && total_vlans > MAX_UNIQUE_VLAN_IDS as u32 {
263254
return Err(format!(
264255
"VLAN range produces {} VLANs, but maximum is {} for XML format",
265256
total_vlans, MAX_UNIQUE_VLAN_IDS

‎src/generator/nat.rs‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ impl NatGenerator {
217217
let name = self.generate_unique_name(&rule_type);
218218
let protocol = self.random_protocol();
219219
let (source, source_port) = self.generate_source(&rule_type);
220-
let (destination, destination_port) = self.generate_destination(&rule_type, &protocol);
220+
let (destination, destination_port) = self.generate_destination(&rule_type, &protocol)?;
221221
let interface = self.random_interface(&rule_type);
222222
let (target_ip, target_port) =
223223
self.generate_target(&rule_type, &destination_port, &protocol);
@@ -375,10 +375,10 @@ impl NatGenerator {
375375
&mut self,
376376
rule_type: &NatRuleType,
377377
protocol: &str,
378-
) -> (String, String) {
379-
match rule_type {
378+
) -> NatResult<(String, String)> {
379+
Ok(match rule_type {
380380
NatRuleType::PortForward => {
381-
let port = self.generate_unique_external_port();
381+
let port = self.generate_unique_external_port()?;
382382
("any".to_string(), port.to_string())
383383
}
384384
NatRuleType::SourceNat => ("any".to_string(), "any".to_string()),
@@ -392,7 +392,7 @@ impl NatGenerator {
392392
}
393393
NatRuleType::OneToOneNat => ("any".to_string(), "any".to_string()),
394394
NatRuleType::OutboundNat => ("any".to_string(), "any".to_string()),
395-
}
395+
})
396396
}
397397

398398
/// Generate interface based on rule type
@@ -454,27 +454,35 @@ impl NatGenerator {
454454
}
455455

456456
/// Generate a unique external port for port forwarding
457-
fn generate_unique_external_port(&mut self) -> u16 {
457+
fn generate_unique_external_port(&mut self) -> NatResult<u16> {
458458
const COMMON_PORTS: &[u16] = &[80, 443, 22, 21, 25, 53, 110, 143, 993, 995, 3389, 5900];
459459
const MAX_ATTEMPTS: usize = 100;
460460

461461
// Try common ports first
462462
for &port in COMMON_PORTS {
463463
if self.used_external_ports.insert(port) {
464-
return port;
464+
return Ok(port);
465465
}
466466
}
467467

468468
// Try random ports
469469
for _ in 0..MAX_ATTEMPTS {
470470
let port = self.rng.random_range(1024..=65535);
471471
if self.used_external_ports.insert(port) {
472-
return port;
472+
return Ok(port);
473+
}
474+
}
475+
476+
// Linear scan as final fallback
477+
for port in 1024..=65535 {
478+
if self.used_external_ports.insert(port) {
479+
return Ok(port);
473480
}
474481
}
475482

476-
// Fallback
477-
8080
483+
Err(ConfigError::validation(
484+
"Unable to generate unique external port: all ports exhausted".to_string(),
485+
))
478486
}
479487

480488
/// Generate a service port

‎src/generator/performance.rs‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,23 @@ pub struct PerformanceMetrics {
4040

4141
impl PerformanceMetrics {
4242
/// Calculate throughput (configs per second)
43+
///
44+
/// Returns 0.0 if generation time is zero.
4345
pub fn throughput(&self) -> f64 {
44-
self.configs_generated as f64 / self.generation_time.as_secs_f64()
46+
let secs = self.generation_time.as_secs_f64();
47+
if secs == 0.0 {
48+
return 0.0;
49+
}
50+
self.configs_generated as f64 / secs
4551
}
4652

4753
/// Calculate memory efficiency (bytes per config)
54+
///
55+
/// Returns 0.0 if no configs were generated.
4856
pub fn memory_efficiency(&self) -> f64 {
57+
if self.configs_generated == 0 {
58+
return 0.0;
59+
}
4960
self.memory_used as f64 / self.configs_generated as f64
5061
}
5162

‎src/generator/vlan.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -673,7 +673,10 @@ pub fn generate_vlan_configurations_from_ranges(
673673
let mut generator = VlanGenerator::new_with_std_rng(seed);
674674

675675
// Calculate total number of VLANs for progress tracking and pre-allocation
676-
let total_vlans: u16 = vlan_ranges.iter().map(|(start, end)| end - start + 1).sum();
676+
let total_vlans: u32 = vlan_ranges
677+
.iter()
678+
.map(|(start, end)| (*end - *start + 1) as u32)
679+
.sum();
677680
let mut configs = Vec::with_capacity(total_vlans as usize);
678681
let mut processed = 0u64;
679682

@@ -711,7 +714,10 @@ pub fn generate_vlan_configurations_from_ranges_with_wan(
711714
let mut generator = VlanGenerator::new_with_std_rng(seed);
712715

713716
// Calculate total number of VLANs for progress tracking and pre-allocation
714-
let total_vlans: u16 = vlan_ranges.iter().map(|(start, end)| end - start + 1).sum();
717+
let total_vlans: u32 = vlan_ranges
718+
.iter()
719+
.map(|(start, end)| (*end - *start + 1) as u32)
720+
.sum();
715721
let mut configs = Vec::with_capacity(total_vlans as usize);
716722
let mut processed = 0u64;
717723
let mut vlan_index = 0usize;

‎src/generator/vpn.rs‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,7 @@ impl VpnGenerator {
173173
let vpn_type = vpn_type.unwrap_or_else(|| self.random_vpn_type());
174174
let name = self.generate_unique_name(&vpn_type);
175175
let server = self.generate_server_address();
176-
let port = self.generate_unique_port(&vpn_type);
176+
let port = self.generate_unique_port(&vpn_type)?;
177177
let protocol = self.get_protocol_for_type(&vpn_type);
178178
let cipher = self.get_cipher_for_type(&vpn_type);
179179
let auth_method = self.get_auth_method_for_type(&vpn_type);
@@ -299,7 +299,7 @@ impl VpnGenerator {
299299
}
300300

301301
/// Generate a unique port for the VPN type
302-
fn generate_unique_port(&mut self, vpn_type: &VpnType) -> u16 {
302+
fn generate_unique_port(&mut self, vpn_type: &VpnType) -> VpnResult<u16> {
303303
const MAX_ATTEMPTS: usize = 100;
304304

305305
let default_ports = match vpn_type {
@@ -311,7 +311,7 @@ impl VpnGenerator {
311311
// Try default ports first
312312
for &port in &default_ports {
313313
if self.used_ports.insert(port) {
314-
return port;
314+
return Ok(port);
315315
}
316316
}
317317

@@ -324,19 +324,20 @@ impl VpnGenerator {
324324
};
325325

326326
if self.used_ports.insert(port) {
327-
return port;
327+
return Ok(port);
328328
}
329329
}
330330

331-
// Fallback - find any available port
331+
// Linear scan as final fallback
332332
for port in 1024..=65535 {
333333
if self.used_ports.insert(port) {
334-
return port;
334+
return Ok(port);
335335
}
336336
}
337337

338-
// Ultimate fallback
339-
1194
338+
Err(ConfigError::validation(
339+
"Unable to generate unique VPN port: all ports exhausted".to_string(),
340+
))
340341
}
341342

342343
/// Get appropriate protocol for VPN type

‎tests/snapshots/snapshot_tests__bash_completion_script.snap‎

Lines changed: 2 additions & 1 deletion
Large diffs are not rendered by default.

0 commit comments

Comments
 (0)