enh(ProcessRunner): does not detect launch errors #4482 (#4483)

* enh(ProcessRunner): does not detect launch errors #4482

* enh(File): add absolutePath and existsAnywhere() #4482

* fix windows build and tsan fail

* fix tsan

* fix windows file tests

* comment out some CI env path -related issues

* fix tsan and windows build

* try to fix ci

* ignore ProcessRunner test fail on windows cmake

* enh(File): canExecute throws FileNotFoundException if the file to be executed can't be found in the path.

* Few C++ modernisation changes.

* enh(File): Windows specifics of File::canExecute. Returns false if the file to be executed can't be found using absolutePath.

---------

Co-authored-by: Matej Kenda <matejken@gmail.com>
This commit is contained in:
Aleksandar Fabijanic 2024-07-29 13:16:18 -05:00 committed by GitHub
parent f24547cdcf
commit 3656f069e1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
20 changed files with 361 additions and 37 deletions

View File

@ -555,7 +555,9 @@ jobs:
class CppUnit::TestCaller<class ICMPClientTest>.testBigPing,
class CppUnit::TestCaller<class ICMPSocketTest>.testMTU,
class CppUnit::TestCaller<class HTTPSClientSessionTest>.testProxy,
class CppUnit::TestCaller<class HTTPSStreamFactoryTest>.testProxy
class CppUnit::TestCaller<class HTTPSStreamFactoryTest>.testProxy,
class CppUnit::TestCaller<class FileTest>.testExists,
class CppUnit::TestCaller<class ProcessRunnerTest>.testProcessRunner
steps:
- uses: actions/checkout@v4
- run: cmake -S. -Bcmake-build -DENABLE_NETSSL_WIN=ON -DENABLE_NETSSL=OFF -DENABLE_CRYPTO=OFF -DENABLE_JWT=OFF -DENABLE_DATA=ON -DENABLE_DATA_ODBC=ON -DENABLE_DATA_MYSQL=OFF -DENABLE_DATA_POSTGRESQL=OFF -DENABLE_TESTS=ON

View File

@ -388,6 +388,9 @@ private:
template <typename ValueType>
friend ValueType* AnyCast(Any*);
template <typename ValueType>
friend const ValueType* AnyCast(const Any*);
template <typename ValueType>
friend ValueType* UnsafeAnyCast(Any*);
@ -426,7 +429,9 @@ const ValueType* AnyCast(const Any* operand)
/// const MyType* pTmp = AnyCast<MyType>(pAny).
/// Returns nullptr if the types don't match.
{
return AnyCast<ValueType>(const_cast<Any*>(operand));
return operand && operand->type() == typeid(ValueType)
? &static_cast<const Any::Holder<ValueType>*>(operand->content())->_held
: nullptr;
}

View File

@ -246,6 +246,7 @@ POCO_DECLARE_EXCEPTION(Foundation_API, CreateFileException, FileException)
POCO_DECLARE_EXCEPTION(Foundation_API, OpenFileException, FileException)
POCO_DECLARE_EXCEPTION(Foundation_API, WriteFileException, FileException)
POCO_DECLARE_EXCEPTION(Foundation_API, ReadFileException, FileException)
POCO_DECLARE_EXCEPTION(Foundation_API, ExecuteFileException, FileException)
POCO_DECLARE_EXCEPTION(Foundation_API, FileNotReadyException, FileException)
POCO_DECLARE_EXCEPTION(Foundation_API, DirectoryNotEmptyException, FileException)
POCO_DECLARE_EXCEPTION(Foundation_API, UnknownURISchemeException, RuntimeException)

View File

