diff options
| -rw-r--r-- | doc/manual/rl-next/leading-period.md | 10 | ||||
| -rw-r--r-- | src/libstore/path-regex.hh | 7 | ||||
| -rw-r--r-- | src/libstore/path.cc | 15 | ||||
| -rw-r--r-- | tests/unit/libstore-support/tests/path.cc | 78 | ||||
| -rw-r--r-- | tests/unit/libstore/path.cc | 70 | ||||
| -rw-r--r-- | tests/unit/libutil-support/tests/hash.cc | 15 |
6 files changed, 146 insertions, 49 deletions
diff --git a/doc/manual/rl-next/leading-period.md b/doc/manual/rl-next/leading-period.md new file mode 100644 index 000000000..7a2fd1f67 --- /dev/null +++ b/doc/manual/rl-next/leading-period.md @@ -0,0 +1,10 @@ +--- +synopsis: Store paths are allowed to start with `.` +issues: 912 +prs: [9867, 9091, 9095, 9120, 9121, 9122, 9130, 9219, 9224] +--- + +Leading periods were allowed by accident in Nix 2.4. The Nix team has considered this to be a bug, but this behavior has since been relied on by users, leading to unnecessary difficulties. +From now on, leading periods are officially, definitively supported. The names `.` and `..` are disallowed, as well as those starting with `.-` or `..-`. + +Nix versions that denied leading periods are documented [in the issue](https://github.com/NixOS/nix/issues/912#issuecomment-1919583286). diff --git a/src/libstore/path-regex.hh b/src/libstore/path-regex.hh index a44e6a2eb..56c2cfc1d 100644 --- a/src/libstore/path-regex.hh +++ b/src/libstore/path-regex.hh @@ -3,6 +3,11 @@ namespace nix { -static constexpr std::string_view nameRegexStr = R"([0-9a-zA-Z\+\-_\?=][0-9a-zA-Z\+\-\._\?=]*)"; + +static constexpr std::string_view nameRegexStr = + // This uses a negative lookahead: (?!\.\.?(-|$)) + // - deny ".", "..", or those strings followed by '-' + // - when it's not those, start again at the start of the input and apply the next regex, which is [0-9a-zA-Z\+\-\._\?=]+ + R"((?!\.\.?(-|$))[0-9a-zA-Z\+\-\._\?=]+)"; } diff --git a/src/libstore/path.cc b/src/libstore/path.cc index 2f929b7b3..d029e986b 100644 --- a/src/libstore/path.cc +++ b/src/libstore/path.cc @@ -11,9 +11,20 @@ static void checkName(std::string_view path, std::string_view name) if (name.size() > StorePath::MaxPathLen) throw BadStorePath("store path '%s' has a name longer than %d characters", path, StorePath::MaxPathLen); - if (name[0] == '.') - throw BadStorePath("store path '%s' starts with illegal character '.'", path); // See nameRegexStr for the definition + if (name[0] == '.') { + // check against "." and "..", followed by end or dash + if (name.size() == 1) + throw BadStorePath("store path '%s' has invalid name '%s'", path, name); + if (name[1] == '-') + throw BadStorePath("store path '%s' has invalid name '%s': first dash-separated component must not be '%s'", path, name, "."); + if (name[1] == '.') { + if (name.size() == 2) + throw BadStorePath("store path '%s' has invalid name '%s'", path, name); + if (name[2] == '-') + throw BadStorePath("store path '%s' has invalid name '%s': first dash-separated component must not be '%s'", path, name, ".."); + } + } for (auto c : name) if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'z') diff --git a/tests/unit/libstore-support/tests/path.cc b/tests/unit/libstore-support/tests/path.cc index ffc4fc607..8ddda8027 100644 --- a/tests/unit/libstore-support/tests/path.cc +++ b/tests/unit/libstore-support/tests/path.cc @@ -1,3 +1,4 @@ +#include <rapidcheck/gen/Arbitrary.h> #include <regex> #include <rapidcheck.h> @@ -20,65 +21,60 @@ void showValue(const StorePath & p, std::ostream & os) namespace rc { using namespace nix; -Gen<StorePathName> Arbitrary<StorePathName>::arbitrary() +Gen<char> storePathChar() { - auto len = *gen::inRange<size_t>( - 1, - StorePath::MaxPathLen - StorePath::HashLen); - - std::string pre; - pre.reserve(len); - - for (size_t c = 0; c < len; ++c) { - switch (auto i = *gen::inRange<uint8_t>(0, 10 + 2 * 26 + 6)) { + return rc::gen::apply([](uint8_t i) -> char { + switch (i) { case 0 ... 9: - pre += static_cast<uint8_t>('0' + i); - break; + return '0' + i; case 10 ... 35: - pre += static_cast<uint8_t>('A' + (i - 10)); - break; + return 'A' + (i - 10); case 36 ... 61: - pre += static_cast<uint8_t>('a' + (i - 36)); - break; + return 'a' + (i - 36); case 62: - pre += '+'; - break; + return '+'; case 63: - pre += '-'; - break; + return '-'; case 64: - // names aren't permitted to start with a period, - // so just fall through to the next case here - if (c != 0) { - pre += '.'; - break; - } - [[fallthrough]]; + return '.'; case 65: - pre += '_'; - break; + return '_'; case 66: - pre += '?'; - break; + return '?'; case 67: - pre += '='; - break; + return '='; default: assert(false); } - } + }, + gen::inRange<uint8_t>(0, 10 + 2 * 26 + 6)); +} - return gen::just(StorePathName { - .name = std::move(pre), - }); +Gen<StorePathName> Arbitrary<StorePathName>::arbitrary() +{ + return gen::construct<StorePathName>( + gen::suchThat( + gen::container<std::string>(storePathChar()), + [](const std::string & s) { + return + !( s == "" + || s == "." + || s == ".." + || s.starts_with(".-") + || s.starts_with("..-") + ); + } + ) + ); } Gen<StorePath> Arbitrary<StorePath>::arbitrary() { - return gen::just(StorePath { - *gen::arbitrary<Hash>(), - (*gen::arbitrary<StorePathName>()).name, - }); + return + gen::construct<StorePath>( + gen::arbitrary<Hash>(), + gen::apply([](StorePathName n){ return n.name; }, gen::arbitrary<StorePathName>()) + ); } } // namespace rc diff --git a/tests/unit/libstore/path.cc b/tests/unit/libstore/path.cc index 30631b5fd..213b6e95f 100644 --- a/tests/unit/libstore/path.cc +++ b/tests/unit/libstore/path.cc @@ -39,7 +39,12 @@ TEST_DONT_PARSE(double_star, "**") TEST_DONT_PARSE(star_first, "*,foo") TEST_DONT_PARSE(star_second, "foo,*") TEST_DONT_PARSE(bang, "foo!o") -TEST_DONT_PARSE(dotfile, ".gitignore") +TEST_DONT_PARSE(dot, ".") +TEST_DONT_PARSE(dot_dot, "..") +TEST_DONT_PARSE(dot_dot_dash, "..-1") +TEST_DONT_PARSE(dot_dash, ".-1") +TEST_DONT_PARSE(dot_dot_dash_a, "..-a") +TEST_DONT_PARSE(dot_dash_a, ".-a") #undef TEST_DONT_PARSE @@ -63,6 +68,11 @@ TEST_DO_PARSE(underscore, "foo_bar") TEST_DO_PARSE(period, "foo.txt") TEST_DO_PARSE(question_mark, "foo?why") TEST_DO_PARSE(equals_sign, "foo=foo") +TEST_DO_PARSE(dotfile, ".gitignore") +TEST_DO_PARSE(triple_dot_a, "...a") +TEST_DO_PARSE(triple_dot_1, "...1") +TEST_DO_PARSE(triple_dot_dash, "...-") +TEST_DO_PARSE(triple_dot, "...") #undef TEST_DO_PARSE @@ -84,6 +94,64 @@ RC_GTEST_FIXTURE_PROP( RC_ASSERT(p == store->parseStorePath(store->printStorePath(p))); } + +RC_GTEST_FIXTURE_PROP( + StorePathTest, + prop_check_regex_eq_parse, + ()) +{ + static auto nameFuzzer = + rc::gen::container<std::string>( + rc::gen::oneOf( + // alphanum, repeated to weigh heavier + rc::gen::oneOf( + rc::gen::inRange('0', '9'), + rc::gen::inRange('a', 'z'), + rc::gen::inRange('A', 'Z') + ), + // valid symbols + rc::gen::oneOf( + rc::gen::just('+'), + rc::gen::just('-'), + rc::gen::just('.'), + rc::gen::just('_'), + rc::gen::just('?'), + rc::gen::just('=') + ), + // symbols for scary .- and ..- cases, repeated for weight + rc::gen::just('.'), rc::gen::just('.'), + rc::gen::just('.'), rc::gen::just('.'), + rc::gen::just('-'), rc::gen::just('-'), + // ascii symbol ranges + rc::gen::oneOf( + rc::gen::inRange(' ', '/'), + rc::gen::inRange(':', '@'), + rc::gen::inRange('[', '`'), + rc::gen::inRange('{', '~') + ), + // typical whitespace + rc::gen::oneOf( + rc::gen::just(' '), + rc::gen::just('\t'), + rc::gen::just('\n'), + rc::gen::just('\r') + ), + // some chance of control codes, non-ascii or other garbage we missed + rc::gen::inRange('\0', '\xff') + )); + + auto name = *nameFuzzer; + + std::string path = store->storeDir + "/575s52sh487i0ylmbs9pvi606ljdszr0-" + name; + bool parsed = false; + try { + store->parseStorePath(path); + parsed = true; + } catch (const BadStorePath &) { + } + RC_ASSERT(parsed == std::regex_match(std::string { name }, nameRegex)); +} + #endif } diff --git a/tests/unit/libutil-support/tests/hash.cc b/tests/unit/libutil-support/tests/hash.cc index 577e9890e..7cc994b40 100644 --- a/tests/unit/libutil-support/tests/hash.cc +++ b/tests/unit/libutil-support/tests/hash.cc @@ -11,10 +11,17 @@ using namespace nix; Gen<Hash> Arbitrary<Hash>::arbitrary() { - Hash hash(htSHA1); - for (size_t i = 0; i < hash.hashSize; ++i) - hash.hash[i] = *gen::arbitrary<uint8_t>(); - return gen::just(hash); + Hash prototype(htSHA1); + return + gen::apply( + [](const std::vector<uint8_t> & v) { + Hash hash(htSHA1); + assert(v.size() == hash.hashSize); + std::copy(v.begin(), v.end(), hash.hash); + return hash; + }, + gen::container<std::vector<uint8_t>>(prototype.hashSize, gen::arbitrary<uint8_t>()) + ); } } |