Skip to content

dircolors: skip invalid UTF-8 lines instead of truncating the config - #14281

Open
alexchen-sys wants to merge 1 commit into
uutils:mainfrom
alexchen-sys:fix-dircolors-invalid-utf8-skip
Open

dircolors: skip invalid UTF-8 lines instead of truncating the config#14281
alexchen-sys wants to merge 1 commit into
uutils:mainfrom
alexchen-sys:fix-dircolors-invalid-utf8-skip

Conversation

@alexchen-sys

@alexchen-sys alexchen-sys commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Problem

dircolors reads the config via BufRead::lines().map_while(Result::ok). On a line that is not valid UTF-8, lines() returns an InvalidData error and map_while ends the iteration: every line after the bad one is silently dropped, and the user gets a truncated LS_COLORS with exit code 0.

GNU dircolors tokenizes the file bytewise, so a line that fails to decode is simply ignored and reading continues. With this config:

DIR 01;31
BAD\xffLINE 99
*.txt 00;32

Current GNU master prints LS_COLORS='di=01;31:*.txt=00;32:' and exits 0, while uutils main stops after the first entry.

Fix

Read the input bytewise with split(b'\n') (stripping a trailing \r) and decode each line via String::from_utf8(...).unwrap_or_default(). A line that fails to decode becomes an empty line: the parser skips it but still counts it, so line numbers in error messages stay aligned with the file. The file and stdin paths share the new config_lines helper.

Known deviation

A line that is both single-token and invalid UTF-8 (e.g. BAD\xffLINE alone) makes GNU exit 1 with "invalid line; missing second token", because its byte tokenizer still sees one token. We normalize such a line to empty and skip it, exiting 0. I don't think this corner justifies byte-tokenizing the parser: the line is malformed either way, and the multi-token case (the realistic one) now matches GNU exactly.

Tests

Four new tests in tests/by-util/test_dircolors.rs:

  • bad line in the middle of a file keeps the remaining entries
  • same input through stdin
  • a file containing only a bad line yields an empty LS_COLORS and success
  • an error after a skipped bad line still reports the correct line number

Verified against a dircolors binary built from GNU git master (aea70b2) and the 9.11 release: bad line skipped, exit status 0, LS_COLORS output identical.

Same bug class as #12920, where date -f also silently accepted invalid UTF-8 input (fixed in #14280).

@oech3

oech3 commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Did you check that it is compatible with GNU? GNU does not reject invalid unicode at most utils generally.

@alexchen-sys

Copy link
Copy Markdown
Contributor Author

Yes — checked against GNU 9.4 before opening. The fix does not reject invalid UTF-8: the bad line is skipped, reading continues, exit code stays 0. Output matches GNU byte-for-byte on file and stdin (details and the one single-token edge case are in the Known deviation section above).

@github-actions

Copy link
Copy Markdown

Binary size comparison:

Individual binary size comparison VS main (threshold: >=5% AND >=4 KB).

Total size of compared binaries: 152.18 MB (+1004 KB, +0.65%)

Significant per-binary changes:
  comm     1.12 MB ->    2.33 MB  (+1.21 MB, +107.29%)

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/date/date-locale-hour (passes in this run but fails in the 'main' branch)

@sylvestre sylvestre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check against git HEAD for GNU binaries (not sources)
9.4 is too old

  • you have a bunch of jobs failing

@alexchen-sys
alexchen-sys force-pushed the fix-dircolors-invalid-utf8-skip branch from 8e54a98 to b4e711e Compare August 30, 2026 19:06
@alexchen-sys

Copy link
Copy Markdown
Contributor Author

Fixed and pushed, forgot --tests on my local clippy pass. Re-checked against GNU HEAD and behavior matches everywhere except the single-token case. Looks like the musl CI failure is just the cp reflink flake.

Comment thread src/uu/dircolors/src/dircolors.rs Outdated
Comment on lines +312 to +319
/// Iterate over the lines of a config file.
///
/// GNU dircolors is byte-oriented: it never validates UTF-8 and keeps reading
/// past a malformed line. A `String`-based parser cannot hold invalid UTF-8,
/// so this approximates GNU by mapping a line that fails to decode to an
/// empty line: the parser skips it but still counts it, keeping line numbers
/// in error messages aligned with the file and avoiding the silent truncation
/// that `lines().map_while(Result::ok)` caused on `InvalidData`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it really need 7 lines of comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point — trimmed to 3 lines, kept the essentials.

@alexchen-sys
alexchen-sys force-pushed the fix-dircolors-invalid-utf8-skip branch from b4e711e to fdd15ef Compare August 31, 2026 07:02
Signed-off-by: Alex Chen <l46983284@gmail.com>
@alexchen-sys
alexchen-sys force-pushed the fix-dircolors-invalid-utf8-skip branch from fdd15ef to bdf6530 Compare September 1, 2026 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants