From 64a3b045c12c3e0fa98f5967ceb38378eaf70e2d Mon Sep 17 00:00:00 2001 From: Alexey Novikov Date: Sun, 17 Oct 2021 11:51:33 +0400 Subject: [PATCH 1/2] Fix error detection in 'base64Decode()' Fixed a bug in initialization of 'base64DecodeChars' variable. Currently decoder do not fail on invalid Base64 strings. Added test-case to verify the fix. Also have made 'base64DecodeChars' to be computed at compile time. And added a test case to encode/decode string with non-printable charactes. --- src/libutil/tests/tests.cc | 17 +++++++++++++++++ src/libutil/util.cc | 18 ++++++++++-------- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/libutil/tests/tests.cc b/src/libutil/tests/tests.cc index 58df9c5ac..92972ed14 100644 --- a/src/libutil/tests/tests.cc +++ b/src/libutil/tests/tests.cc @@ -4,6 +4,8 @@ #include #include +#include + namespace nix { /* ----------- tests for util.hh ------------------------------------------------*/ @@ -282,6 +284,17 @@ namespace nix { ASSERT_EQ(decoded, s); } + TEST(base64Encode, encodeAndDecodeNonPrintable) { + char s[256]; + std::iota(std::rbegin(s), std::rend(s), 0); + + auto encoded = base64Encode(s); + auto decoded = base64Decode(encoded); + + EXPECT_EQ(decoded.length(), 255); + ASSERT_EQ(decoded, s); + } + /* ---------------------------------------------------------------------------- * base64Decode * --------------------------------------------------------------------------*/ @@ -294,6 +307,10 @@ namespace nix { ASSERT_EQ(base64Decode("cXVvZCBlcmF0IGRlbW9uc3RyYW5kdW0="), "quod erat demonstrandum"); } + TEST(base64Decode, decodeThrowsOnInvalidChar) { + ASSERT_THROW(base64Decode("cXVvZCBlcm_0IGRlbW9uc3RyYW5kdW0="), Error); + } + /* ---------------------------------------------------------------------------- * toLower * --------------------------------------------------------------------------*/ diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 563a72c12..a7b3b879a 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1436,8 +1436,7 @@ std::string filterANSIEscapes(const std::string & s, bool filterAll, unsigned in } -static char base64Chars[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; -static std::array base64DecodeChars; +constexpr char base64Chars[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; string base64Encode(std::string_view s) { @@ -1462,12 +1461,15 @@ string base64Encode(std::string_view s) string base64Decode(std::string_view s) { - static std::once_flag flag; - std::call_once(flag, [](){ - base64DecodeChars = { (char)-1 }; + constexpr char npos = -1; + constexpr std::array base64DecodeChars = [&]() { + std::array result{}; + for (auto& c : result) + c = npos; for (int i = 0; i < 64; i++) - base64DecodeChars[(int) base64Chars[i]] = i; - }); + result[base64Chars[i]] = i; + return result; + }(); string res; unsigned int d = 0, bits = 0; @@ -1477,7 +1479,7 @@ string base64Decode(std::string_view s) if (c == '\n') continue; char digit = base64DecodeChars[(unsigned char) c]; - if (digit == -1) + if (digit == npos) throw Error("invalid character in Base64 string: '%c'", c); bits += 6; From e989c83b44d7c4d8ffe2e5f4231e4861c5f7732f Mon Sep 17 00:00:00 2001 From: Alexey Novikov Date: Tue, 12 Oct 2021 16:18:44 +0400 Subject: [PATCH 2/2] Add error reporting to machine spec paser Currently machine specification (`/etc/nix/machine`) parser fails with a vague exception if the file had incorrect format. This commit adds verbose exceptions and unit-tests for the parser. --- src/libstore/build/entry-points.cc | 1 - src/libstore/machines.cc | 78 +++++--- src/libstore/tests/machines.cc | 169 ++++++++++++++++++ .../tests/test-data/machines.bad_format | 1 + src/libstore/tests/test-data/machines.valid | 3 + 5 files changed, 227 insertions(+), 25 deletions(-) create mode 100644 src/libstore/tests/machines.cc create mode 100644 src/libstore/tests/test-data/machines.bad_format create mode 100644 src/libstore/tests/test-data/machines.valid diff --git a/src/libstore/build/entry-points.cc b/src/libstore/build/entry-points.cc index 2b77e4354..b1e5d8504 100644 --- a/src/libstore/build/entry-points.cc +++ b/src/libstore/build/entry-points.cc @@ -1,4 +1,3 @@ -#include "machines.hh" #include "worker.hh" #include "substitution-goal.hh" #include "derivation-goal.hh" diff --git a/src/libstore/machines.cc b/src/libstore/machines.cc index 9843ccf04..910e32a76 100644 --- a/src/libstore/machines.cc +++ b/src/libstore/machines.cc @@ -83,53 +83,83 @@ ref Machine::openStore() const { return nix::openStore(storeUri, storeParams); } -void parseMachines(const std::string & s, Machines & machines) -{ - for (auto line : tokenizeString>(s, "\n;")) { +static std::vector expandBuilderLines(const std::string& builders) { + std::vector result; + for (auto line : tokenizeString>(builders, "\n;")) { trim(line); line.erase(std::find(line.begin(), line.end(), '#'), line.end()); if (line.empty()) continue; if (line[0] == '@') { - auto file = trim(std::string(line, 1)); + const std::string path = trim(std::string(line, 1)); + std::string text; try { - parseMachines(readFile(file), machines); + text = readFile(path); } catch (const SysError & e) { if (e.errNo != ENOENT) throw; - debug("cannot find machines file '%s'", file); + debug("cannot find machines file '%s'", path); } + + const auto lines = expandBuilderLines(text); + result.insert(end(result), begin(lines), end(lines)); continue; } - auto tokens = tokenizeString>(line); - auto sz = tokens.size(); - if (sz < 1) - throw FormatError("bad machine specification '%s'", line); + result.emplace_back(line); + } + return result; +} - auto isSet = [&](size_t n) { - return tokens.size() > n && tokens[n] != "" && tokens[n] != "-"; - }; +static Machine parseBuilderLine(const std::string& line) { + const auto tokens = tokenizeString>(line); - machines.emplace_back(tokens[0], + auto isSet = [&](size_t fieldIndex) { + return tokens.size() > fieldIndex && tokens[fieldIndex] != "" && tokens[fieldIndex] != "-"; + }; + + auto parseUnsignedIntField = [&](size_t fieldIndex) { + const auto result = string2Int(tokens[fieldIndex]); + if (!result) { + throw FormatError("bad machine specification: failed to convert column #%lu in a row: '%s' to 'unsigned int'", fieldIndex, line); + } + return result.value(); + }; + + auto ensureBase64 = [&](size_t fieldIndex) { + const auto& str = tokens[fieldIndex]; + try { + base64Decode(str); + } catch (const Error& e) { + throw FormatError("bad machine specification: a column #%lu in a row: '%s' is not valid base64 string: %s", fieldIndex, line, e.what()); + } + return str; + }; + + if (!isSet(0)) { + throw FormatError("bad machine specification: store URI was not found at the first column of a row: '%s'", line); + } + + return {tokens[0], isSet(1) ? tokenizeString>(tokens[1], ",") : std::vector{settings.thisSystem}, isSet(2) ? tokens[2] : "", - isSet(3) ? std::stoull(tokens[3]) : 1LL, - isSet(4) ? std::stoull(tokens[4]) : 1LL, + isSet(3) ? parseUnsignedIntField(3) : 1U, + isSet(4) ? parseUnsignedIntField(4) : 1U, isSet(5) ? tokenizeString>(tokens[5], ",") : std::set{}, isSet(6) ? tokenizeString>(tokens[6], ",") : std::set{}, - isSet(7) ? tokens[7] : ""); - } + isSet(7) ? ensureBase64(7) : ""}; +} + +static Machines parseBuilderLines(const std::vector& builders) { + Machines result; + std::transform(builders.begin(), builders.end(), std::back_inserter(result), parseBuilderLine); + return result; } Machines getMachines() { - static auto machines = [&]() { - Machines machines; - parseMachines(settings.builders, machines); - return machines; - }(); - return machines; + const auto builderLines = expandBuilderLines(settings.builders); + return parseBuilderLines(builderLines); } } diff --git a/src/libstore/tests/machines.cc b/src/libstore/tests/machines.cc new file mode 100644 index 000000000..f51052b14 --- /dev/null +++ b/src/libstore/tests/machines.cc @@ -0,0 +1,169 @@ +#include "machines.hh" +#include "globals.hh" + +#include + +using testing::Contains; +using testing::ElementsAre; +using testing::EndsWith; +using testing::Eq; +using testing::Field; +using testing::SizeIs; + +using nix::absPath; +using nix::FormatError; +using nix::getMachines; +using nix::Machine; +using nix::Machines; +using nix::pathExists; +using nix::Settings; +using nix::settings; + +class Environment : public ::testing::Environment { + public: + void SetUp() override { settings.thisSystem = "TEST_ARCH-TEST_OS"; } +}; + +testing::Environment* const foo_env = + testing::AddGlobalTestEnvironment(new Environment); + +TEST(machines, getMachinesWithEmptyBuilders) { + settings.builders = ""; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(0)); +} + +TEST(machines, getMachinesUriOnly) { + settings.builders = "nix@scratchy.labs.cs.uu.nl"; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(1)); + EXPECT_THAT(actual[0], Field(&Machine::storeUri, Eq("ssh://nix@scratchy.labs.cs.uu.nl"))); + EXPECT_THAT(actual[0], Field(&Machine::systemTypes, ElementsAre("TEST_ARCH-TEST_OS"))); + EXPECT_THAT(actual[0], Field(&Machine::sshKey, SizeIs(0))); + EXPECT_THAT(actual[0], Field(&Machine::maxJobs, Eq(1))); + EXPECT_THAT(actual[0], Field(&Machine::speedFactor, Eq(1))); + EXPECT_THAT(actual[0], Field(&Machine::supportedFeatures, SizeIs(0))); + EXPECT_THAT(actual[0], Field(&Machine::mandatoryFeatures, SizeIs(0))); + EXPECT_THAT(actual[0], Field(&Machine::sshPublicHostKey, SizeIs(0))); +} + +TEST(machines, getMachinesDefaults) { + settings.builders = "nix@scratchy.labs.cs.uu.nl - - - - - - -"; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(1)); + EXPECT_THAT(actual[0], Field(&Machine::storeUri, Eq("ssh://nix@scratchy.labs.cs.uu.nl"))); + EXPECT_THAT(actual[0], Field(&Machine::systemTypes, ElementsAre("TEST_ARCH-TEST_OS"))); + EXPECT_THAT(actual[0], Field(&Machine::sshKey, SizeIs(0))); + EXPECT_THAT(actual[0], Field(&Machine::maxJobs, Eq(1))); + EXPECT_THAT(actual[0], Field(&Machine::speedFactor, Eq(1))); + EXPECT_THAT(actual[0], Field(&Machine::supportedFeatures, SizeIs(0))); + EXPECT_THAT(actual[0], Field(&Machine::mandatoryFeatures, SizeIs(0))); + EXPECT_THAT(actual[0], Field(&Machine::sshPublicHostKey, SizeIs(0))); +} + +TEST(machines, getMachinesWithNewLineSeparator) { + settings.builders = "nix@scratchy.labs.cs.uu.nl\nnix@itchy.labs.cs.uu.nl"; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(2)); + EXPECT_THAT(actual, Contains(Field(&Machine::storeUri, EndsWith("nix@scratchy.labs.cs.uu.nl")))); + EXPECT_THAT(actual, Contains(Field(&Machine::storeUri, EndsWith("nix@itchy.labs.cs.uu.nl")))); +} + +TEST(machines, getMachinesWithSemicolonSeparator) { + settings.builders = "nix@scratchy.labs.cs.uu.nl ; nix@itchy.labs.cs.uu.nl"; + Machines actual = getMachines(); + EXPECT_THAT(actual, SizeIs(2)); + EXPECT_THAT(actual, Contains(Field(&Machine::storeUri, EndsWith("nix@scratchy.labs.cs.uu.nl")))); + EXPECT_THAT(actual, Contains(Field(&Machine::storeUri, EndsWith("nix@itchy.labs.cs.uu.nl")))); +} + +TEST(machines, getMachinesWithCorrectCompleteSingleBuilder) { + settings.builders = "nix@scratchy.labs.cs.uu.nl i686-linux " + "/home/nix/.ssh/id_scratchy_auto 8 3 kvm " + "benchmark SSH+HOST+PUBLIC+KEY+BASE64+ENCODED=="; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(1)); + EXPECT_THAT(actual[0], Field(&Machine::storeUri, EndsWith("nix@scratchy.labs.cs.uu.nl"))); + EXPECT_THAT(actual[0], Field(&Machine::systemTypes, ElementsAre("i686-linux"))); + EXPECT_THAT(actual[0], Field(&Machine::sshKey, Eq("/home/nix/.ssh/id_scratchy_auto"))); + EXPECT_THAT(actual[0], Field(&Machine::maxJobs, Eq(8))); + EXPECT_THAT(actual[0], Field(&Machine::speedFactor, Eq(3))); + EXPECT_THAT(actual[0], Field(&Machine::supportedFeatures, ElementsAre("kvm"))); + EXPECT_THAT(actual[0], Field(&Machine::mandatoryFeatures, ElementsAre("benchmark"))); + EXPECT_THAT(actual[0], Field(&Machine::sshPublicHostKey, Eq("SSH+HOST+PUBLIC+KEY+BASE64+ENCODED=="))); +} + +TEST(machines, + getMachinesWithCorrectCompleteSingleBuilderWithTabColumnDelimiter) { + settings.builders = + "nix@scratchy.labs.cs.uu.nl\ti686-linux\t/home/nix/.ssh/" + "id_scratchy_auto\t8\t3\tkvm\tbenchmark\tSSH+HOST+PUBLIC+" + "KEY+BASE64+ENCODED=="; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(1)); + EXPECT_THAT(actual[0], Field(&Machine::storeUri, EndsWith("nix@scratchy.labs.cs.uu.nl"))); + EXPECT_THAT(actual[0], Field(&Machine::systemTypes, ElementsAre("i686-linux"))); + EXPECT_THAT(actual[0], Field(&Machine::sshKey, Eq("/home/nix/.ssh/id_scratchy_auto"))); + EXPECT_THAT(actual[0], Field(&Machine::maxJobs, Eq(8))); + EXPECT_THAT(actual[0], Field(&Machine::speedFactor, Eq(3))); + EXPECT_THAT(actual[0], Field(&Machine::supportedFeatures, ElementsAre("kvm"))); + EXPECT_THAT(actual[0], Field(&Machine::mandatoryFeatures, ElementsAre("benchmark"))); + EXPECT_THAT(actual[0], Field(&Machine::sshPublicHostKey, Eq("SSH+HOST+PUBLIC+KEY+BASE64+ENCODED=="))); +} + +TEST(machines, getMachinesWithMultiOptions) { + settings.builders = "nix@scratchy.labs.cs.uu.nl Arch1,Arch2 - - - " + "SupportedFeature1,SupportedFeature2 " + "MandatoryFeature1,MandatoryFeature2"; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(1)); + EXPECT_THAT(actual[0], Field(&Machine::storeUri, EndsWith("nix@scratchy.labs.cs.uu.nl"))); + EXPECT_THAT(actual[0], Field(&Machine::systemTypes, ElementsAre("Arch1", "Arch2"))); + EXPECT_THAT(actual[0], Field(&Machine::supportedFeatures, ElementsAre("SupportedFeature1", "SupportedFeature2"))); + EXPECT_THAT(actual[0], Field(&Machine::mandatoryFeatures, ElementsAre("MandatoryFeature1", "MandatoryFeature2"))); +} + +TEST(machines, getMachinesWithIncorrectFormat) { + settings.builders = "nix@scratchy.labs.cs.uu.nl - - eight"; + EXPECT_THROW(getMachines(), FormatError); + settings.builders = "nix@scratchy.labs.cs.uu.nl - - -1"; + EXPECT_THROW(getMachines(), FormatError); + settings.builders = "nix@scratchy.labs.cs.uu.nl - - 8 three"; + EXPECT_THROW(getMachines(), FormatError); + settings.builders = "nix@scratchy.labs.cs.uu.nl - - 8 -3"; + EXPECT_THROW(getMachines(), FormatError); + settings.builders = "nix@scratchy.labs.cs.uu.nl - - 8 3 - - BAD_BASE64"; + EXPECT_THROW(getMachines(), FormatError); +} + +TEST(machines, getMachinesWithCorrectFileReference) { + auto path = absPath("src/libstore/tests/test-data/machines.valid"); + ASSERT_TRUE(pathExists(path)); + + settings.builders = std::string("@") + path; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(3)); + EXPECT_THAT(actual, Contains(Field(&Machine::storeUri, EndsWith("nix@scratchy.labs.cs.uu.nl")))); + EXPECT_THAT(actual, Contains(Field(&Machine::storeUri, EndsWith("nix@itchy.labs.cs.uu.nl")))); + EXPECT_THAT(actual, Contains(Field(&Machine::storeUri, EndsWith("nix@poochie.labs.cs.uu.nl")))); +} + +TEST(machines, getMachinesWithCorrectFileReferenceToEmptyFile) { + auto path = "/dev/null"; + ASSERT_TRUE(pathExists(path)); + + settings.builders = std::string("@") + path; + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(0)); +} + +TEST(machines, getMachinesWithIncorrectFileReference) { + settings.builders = std::string("@") + absPath("/not/a/file"); + Machines actual = getMachines(); + ASSERT_THAT(actual, SizeIs(0)); +} + +TEST(machines, getMachinesWithCorrectFileReferenceToIncorrectFile) { + settings.builders = std::string("@") + absPath("src/libstore/tests/test-data/machines.bad_format"); + EXPECT_THROW(getMachines(), FormatError); +} diff --git a/src/libstore/tests/test-data/machines.bad_format b/src/libstore/tests/test-data/machines.bad_format new file mode 100644 index 000000000..7255a1216 --- /dev/null +++ b/src/libstore/tests/test-data/machines.bad_format @@ -0,0 +1 @@ +nix@scratchy.labs.cs.uu.nl - - eight diff --git a/src/libstore/tests/test-data/machines.valid b/src/libstore/tests/test-data/machines.valid new file mode 100644 index 000000000..1a6c8017c --- /dev/null +++ b/src/libstore/tests/test-data/machines.valid @@ -0,0 +1,3 @@ +nix@scratchy.labs.cs.uu.nl i686-linux /home/nix/.ssh/id_scratchy_auto 8 1 kvm +nix@itchy.labs.cs.uu.nl i686-linux /home/nix/.ssh/id_scratchy_auto 8 2 +nix@poochie.labs.cs.uu.nl i686-linux /home/nix/.ssh/id_scratchy_auto 1 2 kvm benchmark c3NoLXJzYSBBQUFBQjNOemFDMXljMkVBQUFBREFRQUJBQUFDQVFDWWV5R1laNTNzd1VjMUZNSHBWL1BCcXlKaFR5S1JoRkpWWVRpRHlQN2h5c1JGa0w4VDlLOGdhL2Y2L3c3QjN2SjNHSFRIUFkybENiUEdZbGNLd2h6M2ZRbFNNOEViNi95b3ZLajdvM1FsMEx5Y0dzdGJvRmcwWkZKNldncUxsR0ltS0NobUlxOGZ3TW5ZTWUxbnRQeTBUZFZjSU1tOTV3YzF3SjBMd2c3cEVMRmtHazdkeTVvYnM4a3lGZ0pORDVRSmFwQWJjeWp4Z1QzdzdMcktNZ2xzeWhhd01JNVpkMGZsQTVudW5OZ3pid3plYVhLaUsyTW0vdGJXYTU1YTd4QmNYdHpIZGlPSWdSajJlRWxaMGh5bk10YjBmcklsdmxIcEtLaVFaZ3pQdCtIVXQ2bXpRMkRVME52MGYyYnNSU0krOGpJU2pQcmdlcVVHRldMUzVIUTg2N2xSMlpiaWtyclhZNTdqbVFEZk5DRHY1VFBHZU9UekFEd2pjMDc2aFZ3VFJCd3VTZFhtaWNxTS95b3lrWitkV1dnZ25MenE5QU1tdlNZcDhmZkZDcS9CSDBZNUFXWTFHay9vS3hMVTNaOWt3ZDd2UWNFQWFCQ2dxdnVZRGdTaHE1RlhndDM3OVZESWtEL05ZSTg2QXVvajVDRmVNTzlRM2pJSlRadlh6c1VldjVoSnA2djcxSVh5ODVtbTY5R20zcXdicVE1SjVQZDU1Um56SitpaW5BNjZxTEFSc0Y4amNsSnd5ekFXclBoYU9DRVY2bjVMeVhVazhzMW9EVVR4V1pWN25rVkFTbHJ0MllGcjN5dzdjRTRXQVhsemhHcDhocmdLMVVkMUlyeDVnZWRaSnBWcy9uNWVybmJFMUxmb2x5UHUvRUFIWlh6VGd4dHVDUFNobXc9PQo=