@ -54,7 +54,7 @@ class Foundation_API File: private FileImpl
/// use the forward slash ("/") as directory separator.
{
public:
typedef FileSizeImpl FileSize;
using FileSize = FileSizeImpl;
enum LinkType
/// Type of link for linkTo().
@ -84,7 +84,7 @@ public:
File(const File& file);
/// Copy constructor.
virtual ~File();
~File() override;
/// Destroys the file.
File& operator = (const File& file);
@ -105,9 +105,24 @@ public:
const std::string& path() const;
/// Returns the path.
std::string absolutePath() const;
/// Returns absolute path.
/// Attempts to find the existing file
/// using curent work directory and the PATH
/// environment variable.
/// If the file doesn't exist, returns empty string.
bool exists() const;
/// Returns true iff the file exists.
bool existsAnywhere() const;
/// If the file path is relative, searches
/// for the file in the current working directory
/// and the environment paths.
/// If the file path is absolute, the
/// functionality is identical to the
/// exists() call.
bool canRead() const;
/// Returns true iff the file is readable.

View File

@ -19,6 +19,7 @@
#include "Poco/Foundation.h"
#include "Poco/Timestamp.h"
namespace Poco {
@ -32,7 +33,7 @@ protected:
OPT_FAIL_ON_OVERWRITE_IMPL = 0x01
};
typedef UInt64 FileSizeImpl;
using FileSizeImpl = UInt64;
FileImpl();
FileImpl(const std::string& path);
@ -40,10 +41,11 @@ protected:
void swapImpl(FileImpl& file);
void setPathImpl(const std::string& path);
const std::string& getPathImpl() const;
std::string getExecutablePathImpl() const;
bool existsImpl() const;
bool canReadImpl() const;
bool canWriteImpl() const;
bool canExecuteImpl() const;
bool canExecuteImpl(const std::string& absolutePath) const;
bool isFileImpl() const;
bool isDirectoryImpl() const;
bool isLinkImpl() const;

View File

@ -19,7 +19,7 @@
#include "Poco/Foundation.h"
#include "Poco/Timestamp.h"
namespace Poco {
@ -32,7 +32,7 @@ protected:
OPT_FAIL_ON_OVERWRITE_IMPL = 0x01
};
typedef UInt64 FileSizeImpl;
using FileSizeImpl = UInt64;
FileImpl();
FileImpl(const std::string& path);
@ -40,10 +40,11 @@ protected:
void swapImpl(FileImpl& file);
void setPathImpl(const std::string& path);
const std::string& getPathImpl() const;
std::string getExecutablePathImpl() const;
bool existsImpl() const;
bool canReadImpl() const;
bool canWriteImpl() const;
bool canExecuteImpl() const;
bool canExecuteImpl(const std::string& absolutePath) const;
bool isFileImpl() const;
bool isDirectoryImpl() const;
bool isLinkImpl() const;

View File

@ -33,7 +33,7 @@ protected:
OPT_FAIL_ON_OVERWRITE_IMPL = 0x01
};
typedef UInt64 FileSizeImpl;
using FileSizeImpl = UInt64;
FileImpl();
FileImpl(const std::string& path);
@ -41,10 +41,11 @@ protected:
void swapImpl(FileImpl& file);
void setPathImpl(const std::string& path);
const std::string& getPathImpl() const;
std::string getExecutablePathImpl() const;
bool existsImpl() const;
bool canReadImpl() const;
bool canWriteImpl() const;
bool canExecuteImpl() const;
bool canExecuteImpl(const std::string& absolutePath) const;
bool isFileImpl() const;
bool isDirectoryImpl() const;
bool isLinkImpl() const;

View File

@ -48,7 +48,7 @@ public:
PATH_GUESS /// Guess the style by examining the path
};
typedef std::vector<std::string> StringVec;
using StringVec = std::vector<std::string>;
Path();
/// Creates an empty relative path.

View File

@ -121,6 +121,8 @@ public:
int runCount() const;
/// Returns the number of times the process has been executed.
const std::string& error() const;
/// Returns the error message.
private:
static const Poco::ProcessHandle::PID INVALID_PID = -1;
@ -141,10 +143,23 @@ private:
/// Process initialization completion is indicated by new pid in
/// the pid file. If pid file is not specified, there is no waiting.
void checkTimeout(const Poco::Stopwatch& sw, const std::string& msg);
void checkError();
/// If timeout is exceeded, throws TimeoutException with `msg`
/// message.
void checkTimeout(const std::string& msg);
/// If timeout is exceeded, throws TimeoutException with `msg`
/// message.
void checkStatus(const std::string& msg, bool tOut = true);
/// If there were andy errors during process start/stop,
/// throws RuntimeException with the error message;
/// otherwise, if tOut is true and timeout is exceeded, throws
/// TimeoutException with `msg` message.
void setError(const std::string& msg);
/// Sets the error message.
Poco::Thread _t;
std::string _cmd;
Args _args;
@ -156,6 +171,9 @@ private:
std::atomic<bool> _started;
std::atomic<int> _rc;
std::atomic<int> _runCount;
Stopwatch _sw;
std::string _error;
mutable Poco::FastMutex _mutex;
};
@ -193,6 +211,20 @@ inline int ProcessRunner::runCount() const
}
inline void ProcessRunner::setError(const std::string& msg)
{
_error = Poco::format("ProcessRunner(%s): %s", cmdLine(), msg);
}
inline const std::string& ProcessRunner::error() const
{
Poco::FastMutex::ScopedLock l(_mutex);
return _error;
}
} // namespace Poco

