feat engine: account for PATH in ProcessStarter::Exec

cd29b43fbef034256d01e7f0a1e0d48f6f188a72

Author: i9kin

core/include/userver/engine/subprocess/environment_variables.hpp

@@ -48,8 +48,10 @@ class EnvironmentVariables {
   /// @brief Constructs an instance from pairs: key, value taken from the map.
   explicit EnvironmentVariables(Map vars);
 
-  /// Constructs copy of the instance.
   EnvironmentVariables(const EnvironmentVariables&) = default;
+  EnvironmentVariables& operator=(const EnvironmentVariables&) = default;
+  EnvironmentVariables(EnvironmentVariables&&) noexcept = default;
+  EnvironmentVariables& operator=(EnvironmentVariables&&) noexcept = default;
 
   /// @brief Updates variable.
   /// @note If variable does not exist then it is added.
@@ -69,13 +71,17 @@ class EnvironmentVariables {
   /// Returns the reference to the value.
   std::string& operator[](const std::string& variable_name);
 
+  /// Compares equal if thee map of environment variables are equal.
+  bool operator==(const EnvironmentVariables& rhs) const;
+
   /// Checks whether the container is empty.
   auto empty() const { return vars_.empty(); }
 
   /// Returns the number of elements.
   auto size() const { return vars_.size(); }
 
   using const_iterator = Map::const_iterator;
+  using iterator = const_iterator;
 
   /// Returns a const iterator to the beginning.
   auto begin() const { return vars_.begin(); }
@@ -117,6 +123,17 @@ void SetEnvironmentVariable(const std::string& variable_name,
 /// @warning Not thread-safe.
 void UnsetEnvironmentVariable(const std::string& variable_name);
 
+/// @brief RAII idiom guard of environment variables.
+/// @warning The constructor and destructor are not thread-safe.
+class EnvironmentVariablesScope {
+ public:
+  EnvironmentVariablesScope();
+  ~EnvironmentVariablesScope();
+
+ private:
+  rcu::ReadablePtr<EnvironmentVariables> old_env_;
+};
+
 }  // namespace engine::subprocess
 
 USERVER_NAMESPACE_END

core/include/userver/engine/subprocess/process_starter.hpp

@@ -20,6 +20,29 @@ class ThreadControl;
 
 namespace engine::subprocess {
 
+/// @brief Structure of settings for executing commands in the OS.
+struct ExecOptions final {
+  /// EnvironmentVariables for the environment in execution, or `std::nullopt`
+  /// to use GetCurrentEnvironmentVariables()
+  std::optional<EnvironmentVariables> env{};
+  /// EnvironmentVariablesUpdate for the update environment before execution, or
+  /// `std::nullopt` to leave `env` as is
+  std::optional<EnvironmentVariablesUpdate> env_update{};
+  /// File path to be redirected stdout, or `std::nullopt` to use the service's
+  /// stdout
+  std::optional<std::string> stdout_file{};
+  /// File path to be redirected stderr, or `std::nullopt` to use the service's
+  /// stderr
+  std::optional<std::string> stderr_file{};
+  /// If `false`, `command` is treated as absolute path or a relative path.
+  /// If `true`, and `command` does not contain `/`, and PATH in environment
+  /// variables, then it will be searched in the colon-separated list of
+  /// directory pathnames specified in the PATH environment variable.
+  /// If `true`, and `command` contains `/`, `command` is treated as absolute
+  /// path or a relative path.
+  bool use_path{false};
+};
+
 /// @ingroup userver_clients
 ///
 /// @brief Creates a new OS subprocess and executes a command in it.
@@ -29,23 +52,39 @@ class ProcessStarter {
   /// `main-task-processor is OK for this purpose.
   explicit ProcessStarter(TaskProcessor& task_processor);
 
-  /// @param command the absolute path to the executable
+  /// @param command the absolute path or relative path. If `use_path` is
+  /// `true`, and `command` does not contains `/`, then it will be searched in
+  /// the colon-separated list of directory pathnames specified in the PATH
+  /// environment variable. More details @ref ExecOptions::use_path
+  /// @param args exact args passed to the executable
+  /// @param options @ref ExecOptions settings
+  /// @throws std::runtime_error if `use_path` is `true`, `command` contains `/`
+  /// and PATH not in environment variables
+  ChildProcess Exec(const std::string& command,
+                    const std::vector<std::string>& args,
+                    ExecOptions&& options = {});
+
+  /// @overload
+  /// @param command the absolute path or relative path
   /// @param args exact args passed to the executable
   /// @param env redefines all environment variables
+  /// @deprecated Use the `Exec` overload taking @ref ExecOptions
   ChildProcess Exec(
       const std::string& command, const std::vector<std::string>& args,
       const EnvironmentVariables& env,
       // TODO: use something like pipes instead of path to files
       const std::optional<std::string>& stdout_file = std::nullopt,
       const std::optional<std::string>& stderr_file = std::nullopt);
 
-  /// @param command the absolute path to the executable
+  /// @overload
+  /// @param command the absolute path or relative path
   /// @param args exact args passed to the executable
   /// @param env_update variables to add to the current environment, overwriting
   /// existing ones
+  /// @deprecated Use the `Exec` overload taking @ref ExecOptions
   ChildProcess Exec(
       const std::string& command, const std::vector<std::string>& args,
-      EnvironmentVariablesUpdate env_update = EnvironmentVariablesUpdate{{}},
+      EnvironmentVariablesUpdate env_update,
       // TODO: use something like pipes instead of path to files
       const std::optional<std::string>& stdout_file = std::nullopt,
       const std::optional<std::string>& stderr_file = std::nullopt);

core/src/engine/subprocess/environment_variables.cpp

@@ -14,6 +14,30 @@ USERVER_NAMESPACE_BEGIN
 namespace engine::subprocess {
 namespace {
 
+void UnsetEnvironmentVariableWithoutSync(const std::string& variable_name) {
+  // NOLINTNEXTLINE(concurrency-mt-unsafe)
+  ::unsetenv(variable_name.c_str());
+}
+
+void SetEnvironmentVariableWithoutSync(const std::string& variable_name,
+                                       const std::string& value,
+                                       Overwrite overwrite) {
+  const auto read_ptr = GetCurrentEnvironmentVariablesPtr();
+  const bool value_exist =
+      (read_ptr->GetValueOptional(variable_name) != nullptr);
+  if (value_exist) {
+    UINVARIANT(overwrite != Overwrite::kForbidden,
+               "variable with name= " + variable_name +
+                   " was set earlier and prohibits rewriting");
+    if (overwrite == Overwrite::kIgnored) {
+      return;
+    }
+  }
+
+  // NOLINTNEXTLINE(concurrency-mt-unsafe)
+  ::setenv(variable_name.c_str(), value.c_str(), 1);
+}
+
 EnvironmentVariables::Map GetCurrentEnvironmentVariablesMap() {
   EnvironmentVariables::Map map;
 
@@ -69,6 +93,10 @@ std::string& EnvironmentVariables::operator[](
   return vars_[variable_name];
 }
 
+bool EnvironmentVariables::operator==(const EnvironmentVariables& rhs) const {
+  return vars_ == rhs.vars_;
+}
+
 EnvironmentVariables GetCurrentEnvironmentVariables() {
   return GetCurrentEnvironmentVariablesStorage().ReadCopy();
 }
@@ -82,30 +110,38 @@ void UpdateCurrentEnvironmentVariables() {
       EnvironmentVariables{GetCurrentEnvironmentVariablesMap()});
 }
 
-void SetEnvironmentVariable(const std::string& variable_name,
-                            const std::string& value, Overwrite overwrite) {
-  const auto read_ptr = GetCurrentEnvironmentVariablesPtr();
-  const bool value_exist =
-      (read_ptr->GetValueOptional(variable_name) != nullptr);
-  if (value_exist) {
-    UINVARIANT(overwrite != Overwrite::kForbidden,
-               "variable with name= " + variable_name +
-                   " was set earlier and prohibits rewriting");
-    if (overwrite == Overwrite::kIgnored) {
-      return;
+EnvironmentVariablesScope::EnvironmentVariablesScope()
+    : old_env_(GetCurrentEnvironmentVariablesPtr()) {}
+
+EnvironmentVariablesScope::~EnvironmentVariablesScope() {
+  const auto snapshot = GetCurrentEnvironmentVariablesStorage().Read();
+  const auto& env = *snapshot;
+
+  for (const auto& [variable_name, value] : *old_env_) {
+    if (!env.GetValueOptional(variable_name) ||
+        env.GetValue(variable_name) != value) {
+      SetEnvironmentVariableWithoutSync(variable_name, value,
+                                        Overwrite::kAllowed);
     }
   }
 
-  // NOLINTNEXTLINE(concurrency-mt-unsafe)
-  ::setenv(variable_name.c_str(), value.c_str(), 1);
+  for (const auto& [variable_name, value] : env) {
+    if (!old_env_->GetValueOptional(variable_name)) {
+      UnsetEnvironmentVariableWithoutSync(variable_name);
+    }
+  }
 
   UpdateCurrentEnvironmentVariables();
 }
 
-void UnsetEnvironmentVariable(const std::string& variable_name) {
-  // NOLINTNEXTLINE(concurrency-mt-unsafe)
-  ::unsetenv(variable_name.c_str());
+void SetEnvironmentVariable(const std::string& variable_name,
+                            const std::string& value, Overwrite overwrite) {
+  SetEnvironmentVariableWithoutSync(variable_name, value, overwrite);
+  UpdateCurrentEnvironmentVariables();
+}
 
+void UnsetEnvironmentVariable(const std::string& variable_name) {
+  UnsetEnvironmentVariableWithoutSync(variable_name);
   UpdateCurrentEnvironmentVariables();
 }
 

core/src/engine/subprocess/process_starter.cpp

@@ -24,15 +24,17 @@
 #include <userver/utils/algo.hpp>
 #include <utils/check_syscall.hpp>
 
+extern char** environ;
+
 USERVER_NAMESPACE_BEGIN
 
 namespace engine::subprocess {
 namespace {
 
-void DoExecve(const std::string& command, const std::vector<std::string>& args,
-              const EnvironmentVariables& env,
-              const std::optional<std::string>& stdout_file,
-              const std::optional<std::string>& stderr_file) {
+void DoExec(const std::string& command, const std::vector<std::string>& args,
+            const EnvironmentVariables& env,
+            const std::optional<std::string>& stdout_file,
+            const std::optional<std::string>& stderr_file, bool use_path) {
   if (stdout_file) {
     if (!std::freopen(stdout_file->c_str(), "a", stdout)) {
       utils::CheckSyscall(-1, "freopen stdout to {}", *stdout_file);
@@ -65,8 +67,33 @@ void DoExecve(const std::string& command, const std::vector<std::string>& args,
   }
   envp_ptrs.push_back(nullptr);
 
-  utils::CheckSyscall(
-      execve(command.c_str(), argv_ptrs.data(), envp_ptrs.data()), "execve");
+  environ = envp_ptrs.data();  // The variable is assigned to the environment to
+                               // use the execv, execvp functions
+
+  if (!use_path) {
+    utils::CheckSyscall(execv(command.c_str(), argv_ptrs.data()), "execv");
+  } else {
+    utils::CheckSyscall(execvp(command.c_str(), argv_ptrs.data()), "execvp");
+  }
+}
+
+EnvironmentVariables ApplyEnviromentUpdate(
+    std::optional<EnvironmentVariables>&& env,
+    std::optional<EnvironmentVariablesUpdate>&& env_update) {
+  if (env) {
+    if (env_update) {
+      return env->UpdateWith(std::move(env_update.value()));
+    } else {
+      return std::move(env.value());
+    }
+  } else {
+    if (env_update) {
+      return GetCurrentEnvironmentVariables().UpdateWith(
+          std::move(env_update.value()));
+    } else {
+      return GetCurrentEnvironmentVariables();
+    }
+  }
 }
 
 }  // namespace
@@ -75,11 +102,19 @@ ProcessStarter::ProcessStarter(TaskProcessor& task_processor)
     : thread_control_(
           task_processor.EventThreadPool().GetEvDefaultLoopThread()) {}
 
-ChildProcess ProcessStarter::Exec(
-    const std::string& command, const std::vector<std::string>& args,
-    const EnvironmentVariables& env,
-    const std::optional<std::string>& stdout_file,
-    const std::optional<std::string>& stderr_file) {
+ChildProcess ProcessStarter::Exec(const std::string& command,
+                                  const std::vector<std::string>& args,
+                                  ExecOptions&& options) {
+  EnvironmentVariables env = ApplyEnviromentUpdate(
+      std::move(options.env), std::move(options.env_update));
+
+  if (options.use_path && command.find('/') != std::string::npos &&
+      !env.GetValueOptional("PATH")) {
+    throw std::runtime_error(
+        "execvp potential vulnerability. more details "
+        "https://github.com/userver-framework/userver/issues/588");
+  }
+
   tracing::Span span("ProcessStarter::Exec");
   span.AddTag("command", command);
   Promise<ChildProcess> promise;
@@ -91,8 +126,9 @@ ChildProcess ProcessStarter::Exec(
           return key_value.first + '=' + key_value.second;
         });
     LOG_DEBUG() << fmt::format(
-        "do fork() + execve(), command={}, args=[\'{}\'], env=[]",
-        fmt::join(args, "' '"), fmt::join(keys, ", "));
+        "do fork() + {}(), command={}, args=[\'{}\'], env=[]",
+        options.use_path ? "execv" : "execvp", fmt::join(args, "' '"),
+        fmt::join(keys, ", "));
 
     const auto pid = utils::CheckSyscall(fork(), "fork");
     if (pid) {
@@ -116,15 +152,16 @@ ChildProcess ProcessStarter::Exec(
       // in child thread
       try {
         try {
-          DoExecve(command, args, env, stdout_file, stderr_file);
+          DoExec(command, args, env, options.stdout_file, options.stderr_file,
+                 options.use_path);
         } catch (const std::exception& ex) {
           std::cerr << "Cannot execute child: " << ex.what();
         }
       } catch (...) {
         // must not do anything in a child
         std::abort();
       }
-      // on success execve does not return
+      // on success execve or execvp does not return
       std::abort();
     }
   });
@@ -133,15 +170,24 @@ ChildProcess ProcessStarter::Exec(
   return future.get();
 }
 
+ChildProcess ProcessStarter::Exec(
+    const std::string& command, const std::vector<std::string>& args,
+    const EnvironmentVariables& env,
+    const std::optional<std::string>& stdout_file,
+    const std::optional<std::string>& stderr_file) {
+  ExecOptions options{std::move(env), std::nullopt, std::move(stdout_file),
+                      std::move(stderr_file), false};
+  return Exec(command, args, std::move(options));
+}
+
 ChildProcess ProcessStarter::Exec(
     const std::string& command, const std::vector<std::string>& args,
     EnvironmentVariablesUpdate env_update,
     const std::optional<std::string>& stdout_file,
     const std::optional<std::string>& stderr_file) {
-  return Exec(command, args,
-              EnvironmentVariables{GetCurrentEnvironmentVariables()}.UpdateWith(
-                  std::move(env_update)),
-              stdout_file, stderr_file);
+  ExecOptions options{std::nullopt, std::move(env_update),
+                      std::move(stdout_file), std::move(stderr_file), false};
+  return Exec(command, args, std::move(options));
 }
 
 }  // namespace engine::subprocess