From b71673109c2172cb1f933cc8a97c26b4352ac239 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Mon, 22 Jan 2024 15:50:00 -0500 Subject: [PATCH] Make `SSHMaster::startCommand` work on an args list This avoids split-on-whitespace errors: - No more `bash -c` needed - No more `shellEscape` needed - `remote-program` ssh store setting also cleanly supports args (e.g. `nix daemon`) - `ssh` uses `--` to separate args for SSH from args for the command to run. and will help with Hydra dedup. Some code taken from #6628. Co-Authored-By: Alexander Bantyev --- src/libstore/legacy-ssh-store.cc | 11 ++++++++--- src/libstore/legacy-ssh-store.hh | 2 +- src/libstore/ssh-store.cc | 19 ++++++++++--------- src/libstore/ssh.cc | 12 +++++++----- src/libstore/ssh.hh | 11 ++++++++++- 5 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/libstore/legacy-ssh-store.cc b/src/libstore/legacy-ssh-store.cc index 4f020c452..e422adeec 100644 --- a/src/libstore/legacy-ssh-store.cc +++ b/src/libstore/legacy-ssh-store.cc @@ -55,9 +55,14 @@ LegacySSHStore::LegacySSHStore(const std::string & scheme, const std::string & h ref LegacySSHStore::openConnection() { auto conn = make_ref(); - conn->sshConn = master.startCommand( - fmt("%s --serve --write", remoteProgram) - + (remoteStore.get() == "" ? "" : " --store " + shellEscape(remoteStore.get()))); + Strings command = remoteProgram.get(); + command.push_back("--serve"); + command.push_back("--write"); + if (remoteStore.get() != "") { + command.push_back("--store"); + command.push_back(remoteStore.get()); + } + conn->sshConn = master.startCommand(std::move(command)); conn->to = FdSink(conn->sshConn->in.get()); conn->from = FdSource(conn->sshConn->out.get()); diff --git a/src/libstore/legacy-ssh-store.hh b/src/libstore/legacy-ssh-store.hh index bdf79eab3..ae890177b 100644 --- a/src/libstore/legacy-ssh-store.hh +++ b/src/libstore/legacy-ssh-store.hh @@ -13,7 +13,7 @@ struct LegacySSHStoreConfig : virtual CommonSSHStoreConfig { using CommonSSHStoreConfig::CommonSSHStoreConfig; - const Setting remoteProgram{this, "nix-store", "remote-program", + const Setting remoteProgram{this, {"nix-store"}, "remote-program", "Path to the `nix-store` executable on the remote machine."}; const Setting maxConnections{this, 1, "max-connections", diff --git a/src/libstore/ssh-store.cc b/src/libstore/ssh-store.cc index d4c8ab5b2..0cf92b114 100644 --- a/src/libstore/ssh-store.cc +++ b/src/libstore/ssh-store.cc @@ -17,7 +17,7 @@ struct SSHStoreConfig : virtual RemoteStoreConfig, virtual CommonSSHStoreConfig using RemoteStoreConfig::RemoteStoreConfig; using CommonSSHStoreConfig::CommonSSHStoreConfig; - const Setting remoteProgram{this, "nix-daemon", "remote-program", + const Setting remoteProgram{this, {"nix-daemon"}, "remote-program", "Path to the `nix-daemon` executable on the remote machine."}; const std::string name() override { return "Experimental SSH Store"; } @@ -212,14 +212,15 @@ public: ref SSHStore::openConnection() { auto conn = make_ref(); - - std::string command = remoteProgram + " --stdio"; - if (remoteStore.get() != "") - command += " --store " + shellEscape(remoteStore.get()); - for (auto & arg : extraRemoteProgramArgs) - command += " " + shellEscape(arg); - - conn->sshConn = master.startCommand(command); + Strings command = remoteProgram.get(); + command.push_back("--stdio"); + if (remoteStore.get() != "") { + command.push_back("--store"); + command.push_back(remoteStore.get()); + } + command.insert(command.end(), + extraRemoteProgramArgs.begin(), extraRemoteProgramArgs.end()); + conn->sshConn = master.startCommand(std::move(command)); conn->to = FdSink(conn->sshConn->in.get()); conn->from = FdSource(conn->sshConn->out.get()); return conn; diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index 5c8d6a504..30fe73adb 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -52,7 +52,8 @@ bool SSHMaster::isMasterRunning() { return res.first == 0; } -std::unique_ptr SSHMaster::startCommand(const std::string & command) +std::unique_ptr SSHMaster::startCommand( + Strings && command, Strings && extraSshArgs) { Path socketPath = startMaster(); @@ -84,18 +85,19 @@ std::unique_ptr SSHMaster::startCommand(const std::string Strings args; - if (fakeSSH) { - args = { "bash", "-c" }; - } else { + if (!fakeSSH) { args = { "ssh", host.c_str(), "-x" }; addCommonSSHOpts(args); if (socketPath != "") args.insert(args.end(), {"-S", socketPath}); if (verbosity >= lvlChatty) args.push_back("-v"); + args.splice(args.end(), std::move(extraSshArgs)); + args.push_back("--"); } - args.push_back(command); + args.splice(args.end(), std::move(command)); + execvp(args.begin()->c_str(), stringsToCharPtrs(args).data()); // could not exec ssh/bash diff --git a/src/libstore/ssh.hh b/src/libstore/ssh.hh index bfcd6f21c..08bb43dfa 100644 --- a/src/libstore/ssh.hh +++ b/src/libstore/ssh.hh @@ -41,7 +41,16 @@ public: AutoCloseFD out, in; }; - std::unique_ptr startCommand(const std::string & command); + /** + * @param command The command (arg vector) to execute. + * + * @param extraSShArgs Extra args to pass to SSH (not the command to + * execute). Will not be used when "fake SSHing" to the local + * machine. + */ + std::unique_ptr startCommand( + Strings && command, + Strings && extraSshArgs = {}); Path startMaster(); };