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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nix shell: Handle output paths that are symlinks #10467

Merged
merged 3 commits into from
Apr 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/libstore/local-fs-store.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ struct LocalStoreAccessor : PosixSourceAccessor

std::optional<Stat> maybeLstat(const CanonPath & path) override
{
/* Handle the case where `path` is (a parent of) the store. */
if (isDirOrInDir(store->storeDir, path.abs()))
return Stat{ .type = tDirectory };
Comment on lines +37 to +38
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear why this is needed (calling it with a parent of the store feels quite wrong).

I guess it should at least honor requireValidPath

Copy link
Member Author

Choose a reason for hiding this comment

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

Because when following a symlink from one store path to another, resolveSymlinks() will stat the parents of those paths, so it will do a maybeLstat("/nix") and maybeLstat("/nix/store"). Those are not valid store paths so it failed.

Copy link
Member

Choose a reason for hiding this comment

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

I've been thinking about whether stores' source accessors should prefix everything with the store directory or not, and leaning towards "no".

This would be something not doing that helps with: the path is just xxxxxxxxxxxx-foo, no parent directories etc.

Copy link
Member

Choose a reason for hiding this comment

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

makeMountedInputAccessor can be used to put put things in store directories if it matters.

Copy link
Member

Choose a reason for hiding this comment

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

#10510 Broke into new issue

Copy link
Member

Choose a reason for hiding this comment

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

Given that #10510 isn't fixed, I think this is actually a fine fix for now.


return PosixSourceAccessor::maybeLstat(toRealPath(path));
}

Expand Down
44 changes: 41 additions & 3 deletions src/libutil/source-accessor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ void SourceAccessor::readFile(
}

Hash SourceAccessor::hashPath(
const CanonPath & path,
PathFilter & filter,
HashAlgorithm ha)
const CanonPath & path,
PathFilter & filter,
HashAlgorithm ha)
{
HashSink sink(ha);
dumpPath(path, sink, filter);
Expand All @@ -67,4 +67,42 @@ std::string SourceAccessor::showPath(const CanonPath & path)
return displayPrefix + path.abs() + displaySuffix;
}

CanonPath SourceAccessor::resolveSymlinks(
const CanonPath & path,
SymlinkResolution mode)
{
auto res = CanonPath::root;

int linksAllowed = 1024;

std::list<std::string> todo;
for (auto & c : path)
todo.push_back(std::string(c));

while (!todo.empty()) {
auto c = *todo.begin();
todo.pop_front();
if (c == "" || c == ".")
;
else if (c == "..")
res.pop();
else {
res.push(c);
if (mode == SymlinkResolution::Full || !todo.empty()) {
if (auto st = maybeLstat(res); st && st->type == SourceAccessor::tSymlink) {
if (!linksAllowed--)
throw Error("infinite symlink recursion in path '%s'", showPath(path));
auto target = readLink(res);
res.pop();
if (hasPrefix(target, "/"))
res = CanonPath::root;
todo.splice(todo.begin(), tokenizeString<std::list<std::string>>(target, "/"));
}
}
}
}

return res;
}
Comment on lines +70 to +106
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be deduped

Copy link
Member

Choose a reason for hiding this comment

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

With the code in src/libutil/file-path-impl.hh

See the implementation of canonPath in src/libutil/file-system.cc

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not the job of this PR though. It just moves resolveSymlinks().

Copy link
Member

Choose a reason for hiding this comment

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

OK fair


}
37 changes: 34 additions & 3 deletions src/libutil/source-accessor.hh
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,26 @@ namespace nix {

struct Sink;

/**
* Note there is a decent chance this type soon goes away because the problem is solved another way.
* See the discussion in https://github.com/NixOS/nix/pull/9985.
*/
enum class SymlinkResolution {
/**
* Resolve symlinks in the ancestors only.
*
* Only the last component of the result is possibly a symlink.
*/
Ancestors,

/**
* Resolve symlinks fully, realpath(3)-style.
*
* No component of the result will be a symlink.
*/
Full,
};

/**
* A read-only filesystem abstraction. This is used by the Nix
* evaluator and elsewhere for accessing sources in various
Expand Down Expand Up @@ -112,9 +132,9 @@ struct SourceAccessor
PathFilter & filter = defaultPathFilter);

Hash hashPath(
const CanonPath & path,
PathFilter & filter = defaultPathFilter,
HashAlgorithm ha = HashAlgorithm::SHA256);
const CanonPath & path,
PathFilter & filter = defaultPathFilter,
HashAlgorithm ha = HashAlgorithm::SHA256);

/**
* Return a corresponding path in the root filesystem, if
Expand All @@ -137,6 +157,17 @@ struct SourceAccessor
void setPathDisplay(std::string displayPrefix, std::string displaySuffix = "");

virtual std::string showPath(const CanonPath & path);

/**
* Resolve any symlinks in `path` according to the given
* resolution mode.
*
* @param mode might only be a temporary solution for this.
* See the discussion in https://github.com/NixOS/nix/pull/9985.
*/
CanonPath resolveSymlinks(
const CanonPath & path,
SymlinkResolution mode = SymlinkResolution::Full);
};

}
38 changes: 0 additions & 38 deletions src/libutil/source-path.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,44 +62,6 @@ bool SourcePath::operator<(const SourcePath & x) const
return std::tie(*accessor, path) < std::tie(*x.accessor, x.path);
}

