Fix symlink handling by edolstra · Pull Request #9363 · NixOS/nix

edolstra · 2023-11-16T15:48:07Z

Motivation

This restores the symlink handling behaviour prior to 94812cc.

Fixes #9298.

Context

Priorities

Add 👍 to pull requests you find important.

This restores the symlink handling behaviour prior to 94812cc. Fixes NixOS#9298.

Ericson2314 · 2023-11-16T16:53:36Z

Do you think this will fix #9306 ?

fricklerhandwerk · 2023-11-16T16:55:54Z

src/libexpr/parser.y

+    while (true) {
+        // Basic cycle/depth limit to avoid infinite loops.
+        if (++followCount >= maxFollow)
+            throw Error("too many symbolic links encountered while traversing the path '%s'", path);


We may want to say what limit it ran into?

Suggested change

throw Error("too many symbolic links encountered while traversing the path '%s'", path);

throw Error("too many symbolic links encountered while traversing the path '%s' (limit is %d)", path, maxFollow);

Nah, if you have more than 1024 symlinks, it's almost certainly a cycle.

Then let's be explicit about intent, then users will know what to do with the error:

Suggested change

throw Error("too many symbolic links encountered while traversing the path '%s'", path);

throw Error("more than %d symbolic links encountered while traversing the path '%s' (there is probably a cycle)", path, maxFollow);

coreutils:

$ ln -s bar foo $ ln -s foo bar $ cat foo cat: foo: Too many levels of symbolic links

Of course coreutils is kinda bad with error messages, but we can align with it for a consistent user experience and improve on it.

Suggested change

throw Error("too many symbolic links encountered while traversing the path '%s'", path);

throw Error("too many levels of symbolic links while traversing the path '%s'; assuming it's part of a cycle after %d indirections", originalPath, maxFollows);

It would also be slightly more helpful to show the original path instead of an arbitrary path within the cycle, in case it's not the cycle that's the mistake, but rather the use of the cycle. Also note that the original symlink does not even have to be part of the cycle; it only has to lead to it.

roberth · 2023-11-16T22:43:08Z

src/libexpr/parser.y

-SourcePath resolveExprPath(const SourcePath & path)
+SourcePath resolveExprPath(SourcePath path)
 {
+    unsigned int followCount = 0, maxFollow = 1024;


Suggested change

unsigned int followCount = 0, maxFollow = 1024;

unsigned int followCount = 0, maxFollow = 1000;

1024 would suggest some base-2 exponential behavior.

roberth · 2023-11-17T10:03:57Z

src/libexpr/parser.y

+            throw Error("too many symbolic links encountered while traversing the path '%s'", path);
+        if (path.lstat().type != InputAccessor::tSymlink) break;
+        path = {path.accessor, CanonPath(path.readLink(), path.path.parent().value_or(CanonPath::root))};
+    }


So if I understand correctly, this is a "shallow" version of resolveSymlinks that only resolves the final path component?

In any case, this code should have a name and not be in the parser. Could you put this in a method like SourcePath::followSymlink perhaps?

I haven't had enough coffee yet to really understand the subtlety between the two methods yet, so if you could describe that in the doc comments, that will probably even help those who aren't caffeine deprived.

roberth · 2023-11-17T10:18:36Z

src/libexpr/parser.y

+    while (true) {
+        // Basic cycle/depth limit to avoid infinite loops.
+        if (++followCount >= maxFollow)
+            throw Error("too many symbolic links encountered while traversing the path '%s'", path);


coreutils:

$ ln -s bar foo $ ln -s foo bar $ cat foo cat: foo: Too many levels of symbolic links

Of course coreutils is kinda bad with error messages, but we can align with it for a consistent user experience and improve on it.

Suggested change

throw Error("too many symbolic links encountered while traversing the path '%s'", path);

throw Error("too many levels of symbolic links while traversing the path '%s'; assuming it's part of a cycle after %d indirections", originalPath, maxFollows);

It would also be slightly more helpful to show the original path instead of an arbitrary path within the cycle, in case it's not the cycle that's the mistake, but rather the use of the cycle. Also note that the original symlink does not even have to be part of the cycle; it only has to lead to it.

github-actions · 2023-11-17T13:12:10Z

Backport failed for 2.16-maintenance, because it was unable to cherry-pick the commit(s).