-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix symlink handling #9363
Fix symlink handling #9363
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -686,17 +686,25 @@ Expr * EvalState::parse( | |||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
SourcePath resolveExprPath(const SourcePath & path) | ||||||||||||||
SourcePath resolveExprPath(SourcePath path) | ||||||||||||||
{ | ||||||||||||||
unsigned int followCount = 0, maxFollow = 1024; | ||||||||||||||
|
||||||||||||||
/* If `path' is a symlink, follow it. This is so that relative | ||||||||||||||
path references work. */ | ||||||||||||||
auto path2 = path.resolveSymlinks(); | ||||||||||||||
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); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may want to say what limit it ran into?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nah, if you have more than 1024 symlinks, it's almost certainly a cycle. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then let's be explicit about intent, then users will know what to do with the error:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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. |
||||||||||||||
if (path.lstat().type != InputAccessor::tSymlink) break; | ||||||||||||||
path = {path.accessor, CanonPath(path.readLink(), path.path.parent().value_or(CanonPath::root))}; | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if I understand correctly, this is a "shallow" version of In any case, this code should have a name and not be in the parser. Could you put this in a method like 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. |
||||||||||||||
|
||||||||||||||
/* If `path' refers to a directory, append `/default.nix'. */ | ||||||||||||||
if (path2.lstat().type == InputAccessor::tDirectory) | ||||||||||||||
return path2 + "default.nix"; | ||||||||||||||
if (path.lstat().type == InputAccessor::tDirectory) | ||||||||||||||
return path + "default.nix"; | ||||||||||||||
|
||||||||||||||
return path2; | ||||||||||||||
return path; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"test" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
import symlink-resolution/foo/overlays/overlay.nix |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"test" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
../overlays |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
import ../lib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1024 would suggest some base-2 exponential behavior.