diff --git a/src/hooks/dhcp/run_script/run_script.cc b/src/hooks/dhcp/run_script/run_script.cc index d9a4eab588..fc4e94aac2 100644 --- a/src/hooks/dhcp/run_script/run_script.cc +++ b/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()); } diff --git a/src/lib/asiolink/process_spawn.cc b/src/lib/asiolink/process_spawn.cc index 247ca510d9..2b819183d3 100644 --- a/src/lib/asiolink/process_spawn.cc +++ b/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 diff --git a/src/lib/asiolink/process_spawn.h b/src/lib/asiolink/process_spawn.h index c810f50bd6..ca72ce8975 100644 --- a/src/lib/asiolink/process_spawn.h +++ b/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. diff --git a/src/lib/asiolink/tests/process_spawn_unittest.cc b/src/lib/asiolink/tests/process_spawn_unittest.cc index b1fe9c0b00..6abeb95b02 100644 --- a/src/lib/asiolink/tests/process_spawn_unittest.cc +++ b/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