SourcePath SourcePath::resolveSymlinks(SymlinkResolution mode) const
{
auto res = SourcePath(accessor);

int linksAllowed = 1024;

std::list<std::string> todo;
for (auto & c : path)
todo.push_back(std::string(c));

bool resolve_last = mode == SymlinkResolution::Full;

while (!todo.empty()) {
auto c = *todo.begin();
todo.pop_front();
if (c == "" || c == ".")
;
else if (c == "..")
res.path.pop();
else {
res.path.push(c);
if (resolve_last || !todo.empty()) {
if (auto st = res.maybeLstat(); st && st->type == InputAccessor::tSymlink) {
if (!linksAllowed--)
throw Error("infinite symlink recursion in path '%s'", path);
auto target = res.readLink();
res.path.pop();
if (hasPrefix(target, "/"))
res.path = CanonPath::root;
todo.splice(todo.begin(), tokenizeString<std::list<std::string>>(target, "/"));
}
}
}
}

return res;
}

std::ostream & operator<<(std::ostream & str, const SourcePath & path)
{
str << path.to_string();
Expand Down
31 changes: 5 additions & 26 deletions src/libutil/source-path.hh
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,6 @@

namespace nix {

/**
* Note there is a decent chance this type soon goes away because the problem is solved another way.
* See the discussion in https://github.com/NixOS/nix/pull/9985.
*/
enum class SymlinkResolution {
/**
* Resolve symlinks in the ancestors only.
*
* Only the last component of the result is possibly a symlink.
*/
Ancestors,

/**
* Resolve symlinks fully, realpath(3)-style.
*
* No component of the result will be a symlink.
*/
Full,
};

/**
* An abstraction for accessing source files during
* evaluation. Currently, it's just a wrapper around `CanonPath` that
Expand Down Expand Up @@ -123,14 +103,13 @@ struct SourcePath
bool operator<(const SourcePath & x) const;

/**
* Resolve any symlinks in this `SourcePath` according to the
* given resolution mode.
*
* @param mode might only be a temporary solution for this.
* See the discussion in https://github.com/NixOS/nix/pull/9985.
* Convenience wrapper around `SourceAccessor::resolveSymlinks()`.
*/
SourcePath resolveSymlinks(
SymlinkResolution mode = SymlinkResolution::Full) const;
SymlinkResolution mode = SymlinkResolution::Full) const
{
return {accessor, accessor->resolveSymlinks(path, mode)};
}
};

std::ostream & operator << (std::ostream & str, const SourcePath & path);
Expand Down
3 changes: 2 additions & 1 deletion src/nix/unix/run.cc
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,8 @@ struct CmdShell : InstallablesCommand, MixEnvironment
if (true)
pathAdditions.push_back(store->printStorePath(path) + "/bin");

auto propPath = CanonPath(store->printStorePath(path)) / "nix-support" / "propagated-user-env-packages";
auto propPath = accessor->resolveSymlinks(
CanonPath(store->printStorePath(path)) / "nix-support" / "propagated-user-env-packages");
if (auto st = accessor->maybeLstat(propPath); st && st->type == SourceAccessor::tRegular) {
for (auto & p : tokenizeString<Paths>(accessor->readFile(propPath)))
todo.push(store->parseStorePath(p));
Expand Down
10 changes: 9 additions & 1 deletion tests/functional/shell-hello.nix
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
with import ./config.nix;

{
rec {
hello = mkDerivation {
name = "hello";
outputs = [ "out" "dev" ];
Expand All @@ -24,6 +24,14 @@ with import ./config.nix;
'';
};

hello-symlink = mkDerivation {
name = "hello-symlink";
buildCommand =
''
ln -s ${hello} $out
'';
};

salve-mundi = mkDerivation {
name = "salve-mundi";
outputs = [ "out" ];
Expand Down
2 changes: 2 additions & 0 deletions tests/functional/shell.sh
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ nix shell -f shell-hello.nix hello -c hello NixOS | grep 'Hello NixOS'
nix shell -f shell-hello.nix hello^dev -c hello2 | grep 'Hello2'
nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2'

# Test output paths that are a symlink.
#nix shell -f shell-hello.nix hello-symlink -c hello | grep 'Hello World'
edolstra marked this conversation as resolved.
Show resolved Hide resolved

if isDaemonNewer "2.20.0pre20231220"; then
# Test that command line attribute ordering is reflected in the PATH
Expand Down
Loading