On 14/09/2026 04:46, Collin Funk wrote:
Pádraig Brady <[email protected]> writes:
From: Ismail Ramzi <[email protected]>
parse_ls_color stores color_indicator[C_LINK].string as a pointer into
the color_buf buffer, then frees color_buf on the unparsable-value path.
The "ln=target" check after the parse loop still read that freed buffer,
e.g. with LS_COLORS='ln=target:x'. Skip it on the failure path, where
color output is disabled anyway and the buffer is gone.
* src/ls.c (parse_ls_color): Return after freeing color_buf on failure,
so the color_indicator[C_LINK] check only runs when the buffer is live.
* tests/ls/color-symlink-target.sh: New test.
* tests/local.mk (all_tests): Add it.
* NEWS: Mention the fix.
Link: https://github.com/coreutils/coreutils/pull/352
---
NEWS | 4 ++++
src/ls.c | 1 +
tests/local.mk | 1 +
tests/ls/color-symlink-target.sh | 33 ++++++++++++++++++++++++++++++++
4 files changed, 39 insertions(+)
create mode 100755 tests/ls/color-symlink-target.sh
Thankfully, it seems pretty harmless. The bad read is the here:
if (color_indicator[C_LINK].len == 6
&& !STRNCMP_LIT (color_indicator[C_LINK].string, "target"))
color_symlink_as_referent = true;
Where STRNCMP_LIT expands to:
strncmp (color_indicator[C_LINK].string, "target", 6)
and "color_indicator[C_LINK].string" is the just freed string that
previously contained a string constructed from the values parsed from
LS_COLORS.
But I agree it is worth applying before you make the release.
Right. The main potential issue is DOS I think,
where one might induce a failure in a set -e script,
by setting LS_COLORS.
On a semi-related note, I've been a bit tempted to mess with the
--enable-single-binary code a bit to allow building a library with
ls_main, etc. That would make it possible to use LibFuzzer to try to
catch stuff like this. I expiremented a bit with AFL++, which seems
fine, but I suspect it is less efficient since it needs to execute a
process for each case.
An interesting idea.
One potential issue is how for efficiency reasons we don't free all heap memory
just before exit,
which might need consideration.
cheers,
Padraig
