stty: fix: reject "+hex" in parse_saved_state

[Rudxain]

Fixes this pitfall: rust-lang/rust-clippy#16213

Related: #9255

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

[sylvestre]

@Rudxain still draft after 2 weeks, are you going to work on it? thanks

[Rudxain]

are you going to work on it? thanks

Yes! This PR can be merged as-is, but I want to replace from_str_radix by some internal utility. I could simply write an ad-hoc hex-parser, but I assume there's already one, so it's better to reuse the code. The closest I could find was this:

coreutils/src/uucore/src/lib/features/parser/num_parser.rs

Lines 39 to 54 in e6467b1

	/// Return the digit value of a character in the given base
	fn digit(&self, c: char) -> Option<u64> {
	fn from_decimal(c: char) -> u64 {
	u64::from(c) - u64::from('0')
	}
	match self {
	Self::Binary => ('0'..='1').contains(&c).then(|| from_decimal(c)),
	Self::Octal => ('0'..='7').contains(&c).then(|| from_decimal(c)),
	Self::Decimal => c.is_ascii_digit().then(|| from_decimal(c)),
	Self::Hexadecimal => match c.to_ascii_lowercase() {
	'0'..='9' => Some(from_decimal(c)),
	c @ 'a'..='f' => Some(u64::from(c) - u64::from('a') + 10),
	_ => None,
	},
	}
	}

Should I use that? Or is there something more appropriate?

[sylvestre]

Restarted

[Rudxain]

BTW, this branch is soft-blocked on this PR. Should I include that patch into this PR, or should I keep it separate for better scoping?

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

[sylvestre]

Could you please add a test to make sure we don't regress in the future? Thanks

[github-actions]

GNU testsuite comparison:

Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/pr/bounded-memory (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/cp/link-heap is now passing!

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/date/date-locale-hour (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/tail-n0f (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/tail/retry (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/rm/many-dir-entries-vs-OOM is now passing!

[Copilot]

Pull request overview

This PR hardens stty’s saved-state parser to reject hex fields with a leading +, matching expected stty behavior and avoiding from_str_radix accepting inputs like +00.

Changes:

• Add an explicit rejection of +hex fields in parse_saved_state.
• Add tests intended to cover valid saved-state parsing and the new + rejection case.

Suppressed comments (1)

src/uu/stty/src/stty.rs:1449

• This test input also has only 4 parts, so it would return None even without the new + rejection. Build an input with the correct 4 + NCCS part count so the test specifically validates the new +hex behavior.

    #[test]
    fn test_parse_saved_state_no_plus() {
        let result = parse_saved_state("+00:+01:+ff:+7f");
        assert_eq!(&result, None);
    }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/uu/stty/src/stty.rs:1443

• The new test_sane_parse_saved_state uses an input with only 4 colon-separated parts, but parse_saved_state requires exactly 4 + nix::libc::NCCS parts, so this test will always fail. The assertion also compares Option<Vec<u32>> to Some(&[...]), which is a type mismatch.

    fn test_sane_parse_saved_state() {
        let result = parse_saved_state("00:01:ff:7f");
        assert_eq!(&result, Some(&[0, 1, 255, 127]));
    }

src/uu/stty/src/stty.rs:543

• Indexing part.as_bytes()[0] is less readable than a string predicate and relies on the preceding emptiness check to avoid panics. Using starts_with('+') is clearer and keeps the same behavior; also consider using the conventional TODO spelling in the comment.

        // TO-DO: avoid `from_str_radix`
        if part.as_bytes()[0] == b'+' {
            return None;
        }

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/uu/stty/src/stty.rs:543

• Using part.as_bytes()[0] is less idiomatic and ties the check to byte indexing. starts_with('+') is clearer and avoids manual indexing while keeping the same behavior (after the empty-string guard).

        // TO-DO: avoid `from_str_radix`
        if part.as_bytes()[0] == b'+' {
            return None;
        }

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/uu/stty/src/stty.rs:542

• The explicit rejection of leading '+' is good, but this block is a bit brittle/unclear:

• part.as_bytes()[0] is less readable than starts_with('+') and can become a panic hazard if the emptiness guard changes.
• The comment about from_str_radix being undocumented here and the TO-DO are confusing once we’re already validating the input.
  Consider simplifying to an explicit GNU-compatibility check using starts_with and dropping the TODO.

        // TO-DO: avoid `from_str_radix`
        if part.as_bytes()[0] == b'+' {
            return None;