dd: should terminate with error if skip argument is too large #7275 #9281
Conversation
src/uu/dd/src/parseargs.rs
Outdated
| BsOutOfRange(String), | ||
| #[error("{}", translate!("dd-error-invalid-number", "input" => .0.clone()))] | ||
| InvalidNumber(String), | ||
| #[error("invalid number: ‘{0}’: {1}")] |
There was a problem hiding this comment.
Please use the translate macro here
There was a problem hiding this comment.
Could it be the case that the translate! macro has a bug and cannot be used here?
After implementing tests started failing, if I print without translate the correct numeric value is printed, otherwise it prints 9223372036854776000. When looking into the macro I noticed this:
let value_str = $value.to_string();
if let Ok(num_val) = value_str.parse::<i64>() {
args.set($key, num_val);
} else if let Ok(float_val) = value_str.parse::<f64>() {
args.set($key, float_val);
Couldn't that logic be problematic if the number is an integer but sits between i64 and u64? It would get converted to f64, which could be a bug source.
There was a problem hiding this comment.
Seems like a bug in Fluent. Even after adding an arm for u64, when args.set($key, unsigned_val) is called, the resulting object is parsing to f64 under the hood.
Even worse, for simple i64 values like 0, it is still getting converted to FluentNumber under the hood, which it only allows f64 values. I guess the whole branching across value_str.parse is completely unnecessary as everything ends up converted to FluentNumber (an f64) (reference).
How something so simple turned out in something considerably more complex 😅
@sylvestre @cakebaker This being my first time working with Rust, I may need some guidance here.
P.S: This is a documented shortcoming: projectfluent/fluent-rs#337
There was a problem hiding this comment.
Pushed a version with an ugly workaround. Will work on a more general workaround for translate! on a separate branch
There was a problem hiding this comment.
A version which adds a general (but ugly) workaround in translate! definition and consequently reverts the workarounds specific to dd/parseargs.rs.
I pushed it here for easy visualization of changes: temp-mfontanaar#1
src/uu/dd/src/parseargs.rs
Outdated
| if skip > i64::MAX as u64 { | ||
| return Err(ParseError::InvalidNumberWithErrMsg( | ||
| format!("{skip}"), | ||
| "Value too large for defined data type".to_string(), |
There was a problem hiding this comment.
Nice catch, with this I realized the tests were missing a trailing newline.
However, I cannot get the right value to be printed, see above.
|
GNU testsuite comparison: |
mfontanaar
force-pushed
the
issue_7216
branch
from
b9421d7 to
3ebbac2
Compare
3 participants
Fixes #7216
cargo run dd bs=1 skip=9223372036854775808 count=0 Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s Running `target/debug/coreutils dd bs=1 skip=9223372036854775808 count=0` dd: invalid number: ‘'9223372036854775808': Value too large for defined data type’Original PR: #7275