From 51adfb9b277fe54ad03fa2c9981585f123fcc200 Mon Sep 17 00:00:00 2001 From: Valentin Gagarin Date: Sat, 2 Dec 2023 02:21:17 +0100 Subject: [PATCH 01/20] reword documentation on settings and attributes related to substitution - add links - be more concise - clarify the distinction between `preferLocalBuild` and `allowSubstitutes` --- .../src/language/advanced-attributes.md | 23 +++++-------------- src/libstore/globals.hh | 4 +--- 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/doc/manual/src/language/advanced-attributes.md b/doc/manual/src/language/advanced-attributes.md index 282b75af2..5a6c00cd4 100644 --- a/doc/manual/src/language/advanced-attributes.md +++ b/doc/manual/src/language/advanced-attributes.md @@ -257,29 +257,18 @@ Derivations can declare some infrequently used optional attributes. of the environment (typically, a few hundred kilobyte). - [`preferLocalBuild`]{#adv-attr-preferLocalBuild}\ - If this attribute is set to `true` and [distributed building is - enabled](../advanced-topics/distributed-builds.md), then, if - possible, the derivation will be built locally instead of forwarded - to a remote machine. This is appropriate for trivial builders - where the cost of doing a download or remote build would exceed - the cost of building locally. + If this attribute is set to `true` and [distributed building is enabled](../advanced-topics/distributed-builds.md), then, if possible, the derivation will be built locally instead of being forwarded to a remote machine. + This is useful for derivations that are cheapest to build locally. - [`allowSubstitutes`]{#adv-attr-allowSubstitutes}\ - If this attribute is set to `false`, then Nix will always build this - derivation; it will not try to substitute its outputs. This is - useful for very trivial derivations (such as `writeText` in Nixpkgs) - that are cheaper to build than to substitute from a binary cache. + If this attribute is set to `false`, then Nix will always build this derivation (locally or remotely); it will not try to substitute its outputs. + This is useful for derivations that are cheaper to build than to substitute. - You may disable the effects of this attibute by enabling the - `always-allow-substitutes` configuration option in Nix. + This attribute can be ignored by setting [`always-allow-substitutes`](@docroot@/command-ref/conf-file.md#conf-always-allow-substitutes) to `true`. > **Note** > - > You need to have a builder configured which satisfies the - > derivation’s `system` attribute, since the derivation cannot be - > substituted. Thus it is usually a good idea to align `system` with - > `builtins.currentSystem` when setting `allowSubstitutes` to - > `false`. For most trivial derivations this should be the case. + > If set to `false`, the [`builder`](./derivations.md#attr-builder) should be able to run on the system type specified in the [`system` attribute](./derivations.md#attr-system), since the derivation cannot be substituted. - [`__structuredAttrs`]{#adv-attr-structuredAttrs}\ If the special attribute `__structuredAttrs` is set to `true`, the other derivation diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 38b0d516c..36ba51e23 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -268,9 +268,7 @@ public: Setting alwaysAllowSubstitutes{ this, false, "always-allow-substitutes", R"( - If set to `true`, Nix will ignore the `allowSubstitutes` attribute in - derivations and always attempt to use available substituters. - For more information on `allowSubstitutes`, see [the manual chapter on advanced attributes](../language/advanced-attributes.md). + If set to `true`, Nix will ignore the [`allowSubstitutes`](@docroot@/language/advanced-attributes.md) attribute in derivations and always attempt to use [available substituters](#conf-substituters). )"}; Setting buildersUseSubstitutes{ From 368fdb482da039fd40a1a51bbf851c54f65eb4c5 Mon Sep 17 00:00:00 2001 From: Valentin Gagarin Date: Sat, 2 Dec 2023 03:06:47 +0100 Subject: [PATCH 02/20] reword description of the `builders-use-substitutes` setting --- src/libstore/globals.hh | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index 38b0d516c..dcdcd31d5 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -276,13 +276,10 @@ public: Setting buildersUseSubstitutes{ this, false, "builders-use-substitutes", R"( - If set to `true`, Nix will instruct remote build machines to use - their own binary substitutes if available. In practical terms, this - means that remote hosts will fetch as many build dependencies as - possible from their own substitutes (e.g, from `cache.nixos.org`), - instead of waiting for this host to upload them all. This can - drastically reduce build times if the network connection between - this computer and the remote build host is slow. + If set to `true`, Nix will instruct [remote build machines](#conf-builders) to use their own [`substituters`](#conf-substituters) if available. + + It means that remote build hosts will fetch as many dependencies as possible from their own substituters (e.g, from `cache.nixos.org`) instead of waiting for the local machine to upload them all. + This can drastically reduce build times if the network connection between the local machine and the remote build host is slow. )"}; Setting reservedSize{this, 8 * 1024 * 1024, "gc-reserved-space", From 91ba7b230777e3fb023bda48c269d533702e50e8 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 6 Dec 2023 12:41:47 +0100 Subject: [PATCH 03/20] isAllowedURI: Extract function and test --- src/libexpr/eval.cc | 18 +++++-- src/libexpr/eval.hh | 5 ++ tests/unit/libexpr/eval.cc | 106 +++++++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 5 deletions(-) create mode 100644 tests/unit/libexpr/eval.cc diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 9e494148e..0eb6f406e 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -599,21 +599,29 @@ void EvalState::allowAndSetStorePathString(const StorePath & storePath, Value & mkStorePathString(storePath, v); } -void EvalState::checkURI(const std::string & uri) +bool isAllowedURI(std::string_view uri, const Strings & allowedUris) { - if (!evalSettings.restrictEval) return; - /* 'uri' should be equal to a prefix, or in a subdirectory of a prefix. Thus, the prefix https://github.co does not permit access to https://github.com. Note: this allows 'http://' and 'https://' as prefixes for any http/https URI. */ - for (auto & prefix : evalSettings.allowedUris.get()) + for (auto & prefix : allowedUris) { if (uri == prefix || (uri.size() > prefix.size() && prefix.size() > 0 && hasPrefix(uri, prefix) && (prefix[prefix.size() - 1] == '/' || uri[prefix.size()] == '/'))) - return; + return true; + } + + return false; +} + +void EvalState::checkURI(const std::string & uri) +{ + if (!evalSettings.restrictEval) return; + + if (isAllowedURI(uri, evalSettings.allowedUris.get())) return; /* If the URI is a path, then check it against allowedPaths as well. */ diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index f3f6d35b9..6008c3f60 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -837,6 +837,11 @@ std::string showType(const Value & v); */ SourcePath resolveExprPath(SourcePath path); +/** + * Whether a URI is allowed, assuming restrictEval is enabled + */ +bool isAllowedURI(std::string_view uri, const Strings & allowedPaths); + struct InvalidPathError : EvalError { Path path; diff --git a/tests/unit/libexpr/eval.cc b/tests/unit/libexpr/eval.cc new file mode 100644 index 000000000..cc5d6bbfa --- /dev/null +++ b/tests/unit/libexpr/eval.cc @@ -0,0 +1,106 @@ +#include +#include + +#include "eval.hh" +#include "tests/libexpr.hh" + +namespace nix { + +TEST(nix_isAllowedURI, http_example_com) { + Strings allowed; + allowed.push_back("http://example.com"); + + ASSERT_TRUE(isAllowedURI("http://example.com", allowed)); + ASSERT_TRUE(isAllowedURI("http://example.com/foo", allowed)); + ASSERT_TRUE(isAllowedURI("http://example.com/foo/", allowed)); + ASSERT_FALSE(isAllowedURI("/", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.co", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.como", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.org", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.org/foo", allowed)); +} + +TEST(nix_isAllowedURI, http_example_com_foo) { + Strings allowed; + allowed.push_back("http://example.com/foo"); + + ASSERT_TRUE(isAllowedURI("http://example.com/foo", allowed)); + ASSERT_TRUE(isAllowedURI("http://example.com/foo/", allowed)); + ASSERT_FALSE(isAllowedURI("/foo", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.como", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.org/foo", allowed)); + // Broken? + // ASSERT_TRUE(isAllowedURI("http://example.com/foo?ok=1", allowed)); +} + +TEST(nix_isAllowedURI, http) { + Strings allowed; + allowed.push_back("http://"); + + ASSERT_TRUE(isAllowedURI("http://", allowed)); + ASSERT_TRUE(isAllowedURI("http://example.com", allowed)); + ASSERT_TRUE(isAllowedURI("http://example.com/foo", allowed)); + ASSERT_TRUE(isAllowedURI("http://example.com/foo/", allowed)); + ASSERT_TRUE(isAllowedURI("http://example.com", allowed)); + ASSERT_FALSE(isAllowedURI("/", allowed)); + ASSERT_FALSE(isAllowedURI("https://", allowed)); + ASSERT_FALSE(isAllowedURI("http:foo", allowed)); +} + +TEST(nix_isAllowedURI, https) { + Strings allowed; + allowed.push_back("https://"); + + ASSERT_TRUE(isAllowedURI("https://example.com", allowed)); + ASSERT_TRUE(isAllowedURI("https://example.com/foo", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com/https:", allowed)); +} + +TEST(nix_isAllowedURI, absolute_path) { + Strings allowed; + allowed.push_back("/var/evil"); // bad idea + + ASSERT_TRUE(isAllowedURI("/var/evil", allowed)); + ASSERT_TRUE(isAllowedURI("/var/evil/", allowed)); + ASSERT_TRUE(isAllowedURI("/var/evil/foo", allowed)); + ASSERT_TRUE(isAllowedURI("/var/evil/foo/", allowed)); + ASSERT_FALSE(isAllowedURI("/", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evi", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evilo", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evilo/", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evilo/foo", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com/var/evil", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com//var/evil", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com//var/evil/foo", allowed)); +} + +TEST(nix_isAllowedURI, file_url) { + Strings allowed; + allowed.push_back("file:///var/evil"); // bad idea + + ASSERT_TRUE(isAllowedURI("file:///var/evil", allowed)); + ASSERT_TRUE(isAllowedURI("file:///var/evil/", allowed)); + ASSERT_TRUE(isAllowedURI("file:///var/evil/foo", allowed)); + ASSERT_TRUE(isAllowedURI("file:///var/evil/foo/", allowed)); + ASSERT_FALSE(isAllowedURI("/", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evi", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evilo", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evilo/", allowed)); + ASSERT_FALSE(isAllowedURI("/var/evilo/foo", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com/var/evil", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com//var/evil", allowed)); + ASSERT_FALSE(isAllowedURI("http://example.com//var/evil/foo", allowed)); + ASSERT_FALSE(isAllowedURI("http://var/evil", allowed)); + ASSERT_FALSE(isAllowedURI("http:///var/evil", allowed)); + ASSERT_FALSE(isAllowedURI("http://var/evil/", allowed)); + ASSERT_FALSE(isAllowedURI("file:///var/evi", allowed)); + ASSERT_FALSE(isAllowedURI("file:///var/evilo", allowed)); + ASSERT_FALSE(isAllowedURI("file:///var/evilo/", allowed)); + ASSERT_FALSE(isAllowedURI("file:///var/evilo/foo", allowed)); + ASSERT_FALSE(isAllowedURI("file:///", allowed)); + ASSERT_FALSE(isAllowedURI("file://", allowed)); +} + +} // namespace nix \ No newline at end of file From 6cbba914a70eb5da6447fee5528a63723ed13245 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 6 Dec 2023 12:43:20 +0100 Subject: [PATCH 04/20] isAllowedURI: Remove incorrect note --- src/libexpr/eval.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 0eb6f406e..d8a36fa02 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -603,8 +603,7 @@ bool isAllowedURI(std::string_view uri, const Strings & allowedUris) { /* 'uri' should be equal to a prefix, or in a subdirectory of a prefix. Thus, the prefix https://github.co does not permit - access to https://github.com. Note: this allows 'http://' and - 'https://' as prefixes for any http/https URI. */ + access to https://github.com. */ for (auto & prefix : allowedUris) { if (uri == prefix || (uri.size() > prefix.size() From 1fa958dda1ef0cb37441ef8d1a84faf6d501ac12 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 6 Dec 2023 14:08:22 +0100 Subject: [PATCH 05/20] isAllowedURI: Format --- src/libexpr/eval.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index d8a36fa02..9e541f293 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -605,11 +605,14 @@ bool isAllowedURI(std::string_view uri, const Strings & allowedUris) prefix. Thus, the prefix https://github.co does not permit access to https://github.com. */ for (auto & prefix : allowedUris) { - if (uri == prefix || - (uri.size() > prefix.size() - && prefix.size() > 0 - && hasPrefix(uri, prefix) - && (prefix[prefix.size() - 1] == '/' || uri[prefix.size()] == '/'))) + if (uri == prefix + // Allow access to subdirectories of the prefix. + || (uri.size() > prefix.size() + && prefix.size() > 0 + && hasPrefix(uri, prefix) + && ( + prefix[prefix.size() - 1] == '/' + || uri[prefix.size()] == '/'))) return true; } From 79eb2920bb51c7ec9528a403986e79f04738e2be Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 6 Dec 2023 14:39:49 +0100 Subject: [PATCH 06/20] Add nix::isASCII*, locale-independent --- src/libutil/string.hh | 17 +++++++++++ tests/unit/libutil/string.cc | 59 ++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 src/libutil/string.hh create mode 100644 tests/unit/libutil/string.cc diff --git a/src/libutil/string.hh b/src/libutil/string.hh new file mode 100644 index 000000000..16ef75643 --- /dev/null +++ b/src/libutil/string.hh @@ -0,0 +1,17 @@ +#pragma once + +namespace nix { + + /** Locale-independent version of std::islower(). */ + inline bool isASCIILower(char c) { return c >= 'a' && c <= 'z'; }; + + /** Locale-independent version of std::isupper(). */ + inline bool isASCIIUpper(char c) { return c >= 'A' && c <= 'Z'; }; + + /** Locale-independent version of std::isalpha(). */ + inline bool isASCIIAlpha(char c) { return isASCIILower(c) || isASCIIUpper(c); }; + + /** Locale-independent version of std::isdigit(). */ + inline bool isASCIIDigit(char c) { return c >= '0' && c <= '9'; }; + +} diff --git a/tests/unit/libutil/string.cc b/tests/unit/libutil/string.cc new file mode 100644 index 000000000..381f2cc15 --- /dev/null +++ b/tests/unit/libutil/string.cc @@ -0,0 +1,59 @@ +#include +#include "string.hh" + +namespace nix { + +TEST(string, isASCIILower) { + ASSERT_TRUE(isASCIILower('a')); + ASSERT_TRUE(isASCIILower('z')); + ASSERT_FALSE(isASCIILower('A')); + ASSERT_FALSE(isASCIILower('Z')); + ASSERT_FALSE(isASCIILower('0')); + ASSERT_FALSE(isASCIILower('9')); + ASSERT_FALSE(isASCIILower(' ')); + ASSERT_FALSE(isASCIILower('\n')); + ASSERT_FALSE(isASCIILower('\t')); + ASSERT_FALSE(isASCIILower(':')); +} + +TEST(string, isASCIIUpper) { + ASSERT_FALSE(isASCIIUpper('a')); + ASSERT_FALSE(isASCIIUpper('z')); + ASSERT_TRUE(isASCIIUpper('A')); + ASSERT_TRUE(isASCIIUpper('Z')); + ASSERT_FALSE(isASCIIUpper('0')); + ASSERT_FALSE(isASCIIUpper('9')); + ASSERT_FALSE(isASCIIUpper(' ')); + ASSERT_FALSE(isASCIIUpper('\n')); + ASSERT_FALSE(isASCIIUpper('\t')); + ASSERT_FALSE(isASCIIUpper(':')); +} + +TEST(string, isASCIIAlpha) { + ASSERT_TRUE(isASCIIAlpha('a')); + ASSERT_TRUE(isASCIIAlpha('z')); + ASSERT_TRUE(isASCIIAlpha('A')); + ASSERT_TRUE(isASCIIAlpha('Z')); + ASSERT_FALSE(isASCIIAlpha('0')); + ASSERT_FALSE(isASCIIAlpha('9')); + ASSERT_FALSE(isASCIIAlpha(' ')); + ASSERT_FALSE(isASCIIAlpha('\n')); + ASSERT_FALSE(isASCIIAlpha('\t')); + ASSERT_FALSE(isASCIIAlpha(':')); +} + +TEST(string, isASCIIDigit) { + ASSERT_FALSE(isASCIIDigit('a')); + ASSERT_FALSE(isASCIIDigit('z')); + ASSERT_FALSE(isASCIIDigit('A')); + ASSERT_FALSE(isASCIIDigit('Z')); + ASSERT_TRUE(isASCIIDigit('0')); + ASSERT_TRUE(isASCIIDigit('1')); + ASSERT_TRUE(isASCIIDigit('9')); + ASSERT_FALSE(isASCIIDigit(' ')); + ASSERT_FALSE(isASCIIDigit('\n')); + ASSERT_FALSE(isASCIIDigit('\t')); + ASSERT_FALSE(isASCIIDigit(':')); +} + +} \ No newline at end of file From d3a85b68347071d8d93ec796a38c707483d7b272 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 6 Dec 2023 15:14:41 +0100 Subject: [PATCH 07/20] isValidSchemeName: Add function --- src/libutil/url.cc | 17 +++++++++++++++++ src/libutil/url.hh | 9 +++++++++ tests/unit/libutil/url.cc | 18 ++++++++++++++++++ 3 files changed, 44 insertions(+) diff --git a/src/libutil/url.cc b/src/libutil/url.cc index 57b64d607..f2d5f1782 100644 --- a/src/libutil/url.cc +++ b/src/libutil/url.cc @@ -3,6 +3,7 @@ #include "util.hh" #include "split.hh" #include "canon-path.hh" +#include "string.hh" namespace nix { @@ -183,4 +184,20 @@ std::string fixGitURL(const std::string & url) } } +// https://www.rfc-editor.org/rfc/rfc3986#section-3.1 +bool isValidSchemeName(std::string_view s) +{ + if (s.empty()) return false; + if (!isASCIIAlpha(s[0])) return false; + for (auto c : s.substr(1)) { + if (isASCIIAlpha(c)) continue; + if (isASCIIDigit(c)) continue; + if (c == '+') continue; + if (c == '-') continue; + if (c == '.') continue; + return false; + } + return true; +} + } diff --git a/src/libutil/url.hh b/src/libutil/url.hh index 833f54678..24806bbff 100644 --- a/src/libutil/url.hh +++ b/src/libutil/url.hh @@ -55,4 +55,13 @@ ParsedUrlScheme parseUrlScheme(std::string_view scheme); changes absolute paths into file:// URLs. */ std::string fixGitURL(const std::string & url); +/** + * Whether a string is valid as RFC 3986 scheme name. + * Colon `:` is part of the URI; not the scheme name, and therefore rejected. + * See https://www.rfc-editor.org/rfc/rfc3986#section-3.1 + * + * Does not check whether the scheme is understood, as that's context-dependent. + */ +bool isValidSchemeName(std::string_view scheme); + } diff --git a/tests/unit/libutil/url.cc b/tests/unit/libutil/url.cc index a678dad20..09fa4e218 100644 --- a/tests/unit/libutil/url.cc +++ b/tests/unit/libutil/url.cc @@ -344,4 +344,22 @@ namespace nix { ASSERT_EQ(percentDecode(e), s); } +TEST(nix, isValidSchemeName) { + ASSERT_TRUE(isValidSchemeName("http")); + ASSERT_TRUE(isValidSchemeName("https")); + ASSERT_TRUE(isValidSchemeName("file")); + ASSERT_TRUE(isValidSchemeName("file+https")); + ASSERT_TRUE(isValidSchemeName("fi.le")); + ASSERT_TRUE(isValidSchemeName("file-ssh")); + ASSERT_TRUE(isValidSchemeName("file+")); + ASSERT_TRUE(isValidSchemeName("file.")); + ASSERT_TRUE(isValidSchemeName("file1")); + ASSERT_FALSE(isValidSchemeName("file:")); + ASSERT_FALSE(isValidSchemeName("file/")); + ASSERT_FALSE(isValidSchemeName("+file")); + ASSERT_FALSE(isValidSchemeName(".file")); + ASSERT_FALSE(isValidSchemeName("-file")); + ASSERT_FALSE(isValidSchemeName("1file")); +} + } From a05bc9eb92371af631fc9fb83c3595957fb56943 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 6 Dec 2023 15:27:29 +0100 Subject: [PATCH 08/20] allowed-uris: Match whole schemes also when scheme is not followed by slashes --- ...llowed-uris-can-now-match-whole-schemes.md | 7 ++++ src/libexpr/eval-settings.hh | 5 +++ src/libexpr/eval.cc | 17 ++++++++- tests/unit/libexpr/eval.cc | 35 +++++++++++++++++++ 4 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 doc/manual/rl-next/allowed-uris-can-now-match-whole-schemes.md diff --git a/doc/manual/rl-next/allowed-uris-can-now-match-whole-schemes.md b/doc/manual/rl-next/allowed-uris-can-now-match-whole-schemes.md new file mode 100644 index 000000000..3cf75a612 --- /dev/null +++ b/doc/manual/rl-next/allowed-uris-can-now-match-whole-schemes.md @@ -0,0 +1,7 @@ +--- +synopsis: Option `allowed-uris` can now match whole schemes in URIs without slashes +prs: 9547 +--- + +If a scheme, such as `github:` is specified in the `allowed-uris` option, all URIs starting with `github:` are allowed. +Previously this only worked for schemes whose URIs used the `://` syntax. diff --git a/src/libexpr/eval-settings.hh b/src/libexpr/eval-settings.hh index db2971acb..3009a462c 100644 --- a/src/libexpr/eval-settings.hh +++ b/src/libexpr/eval-settings.hh @@ -68,6 +68,11 @@ struct EvalSettings : Config evaluation mode. For example, when set to `https://github.com/NixOS`, builtin functions such as `fetchGit` are allowed to access `https://github.com/NixOS/patchelf.git`. + + Access is granted when + - the URI is equal to the prefix, + - or the URI is a subpath of the prefix, + - or the prefix is a URI scheme ended by a colon `:` and the URI has the same scheme. )"}; Setting traceFunctionCalls{this, false, "trace-function-calls", diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 9e541f293..1552e3e92 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -18,6 +18,7 @@ #include "memory-input-accessor.hh" #include "signals.hh" #include "gc-small-vector.hh" +#include "url.hh" #include #include @@ -599,6 +600,14 @@ void EvalState::allowAndSetStorePathString(const StorePath & storePath, Value & mkStorePathString(storePath, v); } +inline static bool isJustSchemePrefix(std::string_view prefix) +{ + return + !prefix.empty() + && prefix[prefix.size() - 1] == ':' + && isValidSchemeName(prefix.substr(0, prefix.size() - 1)); +} + bool isAllowedURI(std::string_view uri, const Strings & allowedUris) { /* 'uri' should be equal to a prefix, or in a subdirectory of a @@ -611,8 +620,14 @@ bool isAllowedURI(std::string_view uri, const Strings & allowedUris) && prefix.size() > 0 && hasPrefix(uri, prefix) && ( + // Allow access to subdirectories of the prefix. prefix[prefix.size() - 1] == '/' - || uri[prefix.size()] == '/'))) + || uri[prefix.size()] == '/' + + // Allow access to whole schemes + || isJustSchemePrefix(prefix) + ) + )) return true; } diff --git a/tests/unit/libexpr/eval.cc b/tests/unit/libexpr/eval.cc index cc5d6bbfa..93d3f658f 100644 --- a/tests/unit/libexpr/eval.cc +++ b/tests/unit/libexpr/eval.cc @@ -103,4 +103,39 @@ TEST(nix_isAllowedURI, file_url) { ASSERT_FALSE(isAllowedURI("file://", allowed)); } +TEST(nix_isAllowedURI, github_all) { + Strings allowed; + allowed.push_back("github:"); + ASSERT_TRUE(isAllowedURI("github:", allowed)); + ASSERT_TRUE(isAllowedURI("github:foo/bar", allowed)); + ASSERT_TRUE(isAllowedURI("github:foo/bar/feat-multi-bar", allowed)); + ASSERT_TRUE(isAllowedURI("github:foo/bar?ref=refs/heads/feat-multi-bar", allowed)); + ASSERT_TRUE(isAllowedURI("github://foo/bar", allowed)); + ASSERT_FALSE(isAllowedURI("https://github:443/foo/bar/archive/master.tar.gz", allowed)); + ASSERT_FALSE(isAllowedURI("file://github:foo/bar/archive/master.tar.gz", allowed)); + ASSERT_FALSE(isAllowedURI("file:///github:foo/bar/archive/master.tar.gz", allowed)); + ASSERT_FALSE(isAllowedURI("github", allowed)); +} + +TEST(nix_isAllowedURI, github_org) { + Strings allowed; + allowed.push_back("github:foo"); + ASSERT_FALSE(isAllowedURI("github:", allowed)); + ASSERT_TRUE(isAllowedURI("github:foo/bar", allowed)); + ASSERT_TRUE(isAllowedURI("github:foo/bar/feat-multi-bar", allowed)); + ASSERT_TRUE(isAllowedURI("github:foo/bar?ref=refs/heads/feat-multi-bar", allowed)); + ASSERT_FALSE(isAllowedURI("github://foo/bar", allowed)); + ASSERT_FALSE(isAllowedURI("https://github:443/foo/bar/archive/master.tar.gz", allowed)); + ASSERT_FALSE(isAllowedURI("file://github:foo/bar/archive/master.tar.gz", allowed)); + ASSERT_FALSE(isAllowedURI("file:///github:foo/bar/archive/master.tar.gz", allowed)); +} + +TEST(nix_isAllowedURI, non_scheme_colon) { + Strings allowed; + allowed.push_back("https://foo/bar:"); + ASSERT_TRUE(isAllowedURI("https://foo/bar:", allowed)); + ASSERT_TRUE(isAllowedURI("https://foo/bar:/baz", allowed)); + ASSERT_FALSE(isAllowedURI("https://foo/bar:baz", allowed)); +} + } // namespace nix \ No newline at end of file From f45d2ee2b7090560fc30a227d638684268af700d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Mon, 11 Dec 2023 16:02:09 +0100 Subject: [PATCH 09/20] Fix query parsing for path-like flakes --- src/libexpr/flake/flakeref.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/flake/flakeref.cc b/src/libexpr/flake/flakeref.cc index 16f45ace7..8b0eb7460 100644 --- a/src/libexpr/flake/flakeref.cc +++ b/src/libexpr/flake/flakeref.cc @@ -90,7 +90,7 @@ std::pair parsePathFlakeRefWithFragment( fragment = percentDecode(url.substr(fragmentStart+1)); } if (pathEnd != std::string::npos && fragmentStart != std::string::npos) { - query = decodeQuery(url.substr(pathEnd+1, fragmentStart)); + query = decodeQuery(url.substr(pathEnd+1, fragmentStart-pathEnd-1)); } if (baseDir) { From 994f1b5c0de44319992ef6b1b106cee3fa400dc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20M=C3=B6ller?= Date: Mon, 11 Dec 2023 16:05:34 +0100 Subject: [PATCH 10/20] Add test cases for flake urls with fragments --- tests/functional/flakes/flakes.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/functional/flakes/flakes.sh b/tests/functional/flakes/flakes.sh index ccf1699f9..7506b6b3b 100644 --- a/tests/functional/flakes/flakes.sh +++ b/tests/functional/flakes/flakes.sh @@ -193,6 +193,14 @@ nix build -o "$TEST_ROOT/result" flake1 nix build -o "$TEST_ROOT/result" "$flake1Dir" nix build -o "$TEST_ROOT/result" "git+file://$flake1Dir" +# Test explicit packages.default. +nix build -o "$TEST_ROOT/result" "$flake1Dir#default" +nix build -o "$TEST_ROOT/result" "git+file://$flake1Dir#default" + +# Test explicit packages.default with query. +nix build -o "$TEST_ROOT/result" "$flake1Dir?ref=HEAD#default" +nix build -o "$TEST_ROOT/result" "git+file://$flake1Dir?ref=HEAD#default" + # Check that store symlinks inside a flake are not interpreted as flakes. nix build -o "$flake1Dir/result" "git+file://$flake1Dir" nix path-info "$flake1Dir/result" From 9f39dda66ce0f92707d4be05d0a90961c78f8bd4 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Sun, 10 Dec 2023 21:21:21 -0500 Subject: [PATCH 11/20] Fix building CA derivations with and eval store I don't love the way this code looks. There are two larger problems: - eval, build/scratch, destination stores (#5025) should have different types to reflect the fact that they are used for different purposes and those purposes correspond to different operations. It should be impossible to "use the wrong store" in my cases. - Since drvs can end up in both the eval and build/scratch store, we should have some sort of union/layered store (not on the file sytem level, just conceptual level) that allows accessing both. This would get rid of the ugly "check both" boilerplate in this PR. Still, it might be better to land this now / soon after minimal cleanup, so we have a concrete idea of what problem better abstractions are supposed to solve. --- src/libstore/build/derivation-goal.cc | 51 +++++++++++++++++++++------ src/libstore/misc.cc | 9 +++-- src/libstore/store-api.hh | 3 +- src/nix-build/nix-build.cc | 6 ++-- tests/functional/ca/eval-store.sh | 10 ++++++ tests/functional/ca/local.mk | 1 + tests/functional/eval-store.sh | 16 +++++++-- 7 files changed, 76 insertions(+), 20 deletions(-) create mode 100644 tests/functional/ca/eval-store.sh diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index d4da374ba..f8728ed4a 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -196,10 +196,19 @@ void DerivationGoal::loadDerivation() things being garbage collected while we're busy. */ worker.evalStore.addTempRoot(drvPath); - assert(worker.evalStore.isValidPath(drvPath)); + /* Get the derivation. It is probably in the eval store, but it might be inthe main store: - /* Get the derivation. */ - drv = std::make_unique(worker.evalStore.readDerivation(drvPath)); + - Resolved derivation are resolved against main store realisations, and so must be stored there. + + - Dynamic derivations are built, and so are found in the main store. + */ + for (auto * drvStore : { &worker.evalStore, &worker.store }) { + if (drvStore->isValidPath(drvPath)) { + drv = std::make_unique(drvStore->readDerivation(drvPath)); + break; + } + } + assert(drv); haveDerivation(); } @@ -401,11 +410,15 @@ void DerivationGoal::gaveUpOnSubstitution() } /* Copy the input sources from the eval store to the build - store. */ + store. + + Note that some inputs might not be in the eval store because they + are (resolved) derivation outputs in a resolved derivation. */ if (&worker.evalStore != &worker.store) { RealisedPath::Set inputSrcs; for (auto & i : drv->inputSrcs) - inputSrcs.insert(i); + if (worker.evalStore.isValidPath(i)) + inputSrcs.insert(i); copyClosure(worker.evalStore, worker.store, inputSrcs); } @@ -453,7 +466,7 @@ void DerivationGoal::repairClosure() std::map outputsToDrv; for (auto & i : inputClosure) if (i.isDerivation()) { - auto depOutputs = worker.store.queryPartialDerivationOutputMap(i); + auto depOutputs = worker.store.queryPartialDerivationOutputMap(i, &worker.evalStore); for (auto & j : depOutputs) if (j.second) outputsToDrv.insert_or_assign(*j.second, i); @@ -604,7 +617,13 @@ void DerivationGoal::inputsRealised() return *outPath; } else { - auto outMap = worker.evalStore.queryDerivationOutputMap(depDrvPath); + auto outMap = [&]{ + for (auto * drvStore : { &worker.evalStore, &worker.store }) + if (drvStore->isValidPath(depDrvPath)) + return worker.store.queryDerivationOutputMap(depDrvPath, drvStore); + assert(false); + }(); + auto outMapPath = outMap.find(outputName); if (outMapPath == outMap.end()) { throw Error( @@ -1085,8 +1104,12 @@ void DerivationGoal::resolvedFinished() auto newRealisation = realisation; newRealisation.id = DrvOutput { initialOutput->outputHash, outputName }; newRealisation.signatures.clear(); - if (!drv->type().isFixed()) - newRealisation.dependentRealisations = drvOutputReferences(worker.store, *drv, realisation.outPath); + if (!drv->type().isFixed()) { + auto & drvStore = worker.evalStore.isValidPath(drvPath) + ? worker.evalStore + : worker.store; + newRealisation.dependentRealisations = drvOutputReferences(worker.store, *drv, realisation.outPath, &drvStore); + } signRealisation(newRealisation); worker.store.registerDrvOutput(newRealisation); } @@ -1379,7 +1402,10 @@ std::map> DerivationGoal::queryPartialDeri res.insert_or_assign(name, output.path(worker.store, drv->name, name)); return res; } else { - return worker.store.queryPartialDerivationOutputMap(drvPath); + for (auto * drvStore : { &worker.evalStore, &worker.store }) + if (drvStore->isValidPath(drvPath)) + return worker.store.queryPartialDerivationOutputMap(drvPath, drvStore); + assert(false); } } @@ -1392,7 +1418,10 @@ OutputPathMap DerivationGoal::queryDerivationOutputMap() res.insert_or_assign(name, *output.second); return res; } else { - return worker.store.queryDerivationOutputMap(drvPath); + for (auto * drvStore : { &worker.evalStore, &worker.store }) + if (drvStore->isValidPath(drvPath)) + return worker.store.queryDerivationOutputMap(drvPath, drvStore); + assert(false); } } diff --git a/src/libstore/misc.cc b/src/libstore/misc.cc index 9f63fbbb5..cc8ad3d02 100644 --- a/src/libstore/misc.cc +++ b/src/libstore/misc.cc @@ -331,8 +331,11 @@ std::map drvOutputReferences( std::map drvOutputReferences( Store & store, const Derivation & drv, - const StorePath & outputPath) + const StorePath & outputPath, + Store * evalStore_) { + auto & evalStore = evalStore_ ? *evalStore_ : store; + std::set inputRealisations; std::function::ChildNode &)> accumRealisations; @@ -340,7 +343,7 @@ std::map drvOutputReferences( accumRealisations = [&](const StorePath & inputDrv, const DerivedPathMap::ChildNode & inputNode) { if (!inputNode.value.empty()) { auto outputHashes = - staticOutputHashes(store, store.readDerivation(inputDrv)); + staticOutputHashes(evalStore, evalStore.readDerivation(inputDrv)); for (const auto & outputName : inputNode.value) { auto outputHash = get(outputHashes, outputName); if (!outputHash) @@ -362,7 +365,7 @@ std::map drvOutputReferences( SingleDerivedPath next = SingleDerivedPath::Built { d, outputName }; accumRealisations( // TODO deep resolutions for dynamic derivations, issue #8947, would go here. - resolveDerivedPath(store, next), + resolveDerivedPath(store, next, evalStore_), childNode); } } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 13e5a1446..2c883ce97 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -943,6 +943,7 @@ const ContentAddress * getDerivationCA(const BasicDerivation & drv); std::map drvOutputReferences( Store & store, const Derivation & drv, - const StorePath & outputPath); + const StorePath & outputPath, + Store * evalStore = nullptr); } diff --git a/src/nix-build/nix-build.cc b/src/nix-build/nix-build.cc index 01da028d8..8e9be14c1 100644 --- a/src/nix-build/nix-build.cc +++ b/src/nix-build/nix-build.cc @@ -462,7 +462,7 @@ static void main_nix_build(int argc, char * * argv) if (dryRun) return; if (shellDrv) { - auto shellDrvOutputs = store->queryPartialDerivationOutputMap(shellDrv.value()); + auto shellDrvOutputs = store->queryPartialDerivationOutputMap(shellDrv.value(), &*evalStore); shell = store->printStorePath(shellDrvOutputs.at("out").value()) + "/bin/bash"; } @@ -515,7 +515,7 @@ static void main_nix_build(int argc, char * * argv) std::function::ChildNode &)> accumInputClosure; accumInputClosure = [&](const StorePath & inputDrv, const DerivedPathMap::ChildNode & inputNode) { - auto outputs = evalStore->queryPartialDerivationOutputMap(inputDrv); + auto outputs = store->queryPartialDerivationOutputMap(inputDrv, &*evalStore); for (auto & i : inputNode.value) { auto o = outputs.at(i); store->computeFSClosure(*o, inputs); @@ -653,7 +653,7 @@ static void main_nix_build(int argc, char * * argv) if (counter) drvPrefix += fmt("-%d", counter + 1); - auto builtOutputs = evalStore->queryPartialDerivationOutputMap(drvPath); + auto builtOutputs = store->queryPartialDerivationOutputMap(drvPath, &*evalStore); auto maybeOutputPath = builtOutputs.at(outputName); assert(maybeOutputPath); diff --git a/tests/functional/ca/eval-store.sh b/tests/functional/ca/eval-store.sh new file mode 100644 index 000000000..9cc499606 --- /dev/null +++ b/tests/functional/ca/eval-store.sh @@ -0,0 +1,10 @@ +#!/usr/bin/env bash + +# Ensure that garbage collection works properly with ca derivations + +source common.sh + +export NIX_TESTS_CA_BY_DEFAULT=1 + +cd .. +source eval-store.sh diff --git a/tests/functional/ca/local.mk b/tests/functional/ca/local.mk index fd87b8d1f..4f86b268f 100644 --- a/tests/functional/ca/local.mk +++ b/tests/functional/ca/local.mk @@ -5,6 +5,7 @@ ca-tests := \ $(d)/concurrent-builds.sh \ $(d)/derivation-json.sh \ $(d)/duplicate-realisation-in-closure.sh \ + $(d)/eval-store.sh \ $(d)/gc.sh \ $(d)/import-derivation.sh \ $(d)/new-build-cmd.sh \ diff --git a/tests/functional/eval-store.sh b/tests/functional/eval-store.sh index 8fc859730..ec99fd953 100644 --- a/tests/functional/eval-store.sh +++ b/tests/functional/eval-store.sh @@ -11,7 +11,16 @@ rm -rf "$eval_store" nix build -f dependencies.nix --eval-store "$eval_store" -o "$TEST_ROOT/result" [[ -e $TEST_ROOT/result/foobar ]] -(! ls $NIX_STORE_DIR/*.drv) +if [[ ! -n "${NIX_TESTS_CA_BY_DEFAULT:-}" ]]; then + # Resolved CA derivations are written to store for building + # + # TODO when we something more systematic + # (https://github.com/NixOS/nix/issues/5025) that distinguishes + # between scratch storage for building and the final destination + # store, we'll be able to make this unconditional again -- resolved + # derivations should only appear in the scratch store. + (! ls $NIX_STORE_DIR/*.drv) +fi ls $eval_store/nix/store/*.drv clearStore @@ -26,5 +35,8 @@ rm -rf "$eval_store" nix-build dependencies.nix --eval-store "$eval_store" -o "$TEST_ROOT/result" [[ -e $TEST_ROOT/result/foobar ]] -(! ls $NIX_STORE_DIR/*.drv) +if [[ ! -n "${NIX_TESTS_CA_BY_DEFAULT:-}" ]]; then + # See above + (! ls $NIX_STORE_DIR/*.drv) +fi ls $eval_store/nix/store/*.drv From 589fb105f311af65230d374cbbddf7173c7ad103 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 12 Dec 2023 16:05:32 +0100 Subject: [PATCH 12/20] Fix the VM tests Work around https://github.com/NixOS/nixpkgs/issues/271146 until we can depend on a Nixpkgs version containing https://github.com/NixOS/nixpkgs/pull/271423 --- flake.nix | 4 ++++ tests/nixos/default.nix | 1 + 2 files changed, 5 insertions(+) diff --git a/flake.nix b/flake.nix index bbdfc38e9..ada52c05d 100644 --- a/flake.nix +++ b/flake.nix @@ -7,6 +7,10 @@ # Also, do not grab arbitrary further staging commits. This PR was # carefully made to be based on release-23.05 and just contain # rebuild-causing changes to packages that Nix actually uses. + # + # Once this is updated to something containing + # https://github.com/NixOS/nixpkgs/pull/271423, don't forget + # to remove the `nix.checkAllErrors = false;` line in the tests. inputs.nixpkgs.url = "github:NixOS/nixpkgs/staging-23.05"; inputs.nixpkgs-regression.url = "github:NixOS/nixpkgs/215d4d0fd80ca5163643b03a33fde804a29cc1e2"; inputs.lowdown-src = { url = "github:kristapsdz/lowdown"; flake = false; }; diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index 4459aa664..2645cac8e 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -10,6 +10,7 @@ let hostPkgs = nixpkgsFor.${system}.native; defaults = { nixpkgs.pkgs = nixpkgsFor.${system}.native; + nix.checkAllErrors = false; }; _module.args.nixpkgs = nixpkgs; }; From 2e451a663eff96b89360cfd3c0d5eaa60ca46181 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 12 Dec 2023 17:22:54 +0100 Subject: [PATCH 13/20] schemeRegex -> schemeNameRegex Scheme could be understood to include the typical `:` separator. --- src/libutil/url-parts.hh | 2 +- src/libutil/url.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libutil/url-parts.hh b/src/libutil/url-parts.hh index 5c5a30dc2..07bc8d0cd 100644 --- a/src/libutil/url-parts.hh +++ b/src/libutil/url-parts.hh @@ -8,7 +8,7 @@ namespace nix { // URI stuff. const static std::string pctEncoded = "(?:%[0-9a-fA-F][0-9a-fA-F])"; -const static std::string schemeRegex = "(?:[a-z][a-z0-9+.-]*)"; +const static std::string schemeNameRegex = "(?:[a-z][a-z0-9+.-]*)"; const static std::string ipv6AddressSegmentRegex = "[0-9a-fA-F:]+(?:%\\w+)?"; const static std::string ipv6AddressRegex = "(?:\\[" + ipv6AddressSegmentRegex + "\\]|" + ipv6AddressSegmentRegex + ")"; const static std::string unreservedRegex = "(?:[a-zA-Z0-9-._~])"; diff --git a/src/libutil/url.cc b/src/libutil/url.cc index f2d5f1782..e9acd67d0 100644 --- a/src/libutil/url.cc +++ b/src/libutil/url.cc @@ -14,7 +14,7 @@ std::regex revRegex(revRegexS, std::regex::ECMAScript); ParsedURL parseURL(const std::string & url) { static std::regex uriRegex( - "((" + schemeRegex + "):" + "((" + schemeNameRegex + "):" + "(?:(?://(" + authorityRegex + ")(" + absPathRegex + "))|(/?" + pathRegex + ")))" + "(?:\\?(" + queryRegex + "))?" + "(?:#(" + queryRegex + "))?", From 4eaeda6604e2f8977728f14415fe92350d047970 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 12 Dec 2023 17:43:54 +0100 Subject: [PATCH 14/20] isValidSchemeName: Use regex As requested by Eelco Dolstra. I think it used to be simpler. --- src/libutil/url.cc | 15 +++------------ tests/unit/libutil/url.cc | 5 +++++ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/libutil/url.cc b/src/libutil/url.cc index e9acd67d0..152c06d8e 100644 --- a/src/libutil/url.cc +++ b/src/libutil/url.cc @@ -3,7 +3,6 @@ #include "util.hh" #include "split.hh" #include "canon-path.hh" -#include "string.hh" namespace nix { @@ -187,17 +186,9 @@ std::string fixGitURL(const std::string & url) // https://www.rfc-editor.org/rfc/rfc3986#section-3.1 bool isValidSchemeName(std::string_view s) { - if (s.empty()) return false; - if (!isASCIIAlpha(s[0])) return false; - for (auto c : s.substr(1)) { - if (isASCIIAlpha(c)) continue; - if (isASCIIDigit(c)) continue; - if (c == '+') continue; - if (c == '-') continue; - if (c == '.') continue; - return false; - } - return true; + static std::regex regex(schemeNameRegex, std::regex::ECMAScript); + + return std::regex_match(s.begin(), s.end(), regex, std::regex_constants::match_default); } } diff --git a/tests/unit/libutil/url.cc b/tests/unit/libutil/url.cc index 09fa4e218..7d08f467e 100644 --- a/tests/unit/libutil/url.cc +++ b/tests/unit/libutil/url.cc @@ -360,6 +360,11 @@ TEST(nix, isValidSchemeName) { ASSERT_FALSE(isValidSchemeName(".file")); ASSERT_FALSE(isValidSchemeName("-file")); ASSERT_FALSE(isValidSchemeName("1file")); + // regex ok? + ASSERT_FALSE(isValidSchemeName("\nhttp")); + ASSERT_FALSE(isValidSchemeName("\nhttp\n")); + ASSERT_FALSE(isValidSchemeName("http\n")); + ASSERT_FALSE(isValidSchemeName("http ")); } } From 0b87ba50c08d83384e11a8e6db1e2f97fba4b61c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 12 Dec 2023 17:44:53 +0100 Subject: [PATCH 15/20] Revert "Add nix::isASCII*, locale-independent" This reverts commit 79eb2920bb51c7ec9528a403986e79f04738e2be. Not used at this time. --- src/libutil/string.hh | 17 ----------- tests/unit/libutil/string.cc | 59 ------------------------------------ 2 files changed, 76 deletions(-) delete mode 100644 src/libutil/string.hh delete mode 100644 tests/unit/libutil/string.cc diff --git a/src/libutil/string.hh b/src/libutil/string.hh deleted file mode 100644 index 16ef75643..000000000 --- a/src/libutil/string.hh +++ /dev/null @@ -1,17 +0,0 @@ -#pragma once - -namespace nix { - - /** Locale-independent version of std::islower(). */ - inline bool isASCIILower(char c) { return c >= 'a' && c <= 'z'; }; - - /** Locale-independent version of std::isupper(). */ - inline bool isASCIIUpper(char c) { return c >= 'A' && c <= 'Z'; }; - - /** Locale-independent version of std::isalpha(). */ - inline bool isASCIIAlpha(char c) { return isASCIILower(c) || isASCIIUpper(c); }; - - /** Locale-independent version of std::isdigit(). */ - inline bool isASCIIDigit(char c) { return c >= '0' && c <= '9'; }; - -} diff --git a/tests/unit/libutil/string.cc b/tests/unit/libutil/string.cc deleted file mode 100644 index 381f2cc15..000000000 --- a/tests/unit/libutil/string.cc +++ /dev/null @@ -1,59 +0,0 @@ -#include -#include "string.hh" - -namespace nix { - -TEST(string, isASCIILower) { - ASSERT_TRUE(isASCIILower('a')); - ASSERT_TRUE(isASCIILower('z')); - ASSERT_FALSE(isASCIILower('A')); - ASSERT_FALSE(isASCIILower('Z')); - ASSERT_FALSE(isASCIILower('0')); - ASSERT_FALSE(isASCIILower('9')); - ASSERT_FALSE(isASCIILower(' ')); - ASSERT_FALSE(isASCIILower('\n')); - ASSERT_FALSE(isASCIILower('\t')); - ASSERT_FALSE(isASCIILower(':')); -} - -TEST(string, isASCIIUpper) { - ASSERT_FALSE(isASCIIUpper('a')); - ASSERT_FALSE(isASCIIUpper('z')); - ASSERT_TRUE(isASCIIUpper('A')); - ASSERT_TRUE(isASCIIUpper('Z')); - ASSERT_FALSE(isASCIIUpper('0')); - ASSERT_FALSE(isASCIIUpper('9')); - ASSERT_FALSE(isASCIIUpper(' ')); - ASSERT_FALSE(isASCIIUpper('\n')); - ASSERT_FALSE(isASCIIUpper('\t')); - ASSERT_FALSE(isASCIIUpper(':')); -} - -TEST(string, isASCIIAlpha) { - ASSERT_TRUE(isASCIIAlpha('a')); - ASSERT_TRUE(isASCIIAlpha('z')); - ASSERT_TRUE(isASCIIAlpha('A')); - ASSERT_TRUE(isASCIIAlpha('Z')); - ASSERT_FALSE(isASCIIAlpha('0')); - ASSERT_FALSE(isASCIIAlpha('9')); - ASSERT_FALSE(isASCIIAlpha(' ')); - ASSERT_FALSE(isASCIIAlpha('\n')); - ASSERT_FALSE(isASCIIAlpha('\t')); - ASSERT_FALSE(isASCIIAlpha(':')); -} - -TEST(string, isASCIIDigit) { - ASSERT_FALSE(isASCIIDigit('a')); - ASSERT_FALSE(isASCIIDigit('z')); - ASSERT_FALSE(isASCIIDigit('A')); - ASSERT_FALSE(isASCIIDigit('Z')); - ASSERT_TRUE(isASCIIDigit('0')); - ASSERT_TRUE(isASCIIDigit('1')); - ASSERT_TRUE(isASCIIDigit('9')); - ASSERT_FALSE(isASCIIDigit(' ')); - ASSERT_FALSE(isASCIIDigit('\n')); - ASSERT_FALSE(isASCIIDigit('\t')); - ASSERT_FALSE(isASCIIDigit(':')); -} - -} \ No newline at end of file From 04f454f2a0e1bfb4fc0368872f215cb690df11bc Mon Sep 17 00:00:00 2001 From: SharzyL Date: Wed, 13 Dec 2023 10:30:28 +0800 Subject: [PATCH 16/20] fix: nix copy ssh-ng:// not respecting --substitute-on-destination --- src/libstore/remote-store.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index cc26c2a94..dd6347468 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -225,7 +225,7 @@ StorePathSet RemoteStore::queryValidPaths(const StorePathSet & paths, Substitute conn->to << WorkerProto::Op::QueryValidPaths; WorkerProto::write(*this, *conn, paths); if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 27) { - conn->to << (settings.buildersUseSubstitutes ? 1 : 0); + conn->to << maybeSubstitute; } conn.processStderr(); return WorkerProto::Serialise::read(*this, *conn); From cc3913e4584beb19e7af00572db119d2638333d5 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 13 Dec 2023 13:27:23 +0100 Subject: [PATCH 17/20] Remove unused variable --- src/libexpr/eval.hh | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index f3f6d35b9..f452dcb9f 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -335,11 +335,6 @@ private: std::map> searchPathResolved; - /** - * Cache used by checkSourcePath(). - */ - std::unordered_map resolvedPaths; - /** * Cache used by prim_match(). */ From 103ca0bde5d4f32745d4c3aee534cf4aa0a69a9d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 13 Dec 2023 13:27:29 +0100 Subject: [PATCH 18/20] Improve SourcePath display --- src/libfetchers/input-accessor.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libfetchers/input-accessor.hh b/src/libfetchers/input-accessor.hh index d5ac238b1..f385e6231 100644 --- a/src/libfetchers/input-accessor.hh +++ b/src/libfetchers/input-accessor.hh @@ -130,7 +130,7 @@ struct SourcePath { return accessor->getPhysicalPath(path); } std::string to_string() const - { return path.abs(); } + { return accessor->showPath(path); } /** * Append a `CanonPath` to this path. From faa4cae9aed4e9f8c40ed8c6fe00bd0216c3b0ea Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 13 Dec 2023 13:27:39 +0100 Subject: [PATCH 19/20] LibExprTest: Ignore $NIX_PATH Otherwise a broken $NIX_PATH can cause the test suite to fail. --- tests/unit/libexpr-support/tests/libexpr.hh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/unit/libexpr-support/tests/libexpr.hh b/tests/unit/libexpr-support/tests/libexpr.hh index 968431446..d720cedde 100644 --- a/tests/unit/libexpr-support/tests/libexpr.hh +++ b/tests/unit/libexpr-support/tests/libexpr.hh @@ -8,6 +8,7 @@ #include "nixexpr.hh" #include "eval.hh" #include "eval-inline.hh" +#include "eval-settings.hh" #include "store-api.hh" #include "tests/libstore.hh" @@ -18,6 +19,7 @@ namespace nix { static void SetUpTestSuite() { LibStoreTest::SetUpTestSuite(); initGC(); + evalSettings.nixPath = {}; } protected: From 19ec1c9fd4d4bf6e941b046b8549ba2a1a690937 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 13 Dec 2023 15:15:30 +0100 Subject: [PATCH 20/20] Improve the unsafeGetAttrPos test We can use corepkgsFS->addFile() now to create a "real" position. --- tests/unit/libexpr/primops.cc | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/unit/libexpr/primops.cc b/tests/unit/libexpr/primops.cc index 7485fa0d0..384d9924b 100644 --- a/tests/unit/libexpr/primops.cc +++ b/tests/unit/libexpr/primops.cc @@ -1,6 +1,8 @@ #include #include +#include "memory-input-accessor.hh" + #include "tests/libexpr.hh" namespace nix { @@ -148,10 +150,25 @@ namespace nix { } TEST_F(PrimOpTest, unsafeGetAttrPos) { - // The `y` attribute is at position - const char* expr = "builtins.unsafeGetAttrPos \"y\" { y = \"x\"; }"; + state.corepkgsFS->addFile(CanonPath("foo.nix"), "{ y = \"x\"; }"); + + auto expr = "builtins.unsafeGetAttrPos \"y\" (import )"; auto v = eval(expr); - ASSERT_THAT(v, IsNull()); + ASSERT_THAT(v, IsAttrsOfSize(3)); + + auto file = v.attrs->find(createSymbol("file")); + ASSERT_NE(file, nullptr); + ASSERT_THAT(*file->value, IsString()); + auto s = baseNameOf(file->value->string_view()); + ASSERT_EQ(s, "foo.nix"); + + auto line = v.attrs->find(createSymbol("line")); + ASSERT_NE(line, nullptr); + ASSERT_THAT(*line->value, IsIntEq(1)); + + auto column = v.attrs->find(createSymbol("column")); + ASSERT_NE(column, nullptr); + ASSERT_THAT(*column->value, IsIntEq(3)); } TEST_F(PrimOpTest, hasAttr) {