View File

@ -169,6 +169,7 @@ POCO_IMPLEMENT_EXCEPTION(CreateFileException, FileException, "Cannot create file
POCO_IMPLEMENT_EXCEPTION(OpenFileException, FileException, "Cannot open file")
POCO_IMPLEMENT_EXCEPTION(WriteFileException, FileException, "Cannot write file")
POCO_IMPLEMENT_EXCEPTION(ReadFileException, FileException, "Cannot read file")
POCO_IMPLEMENT_EXCEPTION(ExecuteFileException, FileException, "Cannot execute file")
POCO_IMPLEMENT_EXCEPTION(FileNotReadyException, FileException, "File not ready")
POCO_IMPLEMENT_EXCEPTION(DirectoryNotEmptyException, FileException, "Directory not empty")
POCO_IMPLEMENT_EXCEPTION(UnknownURISchemeException, RuntimeException, "Unknown URI scheme")

View File

@ -15,6 +15,8 @@
#include "Poco/File.h"
#include "Poco/Path.h"
#include "Poco/DirectoryIterator.h"
#include "Poco/Environment.h"
#include "Poco/StringTokenizer.h"
#if defined(POCO_OS_FAMILY_WINDOWS)
@ -95,12 +97,77 @@ void File::swap(File& file) noexcept
}
std::string File::absolutePath() const
{
std::string ret;
if (Path(path()).isAbsolute())
// TODO: Should this return empty string if file does not exists to be consistent
// with the function documentation?
ret = getPathImpl();
else
{
Path curPath(Path::current());
curPath.append(path());
if (File(curPath).exists())
ret = curPath.toString();
else
{
const std::string envPath = Environment::get("PATH", "");
const std::string pathSeparator(1, Path::pathSeparator());
if (!envPath.empty())
{
const StringTokenizer st(envPath, pathSeparator,
StringTokenizer::TOK_IGNORE_EMPTY | StringTokenizer::TOK_TRIM);
for (const auto& p: st)
{
try
{
std::string fileName(p);
if (p.size() && p.back() != Path::separator())
fileName.append(1, Path::separator());
fileName.append(path());
if (File(fileName).exists())
{
ret = fileName;
break;
}
}
catch (const Poco::PathSyntaxException&)
{
// shield against bad PATH environment entries
}
}
}
}
}
return ret;
}
bool File::exists() const
{
if (path().empty()) return false;
return existsImpl();
}
bool File::existsAnywhere() const
{
if (path().empty()) return false;
if (Path(path()).isAbsolute())
return existsImpl();
if (File(absolutePath()).exists())
return true;
return false;
}
bool File::canRead() const
{
return canReadImpl();
@ -115,7 +182,14 @@ bool File::canWrite() const
bool File::canExecute() const
{
return canExecuteImpl();
// Resolve (platform-specific) executable path and absolute path from relative.
const auto execPath { getExecutablePathImpl() };
const auto absPath { File(execPath).absolutePath() };
if (absPath.empty() || !File(absPath).exists())
{
return false;
}
return canExecuteImpl(absPath);
}

View File

@ -16,6 +16,7 @@
#include "Poco/Buffer.h"
#include "Poco/Exception.h"
#include "Poco/Error.h"
#include "Poco/Path.h"
#include <algorithm>
#include <sys/stat.h>
#include <sys/types.h>
@ -80,6 +81,12 @@ void FileImpl::setPathImpl(const std::string& path)
}
std::string FileImpl::getExecutablePathImpl() const
{
return _path;
}
bool FileImpl::existsImpl() const
{
poco_assert (!_path.empty());
@ -127,12 +134,12 @@ bool FileImpl::canWriteImpl() const
}
bool FileImpl::canExecuteImpl() const
bool FileImpl::canExecuteImpl(const std::string& absolutePath) const
{
poco_assert (!_path.empty());
poco_assert (!absolutePath.empty());
struct stat st;
if (stat(_path.c_str(), &st) == 0)
if (stat(absolutePath.c_str(), &st) == 0)
{
if (st.st_uid == geteuid() || geteuid() == 0)
return (st.st_mode & S_IXUSR) != 0;

View File

@ -62,6 +62,12 @@ void FileImpl::setPathImpl(const std::string& path)
}
std::string FileImpl::getExecutablePathImpl() const
{
return _path;
}
bool FileImpl::existsImpl() const
{
poco_assert (!_path.empty());
@ -87,7 +93,7 @@ bool FileImpl::canWriteImpl() const
}
bool FileImpl::canExecuteImpl() const
bool FileImpl::canExecuteImpl(const std::string& absolutePath) const
{
return false;
}

View File

@ -14,6 +14,7 @@
#include "Poco/File_WIN32U.h"
#include "Poco/Exception.h"
#include "Poco/Path.h"
#include "Poco/String.h"
#include "Poco/UnicodeConverter.h"
#include "Poco/UnWindows.h"
@ -88,6 +89,19 @@ void FileImpl::setPathImpl(const std::string& path)
convertPath(_path, _upath);
}
std::string FileImpl::getExecutablePathImpl() const
{
// Windows specific: An executable can be invoked without
// the extension .exe, but the file has it nevertheless.
// This function appends extension "exe" if the file path does not have it.
Path p(_path);
if (!p.getExtension().empty())
{
return _path;
}
return p.setExtension("exe"s).toString();
}
bool FileImpl::existsImpl() const
{
@ -140,9 +154,9 @@ bool FileImpl::canWriteImpl() const
}
bool FileImpl::canExecuteImpl() const
bool FileImpl::canExecuteImpl(const std::string& absolutePath) const
{
Path p(_path);
Path p(absolutePath);
return icompare(p.getExtension(), "exe") == 0;
}

