libstore: refuse to serialise ancient protocols

We don't want to deal with these at all, let's stop doing so.

(marking this one as the fix commit since its immediate predecessors
aren't the complete fix)
Fixes: lix-project/lix#325

Change-Id: Ieea1b0b8ac0f903d1e24e5b3e63cfe12eeec119d

src/libstore/daemon.cc

@@ -45,10 +45,15 @@ struct TunnelLogger : public Logger
 
     Sync<State> state_;
 
-    WorkerProto::Version clientVersion;
+    /**
+     * Worker protocol version of the other side. May be newer than this daemon.
+     */
+    const WorkerProto::Version clientVersion;
 
     TunnelLogger(FdSink & to, WorkerProto::Version clientVersion)
-        : to(to), clientVersion(clientVersion) { }
+        : to(to), clientVersion(clientVersion) {
+        assert(clientVersion >= MIN_SUPPORTED_WORKER_PROTO_VERSION);
+    }
 
     void enqueueMsg(const std::string & s)
     {
@@ -127,12 +132,6 @@ struct TunnelLogger : public Logger
     void startActivity(ActivityId act, Verbosity lvl, ActivityType type,
         const std::string & s, const Fields & fields, ActivityId parent) override
     {
-        if (GET_PROTOCOL_MINOR(clientVersion) < 20) {
-            if (!s.empty())
-                log(lvl, s + "...");
-            return;
-        }
-
         StringSink buf;
         buf << STDERR_START_ACTIVITY << act << lvl << type << s << fields << parent;
         enqueueMsg(buf.s);
@@ -140,7 +139,6 @@ struct TunnelLogger : public Logger
 
     void stopActivity(ActivityId act) override
     {
-        if (GET_PROTOCOL_MINOR(clientVersion) < 20) return;
         StringSink buf;
         buf << STDERR_STOP_ACTIVITY << act;
         enqueueMsg(buf.s);
@@ -148,7 +146,6 @@ struct TunnelLogger : public Logger
 
     void result(ActivityId act, ResultType type, const Fields & fields) override
     {
-        if (GET_PROTOCOL_MINOR(clientVersion) < 20) return;
         StringSink buf;
         buf << STDERR_RESULT << act << type << fields;
         enqueueMsg(buf.s);
@@ -265,14 +262,8 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
     TrustedFlag trusted, RecursiveFlag recursive, WorkerProto::Version clientVersion,
     Source & from, BufferedSink & to, WorkerProto::Op op)
 {
-    WorkerProto::ReadConn rconn {
-        .from = from,
-        .version = clientVersion,
-    };
-    WorkerProto::WriteConn wconn {
-        .to = to,
-        .version = clientVersion,
-    };
+    WorkerProto::ReadConn rconn{from, clientVersion};
+    WorkerProto::WriteConn wconn{to, clientVersion};
 
     switch (op) {
 
@@ -525,9 +516,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
 
     case WorkerProto::Op::BuildPaths: {
         auto drvs = WorkerProto::Serialise<DerivedPaths>::read(*store, rconn);
-        BuildMode mode = bmNormal;
-        if (GET_PROTOCOL_MINOR(clientVersion) >= 15) {
-            mode = (BuildMode) readInt(from);
+        BuildMode mode = buildModeFromInteger(readInt(from));
 
         /* Repairing is not atomic, so disallowed for "untrusted"
            clients.
@@ -540,7 +529,6 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
           */
         if (mode == bmRepair && !trusted)
             throw Error("repairing is not allowed because you are not in 'trusted-users'");
-        }
         logger->startWork();
         store->buildPaths(drvs, mode);
         logger->stopWork();
@@ -551,7 +539,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
     case WorkerProto::Op::BuildPathsWithResults: {
         auto drvs = WorkerProto::Serialise<DerivedPaths>::read(*store, rconn);
         BuildMode mode = bmNormal;
-        mode = (BuildMode) readInt(from);
+        mode = buildModeFromInteger(readInt(from));
 
         /* Repairing is not atomic, so disallowed for "untrusted"
            clients.
@@ -582,7 +570,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
          * correctly.
          */
         readDerivation(from, *store, drv, Derivation::nameFromPath(drvPath));
-        BuildMode buildMode = (BuildMode) readInt(from);
+        BuildMode buildMode = buildModeFromInteger(readInt(from));
         logger->startWork();
 
         auto drvType = drv.type();
@@ -744,14 +732,12 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         clientSettings.buildCores = readInt(from);
         clientSettings.useSubstitutes = readInt(from);
 
-        if (GET_PROTOCOL_MINOR(clientVersion) >= 12) {
         unsigned int n = readInt(from);
         for (unsigned int i = 0; i < n; i++) {
             auto name = readString(from);
             auto value = readString(from);
             clientSettings.overrides.emplace(name, value);
         }
-        }
 
         logger->startWork();
 
@@ -820,15 +806,14 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         try {
             info = store->queryPathInfo(path);
         } catch (InvalidPath &) {
-            if (GET_PROTOCOL_MINOR(clientVersion) < 17) throw;
+            // The path being invalid isn't fatal here since it will just be
+            // sent as not present.
         }
         logger->stopWork();
         if (info) {
-            if (GET_PROTOCOL_MINOR(clientVersion) >= 17)
             to << 1;
             WorkerProto::write(*store, wconn, static_cast<const UnkeyedValidPathInfo &>(*info));
         } else {
-            assert(GET_PROTOCOL_MINOR(clientVersion) >= 17);
             to << 0;
         }
         break;
@@ -902,12 +887,7 @@ static void performOp(TunnelLogger * logger, ref<Store> store,
         else {
             std::unique_ptr<Source> source;
             StringSink saved;
-            if (GET_PROTOCOL_MINOR(clientVersion) >= 21)
             source = std::make_unique<TunnelSource>(from, to);
-            else {
-                copyNAR(from, saved);
-                source = std::make_unique<StringSource>(saved.s);
-            }
 
             logger->startWork();
 
@@ -1009,7 +989,7 @@ void processConnection(
     to.flush();
     WorkerProto::Version clientVersion = readInt(from);
 
-    if (clientVersion < 0x10a)
+    if (clientVersion < MIN_SUPPORTED_WORKER_PROTO_VERSION)
         throw Error("the Nix client version is too old");
 
     auto tunnelLogger = new TunnelLogger(to, clientVersion);
@@ -1025,12 +1005,12 @@ void processConnection(
         printMsgUsing(prevLogger, lvlDebug, "%d operations", opCount);
     });
 
-    if (GET_PROTOCOL_MINOR(clientVersion) >= 14 && readInt(from)) {
+    // FIXME: what is *supposed* to be in this even?
+    if (readInt(from)) {
         // Obsolete CPU affinity.
         readInt(from);
     }
 
-    if (GET_PROTOCOL_MINOR(clientVersion) >= 11)
     readInt(from); // obsolete reserveSpace
 
     if (GET_PROTOCOL_MINOR(clientVersion) >= 33)
@@ -1042,10 +1022,7 @@ void processConnection(
         auto temp = trusted
             ? store->isTrustedClient()
             : std::optional { NotTrusted };
-        WorkerProto::WriteConn wconn {
-            .to = to,
-            .version = clientVersion,
-        };
+        WorkerProto::WriteConn wconn {to, clientVersion};
         WorkerProto::write(*store, wconn, temp);
     }
 

src/libstore/remote-store-connection.hh

@@ -27,11 +27,8 @@ struct RemoteStore::Connection
     FdSource from;
 
     /**
-     * Worker protocol version used for the connection.
-     *
-     * Despite its name, I think it is actually the maximum version both
-     * sides support. (If the maximum doesn't exist, we would fail to
-     * establish a connection and produce a value of this type.)
+     * The worker protocol version of the connected daemon. This may be newer
+     * than this Lix supports.
      */
     WorkerProto::Version daemonVersion;
 
@@ -71,10 +68,7 @@ struct RemoteStore::Connection
      */
     operator WorkerProto::ReadConn ()
     {
-        return WorkerProto::ReadConn {
-            .from = from,
-            .version = daemonVersion,
-        };
+        return WorkerProto::ReadConn {from, daemonVersion};
     }
 
     /**
@@ -87,10 +81,7 @@ struct RemoteStore::Connection
      */
     operator WorkerProto::WriteConn ()
     {
-        return WorkerProto::WriteConn {
-            .to = to,
-            .version = daemonVersion,
-        };
+        return WorkerProto::WriteConn {to, daemonVersion};
     }
 
     virtual ~Connection();

src/libstore/remote-store.cc

@@ -76,16 +76,13 @@ void RemoteStore::initConnection(Connection & conn)
         conn.from >> conn.daemonVersion;
         if (GET_PROTOCOL_MAJOR(conn.daemonVersion) != GET_PROTOCOL_MAJOR(PROTOCOL_VERSION))
             throw Error("Nix daemon protocol version not supported");
-        if (GET_PROTOCOL_MINOR(conn.daemonVersion) < 10)
+        if (GET_PROTOCOL_MINOR(conn.daemonVersion) < MIN_SUPPORTED_MINOR_WORKER_PROTO_VERSION)
             throw Error("the Nix daemon version is too old");
         conn.to << PROTOCOL_VERSION;
 
-        if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 14) {
         // Obsolete CPU affinity.
         conn.to << 0;
-        }
 
-        if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 11)
         conn.to << false; // obsolete reserveSpace
 
         if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 33) {
@@ -127,7 +124,6 @@ void RemoteStore::setOptions(Connection & conn)
        << settings.buildCores
        << settings.useSubstitutes;
 
-    if (GET_PROTOCOL_MINOR(conn.daemonVersion) >= 12) {
     std::map<std::string, Config::SettingInfo> overrides;
     settings.getSettings(overrides, true); // libstore settings
     fileTransferSettings.getSettings(overrides, true);
@@ -144,7 +140,6 @@ void RemoteStore::setOptions(Connection & conn)
     conn.to << overrides.size();
     for (auto & i : overrides)
         conn.to << i.first << i.second.value;
-    }
 
     auto ex = conn.processStderr();
     if (ex) std::rethrow_exception(ex);
@@ -208,12 +203,6 @@ bool RemoteStore::isValidPathUncached(const StorePath & path)
 StorePathSet RemoteStore::queryValidPaths(const StorePathSet & paths, SubstituteFlag maybeSubstitute)
 {
     auto conn(getConnection());
-    if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 12) {
-        StorePathSet res;
-        for (auto & i : paths)
-            if (isValidPath(i)) res.insert(i);
-        return res;
-    } else {
     conn->to << WorkerProto::Op::QueryValidPaths;
     WorkerProto::write(*this, *conn, paths);
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 27) {
@@ -221,7 +210,6 @@ StorePathSet RemoteStore::queryValidPaths(const StorePathSet & paths, Substitute
     }
     conn.processStderr();
     return WorkerProto::Serialise<StorePathSet>::read(*this, *conn);
-    }
 }
 
 
@@ -237,20 +225,10 @@ StorePathSet RemoteStore::queryAllValidPaths()
 StorePathSet RemoteStore::querySubstitutablePaths(const StorePathSet & paths)
 {
     auto conn(getConnection());
-    if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 12) {
-        StorePathSet res;
-        for (auto & i : paths) {
-            conn->to << WorkerProto::Op::HasSubstitutes << printStorePath(i);
-            conn.processStderr();
-            if (readInt(conn->from)) res.insert(i);
-        }
-        return res;
-    } else {
     conn->to << WorkerProto::Op::QuerySubstitutablePaths;
     WorkerProto::write(*this, *conn, paths);
     conn.processStderr();
     return WorkerProto::Serialise<StorePathSet>::read(*this, *conn);
-    }
 }
 
 
@@ -260,24 +238,6 @@ void RemoteStore::querySubstitutablePathInfos(const StorePathCAMap & pathsMap, S
 
     auto conn(getConnection());
 
-    if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 12) {
-
-        for (auto & i : pathsMap) {
-            SubstitutablePathInfo info;
-            conn->to << WorkerProto::Op::QuerySubstitutablePathInfo << printStorePath(i.first);
-            conn.processStderr();
-            unsigned int reply = readInt(conn->from);
-            if (reply == 0) continue;
-            auto deriver = readString(conn->from);
-            if (deriver != "")
-                info.deriver = parseStorePath(deriver);
-            info.references = WorkerProto::Serialise<StorePathSet>::read(*this, *conn);
-            info.downloadSize = readLongLong(conn->from);
-            info.narSize = readLongLong(conn->from);
-            infos.insert_or_assign(i.first, std::move(info));
-        }
-
-    } else {
 
     conn->to << WorkerProto::Op::QuerySubstitutablePathInfos;
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) < 22) {
@@ -298,8 +258,6 @@ void RemoteStore::querySubstitutablePathInfos(const StorePathCAMap & pathsMap, S
         info.downloadSize = readLongLong(conn->from);
         info.narSize = readLongLong(conn->from);
     }
-
-    }
 }
 
 
@@ -315,10 +273,9 @@ std::shared_ptr<const ValidPathInfo> RemoteStore::queryPathInfoUncached(const St
             throw InvalidPath(std::move(e.info()));
         throw;
     }
-    if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) {
     bool valid; conn->from >> valid;
     if (!valid) throw InvalidPath("path '%s' is not valid", printStorePath(path));
-    }
+
     return std::make_shared<ValidPathInfo>(
         StorePath{path},
         WorkerProto::Serialise<UnkeyedValidPathInfo>::read(*this, *conn));
@@ -347,7 +304,7 @@ StorePathSet RemoteStore::queryValidDerivers(const StorePath & path)
 
 StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path)
 {
-    if (GET_PROTOCOL_MINOR(getProtocol()) >= 0x16) {
+    if (GET_PROTOCOL_MINOR(getProtocol()) >= 22) {
         return Store::queryDerivationOutputs(path);
     }
     auto conn(getConnection());
@@ -359,7 +316,7 @@ StorePathSet RemoteStore::queryDerivationOutputs(const StorePath & path)
 
 std::map<std::string, std::optional<StorePath>> RemoteStore::queryPartialDerivationOutputMap(const StorePath & path, Store * evalStore_)
 {
-    if (GET_PROTOCOL_MINOR(getProtocol()) >= 0x16) {
+    if (GET_PROTOCOL_MINOR(getProtocol()) >= 22) {
         if (!evalStore_) {
             auto conn(getConnection());
             conn->to << WorkerProto::Op::QueryDerivationOutputMap << printStorePath(path);
@@ -509,7 +466,7 @@ void RemoteStore::addToStore(const ValidPathInfo & info, Source & source,
             sink
                 << exportMagic
                 << printStorePath(info.path);
-            WorkerProto::WriteConn nested { .to = sink, .version = conn->daemonVersion };
+            WorkerProto::WriteConn nested { sink, conn->daemonVersion };
             WorkerProto::write(*this, nested, info.references);
             sink
                 << (info.deriver ? printStorePath(*info.deriver) : "")
@@ -554,14 +511,13 @@ void RemoteStore::addMultipleToStore(
     RepairFlag repair,
     CheckSigsFlag checkSigs)
 {
+    auto remoteVersion = getProtocol();
+
     auto source = sinkToSource([&](Sink & sink) {
         sink << pathsToCopy.size();
         for (auto & [pathInfo, pathSource] : pathsToCopy) {
             WorkerProto::Serialise<ValidPathInfo>::write(*this,
-                 WorkerProto::WriteConn {
-                     .to = sink,
-                     .version = 16,
-                 },
+                 WorkerProto::WriteConn {sink, remoteVersion},
                  pathInfo);
             pathSource->drainInto(sink);
         }
@@ -670,13 +626,7 @@ void RemoteStore::buildPaths(const std::vector<DerivedPath> & drvPaths, BuildMod
     conn->to << WorkerProto::Op::BuildPaths;
     assert(GET_PROTOCOL_MINOR(conn->daemonVersion) >= 13);
     WorkerProto::write(*this, *conn, drvPaths);
-    if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 15)
     conn->to << buildMode;
-    else
-        /* Old daemons did not take a 'buildMode' parameter, so we
-           need to validate it here on the client side.  */
-        if (buildMode != bmNormal)
-            throw Error("repairing or checking is not supported when building through the Nix daemon");
     conn.processStderr();
     readInt(conn->from);
 }

src/libstore/store-api.cc

@@ -22,6 +22,14 @@ using json = nlohmann::json;
 
 namespace nix {
 
+BuildMode buildModeFromInteger(int raw) {
+    switch (raw) {
+    case bmNormal: return bmNormal;
+    case bmRepair: return bmRepair;
+    case bmCheck: return bmCheck;
+    default: throw Error("Invalid BuildMode");
+    }
+}
 
 bool Store::isInStore(PathView path) const
 {
@@ -361,15 +369,14 @@ void Store::addMultipleToStore(
     RepairFlag repair,
     CheckSigsFlag checkSigs)
 {
+    auto remoteVersion = getProtocol();
+
     auto expected = readNum<uint64_t>(source);
     for (uint64_t i = 0; i < expected; ++i) {
-        // FIXME we should not be using the worker protocol here, let
-        // alone the worker protocol with a hard-coded version!
+        // FIXME we should not be using the worker protocol here at all!
         auto info = WorkerProto::Serialise<ValidPathInfo>::read(*this,
-            WorkerProto::ReadConn {
-                .from = source,
-                .version = 16,
-            });
+            WorkerProto::ReadConn {source, remoteVersion}
+        );
         info.ultimate = false;
         addToStore(info, source, repair, checkSigs);
     }

src/libstore/store-api.hh

@@ -90,6 +90,9 @@ const uint32_t exportMagic = 0x4558494e;
 
 
 enum BuildMode { bmNormal, bmRepair, bmCheck };
+/** Checks that a build mode is a valid one, then returns it */
+BuildMode buildModeFromInteger(int);
+
 enum TrustedFlag : bool { NotTrusted = false, Trusted = true };
 
 struct BuildResult;

src/libstore/worker-protocol.hh

@@ -9,7 +9,16 @@ namespace nix {
 #define WORKER_MAGIC_1 0x6e697863
 #define WORKER_MAGIC_2 0x6478696f
 
+// This must remain 1.35 (Nix 2.18) forever in Lix, since the protocol has
+// diverged in CppNix such that we cannot assign newer versions ourselves, the
+// protocol is bad in design and implementation and Lix intends to replace it
+// entirely.
 #define PROTOCOL_VERSION (1 << 8 | 35)
+// Nix 2.3 is protocol 1.21 (see RemoteStore::initConnection for client,
+// processConnection for server).
+#define MIN_SUPPORTED_MINOR_WORKER_PROTO_VERSION 21
+#define MIN_SUPPORTED_WORKER_PROTO_VERSION (1 << 8 | MIN_SUPPORTED_MINOR_WORKER_PROTO_VERSION)
+
 #define GET_PROTOCOL_MAJOR(x) ((x) & 0xff00)
 #define GET_PROTOCOL_MINOR(x) ((x) & 0x00ff)
 
@@ -63,6 +72,10 @@ struct WorkerProto
     struct ReadConn {
         Source & from;
         Version version;
+
+        ReadConn(Source & from, Version version) : from(from), version(version) {
+            assert(version >= MIN_SUPPORTED_WORKER_PROTO_VERSION);
+        }
     };
 
     /**
@@ -72,6 +85,10 @@ struct WorkerProto
     struct WriteConn {
         Sink & to;
         Version version;
+
+        WriteConn(Sink & to, Version version) : to(to), version(version) {
+            assert(version >= MIN_SUPPORTED_WORKER_PROTO_VERSION);
+        }
     };
 
     /**

tests/unit/libstore/protocol.hh

@@ -39,10 +39,8 @@ public:
                 StringSource from { expected };
                 Proto::template Serialise<T>::read(
                     *LibStoreTest::store,
-                    typename Proto::ReadConn {
-                        .from = from,
-                        .version = version,
-                    });
+                    typename Proto::ReadConn {from, version}
+                );
             });
 
             ASSERT_EQ(got, value);
@@ -60,10 +58,7 @@ public:
         StringSink to;
         Proto::write(
             *LibStoreTest::store,
-            typename Proto::WriteConn {
-                .to = to,
-                .version = version,
-            },
+            typename Proto::WriteConn {to, version},
             value);
 
         if (testAccept())

tests/unit/libstore/worker-protocol.cc

@@ -18,9 +18,9 @@ struct WorkerProtoTest : VersionedProtoTest<WorkerProto, workerProtoDir>
 {
     /**
      * For serializers that don't care about the minimum version, we
-     * used the oldest one: 1.0.
+     * have to use the minimum supported to not throw an assert.
      */
-    WorkerProto::Version defaultVersion = 1 << 8 | 0;
+    WorkerProto::Version defaultVersion = MIN_SUPPORTED_WORKER_PROTO_VERSION;
 };
 
 
@@ -331,9 +331,9 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
-    unkeyedValidPathInfo_1_15,
-    "unkeyed-valid-path-info-1.15",
-    1 << 8 | 15,
+    unkeyedValidPathInfo,
+    "unkeyed-valid-path-info",
+    defaultVersion,
     (std::tuple<UnkeyedValidPathInfo, UnkeyedValidPathInfo> {
         ({
             UnkeyedValidPathInfo info {
@@ -363,56 +363,9 @@ VERSIONED_CHARACTERIZATION_TEST(
 
 VERSIONED_CHARACTERIZATION_TEST(
     WorkerProtoTest,
-    validPathInfo_1_15,
-    "valid-path-info-1.15",
-    1 << 8 | 15,
-    (std::tuple<ValidPathInfo, ValidPathInfo> {
-        ({
-            ValidPathInfo info {
-                StorePath {
-                    "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar",
-                },
-                UnkeyedValidPathInfo {
-                    Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="),
-                },
-            };
-            info.registrationTime = 23423;
-            info.narSize = 34878;
-            info;
-        }),
-        ({
-            ValidPathInfo info {
-                StorePath {
-                    "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar",
-                },
-                UnkeyedValidPathInfo {
-                    Hash::parseSRI("sha256-FePFYIlMuycIXPZbWi7LGEiMmZSX9FMbaQenWBzm1Sc="),
-                },
-            };
-            info.deriver = StorePath {
-                "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar.drv",
-            };
-            info.references = {
-                // other reference
-                StorePath {
-                    "g1w7hyyyy1w7hy3qg1w7hy3qgqqqqy3q-foo",
-                },
-                // self reference
-                StorePath {
-                    "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-bar",
-                },
-            };
-            info.registrationTime = 23423;
-            info.narSize = 34878;
-            info;
-        }),
-    }))
-
-VERSIONED_CHARACTERIZATION_TEST(
-    WorkerProtoTest,
-    validPathInfo_1_16,
-    "valid-path-info-1.16",
-    1 << 8 | 16,
+    validPathInfo,
+    "valid-path-info",
+    defaultVersion,
     (std::tuple<ValidPathInfo, ValidPathInfo, ValidPathInfo> {
         ({
             ValidPathInfo info {