always replace spaces in symlinks, unless string_escape=none #4837
Conversation
|
Hmm, that stuff is there since day one of udev, in the early days it was the only way to create multiple links. Not that links separated by space was ever a good idea, but maybe something is actually still using it. Can't we just add the escaping to the NVME rules like we did for md/dm? |
Because just like that "fix" only fixed it for md/dm, doing the same again would only fix it for NVMe, not for any other rule that creates symlinks. So this problem will keep happening in the future as as more rules are added that encounter the same bug, and as more people use devices with spaces in fields used for symlinks. It will be very non-obvious as to why things are failing. And, there are absolutely no rules that actually use this "feature" - are there? Why would we keep default behavior that causes hard-to-debug problems only for select users/devices, and provides no benefit to anyone? What code (besides test cases) is actually expecting this behavior? |
martinpitt
added
the
ci-fails/needs-rework 🔥
|
This also breaks |
|
pull request updated to fix test/udev-test.pl test case (by using string_escape=none for tests that use spaces in SYMLINK value), and update man page clarifying that spaces in SYMLINK values will be replaced by default, unless the string_escape=none option is used. |
|
updated pull request (with patch series of 3 patches: fix the problem, update the test cases in test/udev-test.pl, update the man page) now passes all checks. I think ci-fails/needs-rework label can be removed. |
martinpitt
removed
the
ci-fails/needs-rework 🔥
|
I think we should retain existing behaviour of creating multiple symlinks when a space is present in SYMLINKS format. This is a long-standing and known behaviour. But only when it is present directly in the specified string, and not when the space comes from an expanded format. Hence:
This will allow us to catch the common mistake, while retaining compatibility. Of course behaviour will change if somebody was counting on getting multiple symlinks from an expanded pattern, but I'd say that's rather insane. |
I'm +1 on that, as it will fix the real problem here (unexpected spaces in the expanded variables). Any spaces written explicitly into a SYMLINK value, not coming from variable expansion, certainly aren't unexpected spaces. Kay et. al., is that approach acceptable? I can update my pull request if so. |
|
@ddstreet can you update your PR? I don't see any opposition. |
poettering
added
the
reviewed/needs-rework 🔨
…s in dest Instead of returning 0, which is unhelpful, return the number of chars copied into the dest string. This allows callers that care about that to easily use it, instead of having to calculate the strlen. No current users of the function check the return value, so this does not break any existing code; it is used in the following patch.
If replace_whitespace is true, each substitution value has all its
whitespace removed/replaced by util_replace_whitespace (except the
SUBST_RESULT substitution - $result{} or %c{} - which handles spaces
itself as field separators). All existing callers are updated to
pass false, so no functional change is made by this patch.
This is needed so the SYMLINK assignment can replace any spaces
introduced through variable substitution, becuase the SYMLINK value is
a space-separated list of symlinks to create. Any variables that
contain spaces will thus unexpectedly change the symlink value from
a single symlink to multiple incorrectly-named symlinks.
This is used in the next patch, which enables the whitespace
replacement for SYMLINK variable substitution.
If the string_escape option is either unset or 'replace' (i.e. if it is not 'none'), then enable whitespace replacement in SYMLINK variable substitution values, as added in the last patch. This will keep any whitespace that is directly contained in a SYMLINK value, but will replace any whitespace that is added to the SYMLINK value as a result of variable substitution (except $result/%c). This fixes bug 4833.
I updated the pull request as discussed, to replace all whitespace in SYMLINK variable substitution values (except $result/%c substitution, as that handles spaces itself). It's a bit more code than before, but stays very close to the original behavior, while still fixing the actual bug. It doesn't require any changes to test cases, since they only (currently) test hardcoded spaces and multi-field %c substitution, which both are unaffected by this pull request (their spaces aren't replaced). The only SYMLINK assignments that would break from this change are those that expect spaces in variable values, and thus expect a single (non-%c) variable value to create multiple symlinks; and that is an insane expectation, and nobody should be doing that. |
|
@kaysievers any complaints/suggestions for the new patch series? |
keszybz
removed
the
reviewed/needs-rework 🔨
|
OK, let's give this a go. |
The default when adding a symlink string is to leave spaces in the string,
but since the list of symlinks to create is space-separated, leaving
spaces in individual symlink values creates multiple symlinks, instead of
one symlink with spaces in the name; which is never what is actually
desired. This was fixed only for md and dm a long, long time ago with
commmit ee173c5 ("replace spaces in dm
and md name symlinks"), but this applies to all devices that create
symlinks; the default should always be to replace spaces, as any rules
that actually do want to specify multiple symlinks at once should manually
specify string_escape=none (or use multiple SYMLINK+="" rules), instead of
every other rule needing to specify string_escape=replace (or every other
rule introducing bugs if they don't specify string_escape=replace).
This is not a widespread problem currently because most of the rules that
create symlinks use values that are already whitespace-replaced, e.g.
both the 'scsi_id' and 'ata_id' programs already replace all whitespace
in SCSI/ATA device model/serial strings that are used to create the
disk/by-id symlinks. However new buses like NVMe don't use a separate
program; NVMe rules directly use the sysfs attr values for model and
serial strings, which can contain spaces, and break symlinks when they
do contain spaces.
Fixes: #4833