View File

@ -20,6 +20,7 @@
#include "Poco/File.h"
#include "Poco/Path.h"
#include "Poco/String.h"
#include "Poco/Error.h"
#include <fstream>
@ -104,15 +105,52 @@ std::string ProcessRunner::cmdLine() const
void ProcessRunner::run()
{
int errHandle = 0;
int errPID = 0;
int errRC = 0;
{
Poco::FastMutex::ScopedLock l(_mutex);
_error.clear();
}
_pid = INVALID_PID;
_pPH = nullptr;
ProcessHandle* pPH = nullptr;
try
{
_pPH = pPH = new ProcessHandle(Process::launch(_cmd, _args, _options));
errHandle = Error::last();
_pid = pPH->id();
errPID = Error::last();
_rc = pPH->wait();
errRC = Error::last();
if (errHandle || errPID || errRC || _rc != 0)
{
Poco::FastMutex::ScopedLock l(_mutex);
Poco::format(_error, "ProcessRunner::run() error; "
"handle=%d (%d:%s); pid=%d (%d:%s); return=%d (%d:%s)",
(pPH ? pPH->id() : 0), errHandle, Error::getMessage(errHandle),
_pid.load(), errPID, Error::getMessage(errPID),
_rc.load(), errRC, Error::getMessage(errRC));
}
}
catch (Poco::Exception& ex)
{
setError(ex.displayText());
}
catch (std::exception& ex)
{
setError(ex.what());
}
catch (...)
{
setError("Unknown exception"s);
}
_pid = INVALID_PID;
@ -127,7 +165,7 @@ void ProcessRunner::stop()
if (_started)
{
PID pid;
Stopwatch sw; sw.start();
_sw.restart();
if (_pPH.exchange(nullptr) && ((pid = _pid.exchange(INVALID_PID))) != INVALID_PID)
{
while (Process::isRunning(pid))
@ -135,9 +173,9 @@ void ProcessRunner::stop()
if (pid > 0)
{
Process::requestTermination(pid);
checkTimeout(sw, "Waiting for process termination");
checkStatus("Waiting for process termination");
}
else throw Poco::IllegalStateException("Invalid PID, can;t terminate process");
else throw Poco::IllegalStateException("Invalid PID, can't terminate process");
}
_t.join();
}
@ -150,9 +188,8 @@ void ProcessRunner::stop()
_pidFile.clear();
std::string msg;
Poco::format(msg, "Waiting for PID file (pidFile: '%s')", _pidFile);
sw.restart();
while (pidFile.exists())
checkTimeout(sw, msg);
_sw.restart();
while (pidFile.exists()) checkStatus(msg);
}
}
}
@ -160,9 +197,18 @@ void ProcessRunner::stop()
}
void ProcessRunner::checkTimeout(const Stopwatch& sw, const std::string& msg)
void ProcessRunner::checkError()
{
if (sw.elapsedSeconds() > _timeout)
Poco::FastMutex::ScopedLock l(_mutex);
if (!_error.empty())
throw Poco::RuntimeException(_error);
}
void ProcessRunner::checkTimeout(const std::string& msg)
{
if (_sw.elapsedSeconds() > _timeout)
{
throw Poco::TimeoutException(
Poco::format("ProcessRunner::checkTimeout(): %s", msg));
@ -171,31 +217,50 @@ void ProcessRunner::checkTimeout(const Stopwatch& sw, const std::string& msg)
}
void ProcessRunner::checkStatus(const std::string& msg, bool tOut)
{
checkError();
if (tOut) checkTimeout(msg);
}
void ProcessRunner::start()
{
if (!_started.exchange(true))
{
File exe(_cmd);
if (!exe.existsAnywhere())
{
throw Poco::FileNotFoundException(
Poco::format("ProcessRunner::start(%s): command not found", _cmd));
}
else if (!File(exe.absolutePath()).canExecute())
{
throw Poco::ExecuteFileException(
Poco::format("ProcessRunner::start(%s): cannot execute", _cmd));
}
int prevRunCnt = runCount();
_t.start(*this);
std::string msg;
Poco::format(msg, "Waiting for process to start (pidFile: '%s')", _pidFile);
Stopwatch sw; sw.start();
_sw.restart();
// wait for the process to be either running or completed by monitoring run counts.
while (!running() && prevRunCnt >= runCount()) checkTimeout(sw, msg);
while (!running() && prevRunCnt >= runCount()) checkStatus(msg);
// we could wait for the process handle != INVALID_PID,
// but if pidFile name was given, we should wait for
// the process to write it
if (!_pidFile.empty())
{
sw.restart();
_sw.restart();
// wait until process is fully initialized
File pidFile(_pidFile);
while (!pidFile.exists())
checkTimeout(sw, "waiting for PID file");
checkStatus(Poco::format("waiting for PID file '%s' creation.", _pidFile));
// verify that the file content is actually the process PID
FileInputStream fis(_pidFile);
@ -205,7 +270,7 @@ void ProcessRunner::start()
while (fPID != pid())
{
fis.clear(); fis.seekg(0); fis >> fPID;
checkTimeout(sw, Poco::format("waiting for new PID (%s)", _pidFile));
checkStatus(Poco::format("waiting for new PID (%s)", _pidFile));
}
}
}

View File

@ -134,7 +134,7 @@ void AnyTest::testAnyCastToReference()
int& ra = AnyCast<int &>(a);
int const& ra_c = AnyCast<int const &>(a);
// NOTE: The following two AnyCasts will trigger the
// NOTE: The volatile AnyCasts will trigger the
// undefined behavior sanitizer.
int volatile& ra_v = AnyCast<int volatile &>(a);
int const volatile& ra_cv = AnyCast<int const volatile&>(a);

View File

@ -18,7 +18,9 @@
#include "Poco/Thread.h"
#include <fstream>
#include <set>
#if defined(POCO_OS_FAMILY_WINDOWS)
#include <Windows.h>
#endif
using Poco::File;
using Poco::TemporaryFile;
@ -192,6 +194,61 @@ void FileTest::testCreateFile()
}
void FileTest::testExists()
{
assertFalse (File("").exists());
{
File f("testfile.dat");
f.createFile();
assertTrue (f.exists());
assertTrue (f.existsAnywhere());
assertFalse (f.canExecute());
}
{
File f("/testfile.dat");
assertFalse (f.exists());
assertFalse (f.existsAnywhere());
assertFalse (f.canExecute());
}
{
#if defined(POCO_OS_FAMILY_UNIX)
File f("echo");
File f2("/dev/null");
#elif defined(POCO_OS_FAMILY_WINDOWS)
std::string buffer(MAX_PATH, 0);
UINT r = GetSystemDirectoryA(buffer.data(), static_cast<UINT>(buffer.size()));
if (r)
{
Path p(buffer);
p.makeDirectory().makeAbsolute().makeParent();
buffer = p.toString();
buffer.append("win.ini");
}
else
{
buffer = R"(c:\windows\win.ini)";
}
File f("cmd.exe");
File f2(buffer);
File f3("cmd");
assertTrue (f3.canExecute());
File f4("cmd-nonexistent");
assertFalse (f4.canExecute());
#endif
assertFalse (f.exists());
assertTrue (f.existsAnywhere());
assertTrue (f.canExecute());
assertTrue (f2.exists());
assertTrue (f2.existsAnywhere());
assertFalse (f2.canExecute());
}
}
void FileTest::testFileAttributes2()
{
TemporaryFile f;
@ -660,6 +717,7 @@ CppUnit::Test* FileTest::suite()
CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("FileTest");
CppUnit_addTest(pSuite, FileTest, testCreateFile);
CppUnit_addTest(pSuite, FileTest, testExists);
CppUnit_addTest(pSuite, FileTest, testFileAttributes1);
CppUnit_addTest(pSuite, FileTest, testFileAttributes2);
CppUnit_addTest(pSuite, FileTest, testFileAttributes3);

View File

@ -26,6 +26,7 @@ public:
void testFileAttributes1();
void testCreateFile();
void testExists();
void testFileAttributes2();
void testFileAttributes3();
void testCompare();

View File

@ -349,6 +349,9 @@ void PathTest::testParseUnix4()
assertTrue (!p.isDirectory());
assertTrue (p.isFile());
assertTrue (p.toString(Path::PATH_UNIX) == "a/b/c/d");
p.makeDirectory();
assertTrue (p.toString(Path::PATH_UNIX) == "a/b/c/d/");
}

View File

@ -111,11 +111,11 @@ void ProcessRunnerTest::testProcessRunner()
assertTrue (pr.cmdLine() == cmdLine(cmd, args));
assertFalse (pr.running());
pr.start();
Stopwatch sw; sw.start();
while (!pr.running())
checkTimeout(sw, "Waiting for process to start", 1000, __LINE__);
assertTrue (pr.running());
try
{
@ -264,6 +264,42 @@ void ProcessRunnerTest::testProcessRunner()
}
assertTrue (!File(pidFile).exists());
}
// non-existent executable with no PID file created
{
std::string cmd = "nonexistent_123-xyz";
std::vector<std::string> args;
char c = Path::separator();
std::string pidFile = Poco::format("run%c%s.pid", c, name);
{
std::unique_ptr<ProcessRunner> pr;
try
{
pr.reset(new ProcessRunner(cmd, args));
failmsg("ProcessRunner should throw an exception.");
} catch(const Poco::FileException& e) {}
}
assertTrue (!File(pidFile).exists());
}
// non-existent executable with PID file created
{
std::string cmd = "nonexistent_123-xyz";
std::vector<std::string> args;
char c = Path::separator();
std::string pidFile = Poco::format("run%c%s.pid", c, name);
args.push_back(std::string("-p=").append(pidFile));
{
std::unique_ptr<ProcessRunner> pr;
try
{
pr.reset(new ProcessRunner(cmd, args));
failmsg("ProcessRunner should throw an exception.");
} catch(const Poco::FileException& e) {}
}
assertTrue (!File(pidFile).exists());
}
#if defined(POCO_OS_FAMILY_UNIX)
// start process launching multiple threads
{