[#1680] check executable flags on ProcessSpawn constructor

src/hooks/dhcp/run_script/run_script.cc

@@ -35,9 +35,6 @@ RunScriptImpl::configure(LibraryHandle& handle) {
     IOServicePtr io_service(new asiolink::IOService());
     try {
         ProcessSpawn process(io_service, name->stringValue());
-        if (!process.checkPermissions()) {
-            isc_throw(InvalidParameter, "The 'name' parameter must point to an executable");
-        }
     } catch (const isc::Exception& ex) {
         isc_throw(InvalidParameter, "Invalid 'name' parameter: " << ex.what());
     }

src/lib/asiolink/process_spawn.cc

@@ -132,12 +132,6 @@ public:
     /// @param pid A process pid.
     void clearState(const pid_t pid);
 
-    /// @brief Check executable permissions.
-    ///
-    /// @return true if file has executable permissions, false otherwise.
-    /// @throw ProcessSpawnError if file does not exist.
-    bool checkPermissions() const;
-
 private:
 
     /// @brief Copies the argument specified as a C++ string to the new
@@ -204,6 +198,16 @@ ProcessSpawnImpl::ProcessSpawnImpl(IOServicePtr io_service,
       io_signal_set_(new IOSignalSet(io_service,
                                      std::bind(&ProcessSpawnImpl::waitForProcess,
                                                ph::_1))) {
+    struct stat st;
+
+    if (stat(executable_.c_str(), &st)) {
+        isc_throw(ProcessSpawnError, "File not found: " << executable_);
+    }
+
+    if (!(st.st_mode & S_IEXEC)) {
+        isc_throw(ProcessSpawnError, "File not executable: " << executable_);
+    }
+
     io_signal_set_->add(SIGCHLD);
 
     // Conversion of the arguments to the C-style array we start by setting
@@ -357,18 +361,6 @@ ProcessSpawnImpl::clearState(const pid_t pid) {
     }
 }
 
-bool
-ProcessSpawnImpl::checkPermissions() const {
-    struct stat st;
-    if (stat(executable_.c_str(), &st)) {
-        isc_throw(ProcessSpawnError, "File not found: " << executable_);
-    }
-    if (st.st_mode & S_IEXEC) {
-        return (true);
-    }
-    return (false);
-}
-
 ProcessSpawn::ProcessSpawn(IOServicePtr io_service,
                            const std::string& executable,
                            const ProcessArgs& args,
@@ -406,10 +398,5 @@ ProcessSpawn::clearState(const pid_t pid) {
     return (impl_->clearState(pid));
 }
 
-bool
-ProcessSpawn::checkPermissions() const {
-    return (impl_->checkPermissions());
-}
-
 } // namespace asiolink
 } // namespace isc

src/lib/asiolink/process_spawn.h

@@ -137,12 +137,6 @@ public:
     /// @param pid A process pid.
     void clearState(const pid_t pid);
 
-    /// @brief Check executable permissions.
-    ///
-    /// @return true if file has executable permissions, false otherwise.
-    /// @throw ProcessSpawnError if file does not exist.
-    bool checkPermissions() const;
-
 private:
 
     /// @brief A smart pointer to the implementation of this class.

src/lib/asiolink/tests/process_spawn_unittest.cc

@@ -261,27 +261,17 @@ TEST_F(ProcessSpawnTest, spawnNoArgs) {
 // This test verifies that the EXIT_FAILURE code is returned when
 // application can't be executed.
 TEST_F(ProcessSpawnTest, invalidExecutable) {
-    ProcessSpawn process(io_service_, "foo");
-    pid_t pid = 0;
-    ASSERT_NO_THROW(pid = process.spawn());
+    std::string expected = "File not found: foo";
+    ASSERT_THROW_MSG(ProcessSpawn process(io_service_, "foo"),
+                     ProcessSpawnError, expected);
 
-    // Set test fail safe.
-    setTestTime(1000);
+    std::string name = TEST_SCRIPT_SH;
+    name += ".in";
 
-    // The next handler executed is IOSignal's handler.
-    io_service_->run_one();
-
-    // The first handler executed is the IOSignal's internal timer expire
-    // callback.
-    io_service_->run_one();
-
-    // Polling once to be sure.
-    io_service_->poll();
-
-    ASSERT_EQ(1, processed_signals_.size());
-    ASSERT_EQ(SIGCHLD, processed_signals_[0]);
-
-    EXPECT_EQ(EXIT_FAILURE, process.getExitStatus(pid));
+    expected = "File not executable: ";
+    expected += name;
+    ASSERT_THROW_MSG(ProcessSpawn process(io_service_, name),
+                     ProcessSpawnError, expected);
 }
 
 // This test verifies that the full command line for the process is
@@ -299,14 +289,16 @@ TEST_F(ProcessSpawnTest, getCommandLine) {
         args.push_back("-y");
         args.push_back("foo");
         args.push_back("bar");
-        ProcessSpawn process(io_service_, "myapp", args);
-        EXPECT_EQ("myapp -x -y foo bar", process.getCommandLine());
+        ProcessSpawn process(io_service_, TEST_SCRIPT_SH, args);
+        std::string expected = TEST_SCRIPT_SH;
+        expected += " -x -y foo bar";
+        EXPECT_EQ(expected, process.getCommandLine());
     }
 
     {
         // Case 2: no arguments.
-        ProcessSpawn process(io_service_, "myapp");
-        EXPECT_EQ("myapp", process.getCommandLine());
+        ProcessSpawn process(io_service_, TEST_SCRIPT_SH);
+        EXPECT_EQ(TEST_SCRIPT_SH, process.getCommandLine());
     }
 }
 
@@ -345,19 +337,4 @@ TEST_F(ProcessSpawnTest, isRunning) {
     ASSERT_EQ(SIGCHLD, processed_signals_[0]);
 }
 
-// This test verifies that the checkPermissions function throws if the file does
-// not exist and returns true or false if the file is or it is not executable.
-TEST_F(ProcessSpawnTest, checkPermissions) {
-    ProcessSpawn no_process(io_service_, "no-file");
-    EXPECT_THROW_MSG(no_process.checkPermissions(), ProcessSpawnError,
-                     "File not found: no-file");
-
-    std::string name = TEST_SCRIPT_SH;
-    name += ".in";
-    ProcessSpawn invalid_process(io_service_, name);
-    ASSERT_FALSE(invalid_process.checkPermissions());
-    ProcessSpawn process(io_service_, TEST_SCRIPT_SH);
-    ASSERT_TRUE(process.checkPermissions());
-}
-
 } // end of anonymous namespace