Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix symlink handling #9363

Merged
merged 1 commit into from Nov 17, 2023
Merged

Fix symlink handling #9363

merged 1 commit into from Nov 17, 2023

Conversation

edolstra
Copy link
Member

Motivation

This restores the symlink handling behaviour prior to 94812cc.

Fixes #9298.

Context

Priorities

Add 馃憤 to pull requests you find important.

@edolstra
Fix symlink handling
31ebc60 
This restores the symlink handling behaviour prior to
94812cc.

Fixes NixOS#9298.
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 16, 2023
@Ericson2314
Copy link
Member

Do you think this will fix #9306 ?

fricklerhandwerk
fricklerhandwerk reviewed Nov 16, 2023
View reviewed changes
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
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);

Copy link
Member

Choose a reason for hiding this comment

The 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
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.

@fricklerhandwerk fricklerhandwerk added the regression Something doesn't work anymore label Nov 16, 2023
@roberth roberth added backport 2.16-maintenance Automatically creates a PR against the branch backport 2.17-maintenance Automatically creates a PR against the branch backport 2.18-maintenance Automatically creates a PR against the branch labels Nov 17, 2023
roberth
roberth reviewed Nov 17, 2023
View reviewed changes
src/libexpr/parser.y
{
unsigned int followCount = 0, maxFollow = 1024;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
unsigned int followCount = 0, maxFollow = 1024;
unsigned int followCount = 0, maxFollow = 1000;

1024 would suggest some base-2 exponential behavior.

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))};
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

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);
Copy link
Member

Choose a reason for hiding this comment

The 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
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.

@edolstra edolstra merged commit 3a7f024 into NixOS:master Nov 17, 2023
12 checks passed
@edolstra edolstra deleted the symlink-regression branch November 17, 2023 13:11
@github-actions GitHub Actions
Copy link

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.16-maintenance
git worktree add -d .worktree/backport-9363-to-2.16-maintenance origin/2.16-maintenance
cd .worktree/backport-9363-to-2.16-maintenance
git switch --create backport-9363-to-2.16-maintenance
git cherry-pick -x 31ebc6028b3682969d86a7b39ae87131c41cc604

@github-actions GitHub Actions
Copy link

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.17-maintenance
git worktree add -d .worktree/backport-9363-to-2.17-maintenance origin/2.17-maintenance
cd .worktree/backport-9363-to-2.17-maintenance
git switch --create backport-9363-to-2.17-maintenance
git cherry-pick -x 31ebc6028b3682969d86a7b39ae87131c41cc604

@github-actions GitHub Actions
Copy link

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-9363-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-9363-to-2.18-maintenance
git switch --create backport-9363-to-2.18-maintenance
git cherry-pick -x 31ebc6028b3682969d86a7b39ae87131c41cc604

@roberth roberth mentioned this pull request Nov 19, 2023
Open
@github-actions GitHub Actions
Copy link

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.16-maintenance
git worktree add -d .worktree/backport-9363-to-2.16-maintenance origin/2.16-maintenance
cd .worktree/backport-9363-to-2.16-maintenance
git switch --create backport-9363-to-2.16-maintenance
git cherry-pick -x 31ebc6028b3682969d86a7b39ae87131c41cc604

@github-actions GitHub Actions
Copy link

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

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.17-maintenance
git worktree add -d .worktree/backport-9363-to-2.17-maintenance origin/2.17-maintenance
cd .worktree/backport-9363-to-2.17-maintenance
git switch --create backport-9363-to-2.17-maintenance
git cherry-pick -x 31ebc6028b3682969d86a7b39ae87131c41cc604

@github-actions GitHub Actions
Copy link

Successfully created backport PR for 2.18-maintenance:

@github-actions github-actions bot mentioned this pull request Jan 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth left review comments

@fricklerhandwerk fricklerhandwerk fricklerhandwerk left review comments

Assignees
No one assigned
Labels
backport 2.16-maintenance Automatically creates a PR against the branch backport 2.17-maintenance Automatically creates a PR against the branch backport 2.18-maintenance Automatically creates a PR against the branch bug regression Something doesn't work anymore with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression in symlink handling in 2.16
4 participants
@edolstra @Ericson2314 @roberth @fricklerhandwerk