fix Prevent command-line panics on malformed user input #1997
Conversation
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Review: ✅ Approved
Fixes multiple panic scenarios in the CLI argument parser.
Changes:
--usercolor: Added bounds check to prevent buffer overflow (takes first 7 chars)atol(): Replacedunwrap()with proper error handling for buffer size parsing- Timestamp options:
--startat,--endat,--startcredits*,--endcredits*now handle invalid input gracefully - Service parsing: CEA-708 service numbers handle parse errors
Testing:
$ ./ccextractor --startat "invalid" /dev/null
Error: Malformed parameter: --startat 'invalid' is invalid.
$ ./ccextractor --buffersize "abc" /dev/null
Error: Malformed parameter: Invalid buffer size number 'ab'.
$ ./ccextractor --buffersize "" /dev/null
Error: Malformed parameter: empty buffer size.All tests produce clean error messages instead of Rust panics.
Verdict: Ready to merge. Good defensive coding!
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Actually I don't think this is very rustic. We don't want unwraps(), but I think it's possible for clap to reject bad args and provide good feedback.
There's no need to fatal() here
|
Hi @cfsmp3, Thanks for your feedback,I have updated the code to replace fatal!() with panic!()/expect() for inputs already validated by clap. The idea was to avoid abrupt crashes while keeping clear messages, but now all inputs that clap validates will never reach these panics, so they act as a safeguard rather than normal execution paths |
What I mean is that you can pass a function to clap to do the validation (and return true or false). Take a look at value_parser. It's really important that the code quality and the user experience goes up with each PR. fatal, unwrap, expect, none of those are good things are should be prevented. Also, recommendation: Focus on the CCExtractor logic and what it does. That what matters - focusing on fixing generic bugs without really understand the core of the program is not a good strategy for GSoC, for example. |
cfsmp3
left a comment
cfsmp3
left a comment
There was a problem hiding this comment.
Review: Changes Requested
Thanks for working on this, but the current approach isn't quite right. The issue isn't how we report errors in parser.rs - it's that validation should happen in clap, not after parsing.
The Problem
Replacing fatal!() with panic!() actually makes things worse:
fatal!()gives users a clean error message and exits gracefullypanic!()dumps a stack trace and crashes with "core dumped"
I tested with invalid inputs:
# Master (with fatal!)
$ ./ccextractor /dev/null --quant 5
Error: Invalid quant value
Issues? Open a ticket here
https://github.com/CCExtractor/ccextractor/issues
# PR (with panic!)
$ ./ccextractor /dev/null --quant 5
thread '<unnamed>' panicked at src/parser.rs:760:17:
Invalid quant value (validated by clap)
fatal runtime error: failed to initiate panic, error 5, aborting
Aborted (core dumped)
Also, the comment "(validated by clap)" isn't accurate - clap doesn't validate these constraints currently.
The Solution: Validate in args.rs using clap
The validation should happen in clap itself. Here's how:
1. Numeric ranges (--quant, --oem, --psm)
Use value_parser with .range():
// In args.rs - current:
#[arg(long, value_name="mode")]
pub quant: Option<u8>,
// Should be:
#[arg(long, value_name="mode", value_parser = clap::value_parser!(u8).range(0..=2))]
pub quant: Option<u8>,
// Same for --oem (0-2) and --psm (0-13):
#[arg(long, value_parser = clap::value_parser!(u8).range(0..=2))]
pub oem: Option<u8>,
#[arg(long, value_parser = clap::value_parser!(u8).range(0..=13))]
pub psm: Option<u8>,This gives users a clean error from clap:
$ ./ccextractor /dev/null --quant 5
error: invalid value '5' for '--quant <mode>': 5 is not in 0..=2
2. Custom format validation (--defaultcolor, --startat, --buffersize)
Define validation functions and use them with value_parser:
// Color validation (#RRGGBB format)
fn parse_color(s: &str) -> Result<String, String> {
if s.len() == 7
&& s.starts_with('#')
&& s[1..].chars().all(|c| c.is_ascii_hexdigit())
{
Ok(s.to_string())
} else {
Err(format!(
"invalid color '{}': expected #RRGGBB format (e.g., #FF0000)",
s
))
}
}
#[arg(long, value_parser = parse_color)]
pub defaultcolor: Option<String>,// Time validation (seconds, MM:SS, or HH:MM:SS)
fn parse_time(s: &str) -> Result<String, String> {
// Reuse the existing stringztoms logic
if crate::parser::stringztoms(s).is_some() {
Ok(s.to_string())
} else {
Err(format!(
"invalid time '{}': expected seconds, MM:SS, or HH:MM:SS",
s
))
}
}
#[arg(long, value_parser = parse_time)]
pub startat: Option<String>,
#[arg(long, value_parser = parse_time)]
pub endat: Option<String>,
// Same for --startcreditsnotbefore, --startcreditsnotafter, etc.// Buffer size validation (e.g., "16M", "512K")
fn parse_buffersize(s: &str) -> Result<String, String> {
if s.is_empty() {
return Err("buffer size cannot be empty".to_string());
}
let suffix = s.chars().last().unwrap().to_ascii_uppercase();
if !['K', 'M', 'G'].contains(&suffix) && !suffix.is_ascii_digit() {
return Err(format!(
"invalid buffer size '{}': expected number with K/M/G suffix (e.g., 16M)",
s
));
}
let num_part = if suffix.is_ascii_digit() { s } else { &s[..s.len()-1] };
if num_part.parse::<i32>().is_err() {
return Err(format!("invalid buffer size '{}': not a valid number", s));
}
Ok(s.to_string())
}
#[arg(long, value_parser = parse_buffersize)]
pub buffersize: Option<String>,3. After adding clap validation
Once clap validates these inputs, the checks in parser.rs become truly unreachable. You can either:
- Remove them entirely (preferred - less code)
- Replace with
unreachable!("validated by clap")if you want to be defensive
Summary
The changes should be in args.rs, not parser.rs. Add proper value_parser constraints so that:
- Invalid input is rejected by clap with clean error messages
- The code in
parser.rsnever sees invalid values - No
panic!(), nofatal!(), nounwrap()needed
This is the idiomatic Rust/clap approach and gives users the best experience.
|
Thank you! I have updated the code to move CLI validation into clap using value_parser for numeric ranges, colors, times, and buffer sizes. I also removed panic!()/fatal!() in parser.rs for inputs now validated by clap I’m still learning ,so I’d appreciate any feedback on improvements |
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit b39f923...:
Your PR breaks these cases:
Congratulations: Merging this PR would fix the following tests:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit 0228fbc...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
3 participants
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Description
fixes multiple panic scenarios in
parser.rscaused by malformed or unexpectedcommand-line arguments.
Issues fixed
--usercolorunwrap()panics when parsing numeric CLI valuesfatal!(MalformedParameter)errorsImpact
Changes made
unwrap()calls with safe error handlingVerification