diff --git a/AppInfrastructure/Common/include/ConditionVariable.h b/AppInfrastructure/Common/include/ConditionVariable.h index ef9d5b55d..bdadad576 100644 --- a/AppInfrastructure/Common/include/ConditionVariable.h +++ b/AppInfrastructure/Common/include/ConditionVariable.h @@ -181,14 +181,13 @@ class ConditionVariable { return std::cv_status::no_timeout; } - else if (err == ETIMEDOUT) + else if (err == ETIMEDOUT) { return std::cv_status::timeout; } else { __ConditionVariableThrowOnError(err); - return std::cv_status::timeout; } } diff --git a/AppInfrastructure/Common/include/IDGenerator.h b/AppInfrastructure/Common/include/IDGenerator.h index 859ae7baa..28f7c0223 100644 --- a/AppInfrastructure/Common/include/IDGenerator.h +++ b/AppInfrastructure/Common/include/IDGenerator.h @@ -25,6 +25,7 @@ #include #include +#include #include #include @@ -85,10 +86,17 @@ class IDGenerator (N == 19) ? 0x4032F : (N == 20) ? 0x80534 : 0; +private: + static unsigned getRandomSeed() + { + std::random_device rd; + return rd(); + } + public: IDGenerator(unsigned offset = 0) : mOffset(offset) - , mLfsr(1 + (rand() % (mSize- 2))) + , mLfsr(1 + (getRandomSeed() % (mSize - 2))) { } public: diff --git a/AppInfrastructure/Common/source/ThreadedDispatcher.cpp b/AppInfrastructure/Common/source/ThreadedDispatcher.cpp index 2c62e8312..f9011ff83 100644 --- a/AppInfrastructure/Common/source/ThreadedDispatcher.cpp +++ b/AppInfrastructure/Common/source/ThreadedDispatcher.cpp @@ -40,20 +40,18 @@ ThreadedDispatcher::ThreadedDispatcher(int priority, const std::string& name /*= } void ThreadedDispatcher::post(std::function work) { - std::unique_lock lock(m); - if(running) - { + {std::unique_lock lock(m); + if(!running) + { + AI_LOG_WARN("Ignoring work because the dispatcher is not running anymore"); + return; + //can't throw an exception here because if this is executed from destructor, + //which occurs when work adds more work things go horribly wrong. + //Instead, ignore work. + } q.push_back(work); - lock.unlock(); - cv.notify_one(); - } - else - { - AI_LOG_WARN("Ignoring work because the dispatcher is not running anymore"); - //can't throw an exception here because if this is executed from destructor, - //which occurs when work adds more work things go horribly wrong. - //Instead, ignore work. } + cv.notify_one(); } namespace { @@ -71,10 +69,10 @@ namespace */ void syncCallback(std::mutex* lock, std::condition_variable* cond, bool* fired) { - std::unique_lock locker(*lock); - *fired = true; + {std::unique_lock locker(*lock); + *fired = true; + } cond->notify_all(); - locker.unlock(); } } // namespace /** @@ -85,7 +83,7 @@ bool ThreadedDispatcher::invokedFromDispatcherThread() bool res = (std::this_thread::get_id() == t.get_id()); if (res) { - std::stringstream ss; + std::stringstream ss; ss << "Caller thread Id [" << std::this_thread::get_id() << "] == [dispatcher thread Id " << t.get_id() << "]"; AI_LOG_ERROR("%s", ss.str().c_str()); } @@ -109,15 +107,15 @@ void ThreadedDispatcher::sync() std::condition_variable cond; bool fired = false; // Take the queue lock and ensure we're still running - std::unique_lock qlocker(m); - if (!running) - { - AI_LOG_DEBUG("Ignoring sync because dispatcher is not running"); - return; + {std::unique_lock qlocker(m); + if (!running) + { + AI_LOG_DEBUG("Ignoring sync because dispatcher is not running"); + return; + } + // Add the work object to the queue which takes the lock and sets 'fired' to true + q.push_back(std::bind(syncCallback, &lock, &cond, &fired)); } - // Add the work object to the queue which takes the lock and sets 'fired' to true - q.push_back(std::bind(syncCallback, &lock, &cond, &fired)); - qlocker.unlock(); cv.notify_one(); // Wait for 'fired' to become true std::unique_lock locker(lock); @@ -126,6 +124,7 @@ void ThreadedDispatcher::sync() cond.wait(locker); } } + namespace { void unlockAndSetFlagToFalse(std::mutex& m, bool& flag) @@ -148,6 +147,7 @@ void ThreadedDispatcher::flush() std::mutex m2; m2.lock(); post(bind(unlockAndSetFlagToFalse, std::ref(m2), std::ref(this->running))); + // coverity[double_lock : FALSE] m2.lock(); m2.unlock(); stop(); @@ -157,14 +157,15 @@ void ThreadedDispatcher::flush() AI_LOG_WARN("This dispatcher is no longer running. Ignoring flush request."); } } + /** * @brief Cancels any work that is not already in progress, stop accepting new work */ void ThreadedDispatcher::stop() { - std::unique_lock lock(m); - running = false; - lock.unlock(); + {std::unique_lock lock(m); + running = false; + } cv.notify_one(); t.join(); } @@ -214,4 +215,4 @@ std::function ThreadedDispatcher::next() q.pop_front(); return work; } -} //AICommon \ No newline at end of file +} //AICommon diff --git a/AppInfrastructure/Common/source/Timer.cpp b/AppInfrastructure/Common/source/Timer.cpp index 9fcc8bb73..faff2afad 100644 --- a/AppInfrastructure/Common/source/Timer.cpp +++ b/AppInfrastructure/Common/source/Timer.cpp @@ -32,7 +32,10 @@ using namespace AICommon; Timer::~Timer() { - cancel(); + try { + cancel(); + } catch (const std::exception& e) { + } } /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/AppInfrastructure/IpcService/source/sdbus/SDBusIpcService.cpp b/AppInfrastructure/IpcService/source/sdbus/SDBusIpcService.cpp index 237597967..e80ee8313 100644 --- a/AppInfrastructure/IpcService/source/sdbus/SDBusIpcService.cpp +++ b/AppInfrastructure/IpcService/source/sdbus/SDBusIpcService.cpp @@ -180,9 +180,9 @@ SDBusIpcService::~SDBusIpcService() // invoke the quit lambda if (!runOnEventLoopThread(std::move(quitExec))) - { - AI_LOG_ERROR("Failed to post quitExec to event loop thread"); - } + { + AI_LOG_ERROR("Failed to post quitExec to event loop thread"); + } // wait for the thread to quit @@ -223,7 +223,7 @@ bool SDBusIpcService::init(const std::string &serviceName, if (defaultTimeoutMs <= 0) mDefaultTimeoutUsecs = (25 * 1000 * 1000); else - mDefaultTimeoutUsecs = (defaultTimeoutMs * 1000); + mDefaultTimeoutUsecs = (static_cast(defaultTimeoutMs) * 1000); // eventfd used to wake the poll loop @@ -344,8 +344,8 @@ bool SDBusIpcService::stop() if (!runOnEventLoopThread(std::move(nopExec))) { - AI_LOG_ERROR("Failed to queue noop event on event loop thread"); - } + AI_LOG_ERROR("Failed to queue noop event on event loop thread"); + } return true; @@ -426,7 +426,7 @@ std::shared_ptr SDBusIpcService::invokeMethod(const Method &m if (timeoutMs < 0) timeoutUsecs = mDefaultTimeoutUsecs; else - timeoutUsecs = (timeoutMs * 1000); + timeoutUsecs = (static_cast(timeoutMs) * 1000); // create the reply getter std::shared_ptr replyGetter = @@ -504,7 +504,7 @@ bool SDBusIpcService::invokeMethod(const Method &method, if (timeoutMs < 0) timeoutUsecs = mDefaultTimeoutUsecs; else - timeoutUsecs = (timeoutMs * 1000); + timeoutUsecs = (static_cast(timeoutMs) * 1000); // clear the reply args list replyArgs.clear(); diff --git a/AppInfrastructure/Public/Common/Notifier.h b/AppInfrastructure/Public/Common/Notifier.h index 7eae1d512..c81c36caf 100644 --- a/AppInfrastructure/Public/Common/Notifier.h +++ b/AppInfrastructure/Public/Common/Notifier.h @@ -185,6 +185,7 @@ class Notifier : virtual public Polymorphic { // We are unregistering an observer so make sure we will not notify unregistered observers lock.unlock(); + /* coverity[missing_lock : FALSE] */ dispatcher->sync(); lock.lock(); } @@ -195,7 +196,6 @@ class Notifier : virtual public Polymorphic { cv.notify_all(); } - lock.unlock(); } protected: diff --git a/AppInfrastructure/ReadLine/source/ReadLine.cpp b/AppInfrastructure/ReadLine/source/ReadLine.cpp index 084d5741d..7feb6d9f6 100644 --- a/AppInfrastructure/ReadLine/source/ReadLine.cpp +++ b/AppInfrastructure/ReadLine/source/ReadLine.cpp @@ -438,7 +438,7 @@ void ReadLine::commandExecute(const std::string& cmdStr, } else if (!errStr.empty()) { - fprintf(stderr, "Ambiguous command '\%s', possible commands: %s %s\n", + fprintf(stderr, "Ambiguous command '%s', possible commands: %s %s\n", cmdStr.c_str(), cmdRef->name.c_str(), errStr.c_str()); } else if (cmdRef->handler != nullptr) diff --git a/bundle/lib/include/DobbyBundleConfig.h b/bundle/lib/include/DobbyBundleConfig.h index d751b881c..de053e0fc 100644 --- a/bundle/lib/include/DobbyBundleConfig.h +++ b/bundle/lib/include/DobbyBundleConfig.h @@ -79,7 +79,7 @@ class DobbyBundleConfig : public DobbyConfig std::shared_ptr config() const override; public: - const std::map& rdkPlugins() const override; + const std::map& rdkPlugins() const override; #if defined(LEGACY_COMPONENTS) public: diff --git a/bundle/lib/source/DobbyBundleConfig.cpp b/bundle/lib/source/DobbyBundleConfig.cpp index ea6662cd3..8516298ae 100644 --- a/bundle/lib/source/DobbyBundleConfig.cpp +++ b/bundle/lib/source/DobbyBundleConfig.cpp @@ -81,9 +81,9 @@ DobbyBundleConfig::DobbyBundleConfig(const std::shared_ptr& utils, std::string postInstallPath = bundlePath + "/postinstallhooksuccess"; if (remove(postInstallPath.c_str()) != 0) - { - AI_LOG_ERROR("Failed to remove postinstallhooksuccess"); - } + { + AI_LOG_ERROR("Failed to remove postinstallhooksuccess"); + } // Retry creation of config @@ -164,11 +164,13 @@ bool DobbyBundleConfig::isValid() const uid_t DobbyBundleConfig::userId() const { + std::lock_guard locker(mLock); return mUserId; } gid_t DobbyBundleConfig::groupId() const { + std::lock_guard locker(mLock); return mGroupId; } @@ -244,6 +246,7 @@ const std::string& DobbyBundleConfig::rootfsPath() const bool DobbyBundleConfig::restartOnCrash() const { + std::lock_guard locker(mLock); return mRestartOnCrash; } diff --git a/bundle/lib/source/DobbyConfig.cpp b/bundle/lib/source/DobbyConfig.cpp index 0fe8441ec..67de32c96 100644 --- a/bundle/lib/source/DobbyConfig.cpp +++ b/bundle/lib/source/DobbyConfig.cpp @@ -320,6 +320,7 @@ bool DobbyConfig::changeProcessArgs(const std::string& command) */ void DobbyConfig::printCommand() const { + std::lock_guard locker(mLock); std::shared_ptr cfg = config(); if (cfg == nullptr) { @@ -557,7 +558,7 @@ bool DobbyConfig::writeConfigJsonImpl(const std::string& filePath) const // set file permissions if (chmod(filePath.c_str(), S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) != 0) { - AI_LOG_SYS_WARN(errno, "Failed to set permissions on config file '%s'", filePath.c_str()); + AI_LOG_SYS_WARN(errno, "Failed to set permissions on config file '%s'", filePath.c_str()); } diff --git a/bundle/lib/source/DobbyRootfs.cpp b/bundle/lib/source/DobbyRootfs.cpp index 4e6820625..3cc873ba7 100644 --- a/bundle/lib/source/DobbyRootfs.cpp +++ b/bundle/lib/source/DobbyRootfs.cpp @@ -142,14 +142,23 @@ DobbyRootfs::DobbyRootfs(const std::shared_ptr& utils, } // check that rootfs exists in bundle - if (access(rootfsDirPath.c_str(), F_OK) == -1) + int dirFd = open(rootfsDirPath.c_str(), O_CLOEXEC | O_DIRECTORY); + if (dirFd == -1) { - AI_LOG_ERROR_EXIT("could not find rootfs at %s", rootfsDirPath.c_str()); - return; + if (errno == ENOENT) + { + AI_LOG_ERROR_EXIT("could not find rootfs at %s", rootfsDirPath.c_str()); + return; + } + else + { + AI_LOG_SYS_ERROR(errno, "failed to open rootfs directory '%s'", rootfsDirPath.c_str()); + return; + } } else { - mDirFd = open(rootfsDirPath.c_str(), O_CLOEXEC | O_DIRECTORY); + mDirFd = dirFd; } // store the complete path diff --git a/bundle/lib/source/DobbySpecConfig.cpp b/bundle/lib/source/DobbySpecConfig.cpp index 326d428a3..5d85f526b 100644 --- a/bundle/lib/source/DobbySpecConfig.cpp +++ b/bundle/lib/source/DobbySpecConfig.cpp @@ -917,9 +917,10 @@ bool DobbySpecConfig::processUserNs(const Json::Value& value, bool DobbySpecConfig::processRtPriority(const Json::Value& value, ctemplate::TemplateDictionary* dictionary) { - int rtPriorityDefault; - int rtPriorityLimit; + int rtPriorityDefault = 0; + int rtPriorityLimit = 0; + std::lock_guard locker(mLock); if (mSpecVersion == SpecVersion::Version1_0) { if (!value.isIntegral()) diff --git a/bundle/lib/source/DobbyTemplate.cpp b/bundle/lib/source/DobbyTemplate.cpp index bfe80ca4c..6146d3b63 100644 --- a/bundle/lib/source/DobbyTemplate.cpp +++ b/bundle/lib/source/DobbyTemplate.cpp @@ -124,9 +124,10 @@ DobbyTemplate* DobbyTemplate::instance() } } + DobbyTemplate* result = mInstance; pthread_rwlock_unlock(&mInstanceLock); - return mInstance; + return result; } // ----------------------------------------------------------------------------- diff --git a/bundle/tool/source/Main.cpp b/bundle/tool/source/Main.cpp index f6f13bc00..3337011aa 100644 --- a/bundle/tool/source/Main.cpp +++ b/bundle/tool/source/Main.cpp @@ -113,7 +113,7 @@ static void parseArgs(const int argc, char **argv) fprintf(stderr, "Warning: Unknown option `-%c'.\n", optopt); else fprintf(stderr, "Warning: Unknown option character `\\x%x'.\n", optopt); - + break; default: exit(EXIT_FAILURE); break; @@ -228,57 +228,62 @@ static bool generateOciBundle(std::shared_ptr settings, */ int main(int argc, char *argv[]) { - printf("Dobby Bundle Generator Tool\n"); - parseArgs(argc, argv); + try { + printf("Dobby Bundle Generator Tool\n"); + parseArgs(argc, argv); - // Set up so we can read commands - auto readLine = IReadLine::create(); - if (!readLine || !readLine->isValid()) - { - AI_LOG_ERROR_EXIT("failed to create ReadLine object"); - exit(EXIT_FAILURE); - } + // Set up so we can read commands + auto readLine = IReadLine::create(); + if (!readLine || !readLine->isValid()) + { + AI_LOG_ERROR_EXIT("failed to create ReadLine object"); + exit(EXIT_FAILURE); + } - // Can't do any work without a file to process or somewhere to put the - // output - if (inputPath.empty()) - { - AI_LOG_ERROR("Must provide a Dobby spec as an input"); - exit(EXIT_FAILURE); - } - else if (access(inputPath.c_str(), R_OK) != 0) - { - AI_LOG_ERROR("Cannot access Dobby spec file %s", inputPath.c_str()); - exit(EXIT_FAILURE); - } + // Can't do any work without a file to process or somewhere to put the + // output + if (inputPath.empty()) + { + AI_LOG_ERROR("Must provide a Dobby spec as an input"); + exit(EXIT_FAILURE); + } + else if (access(inputPath.c_str(), R_OK) != 0) + { + AI_LOG_ERROR("Cannot access Dobby spec file %s", inputPath.c_str()); + exit(EXIT_FAILURE); + } - // We'll create the directory if it's missing later, so no need to check - // if we can read it - if (outputDirectory.empty()) - { - AI_LOG_ERROR("Must provide an output directory"); - exit(EXIT_FAILURE); - } + // We'll create the directory if it's missing later, so no need to check + // if we can read it + if (outputDirectory.empty()) + { + AI_LOG_ERROR("Must provide an output directory"); + exit(EXIT_FAILURE); + } - // Dobby uses a JSON settings file to provide STB-specific settings (e.g GPU) - // Can be left blank (defaylt settings will be used) - else if (!settingsPath.empty() && access(settingsPath.c_str(), R_OK) != 0) - { - AI_LOG_ERROR("Cannot access settings file %s", inputPath.c_str()); - exit(EXIT_FAILURE); - } + // Dobby uses a JSON settings file to provide STB-specific settings (e.g GPU) + // Can be left blank (defaylt settings will be used) + else if (!settingsPath.empty() && access(settingsPath.c_str(), R_OK) != 0) + { + AI_LOG_ERROR("Cannot access settings file %s", inputPath.c_str()); + exit(EXIT_FAILURE); + } - AI_LOG_INFO("Parsing Dobby spec file %s\n", inputPath.c_str()); - AI_LOG_INFO("Generating Bundle in directory: %s\n", outputDirectory.c_str()); + AI_LOG_INFO("Parsing Dobby spec file %s\n", inputPath.c_str()); + AI_LOG_INFO("Generating Bundle in directory: %s\n", outputDirectory.c_str()); - // Get settings from the provided json file - auto settings = readSettings(); - auto utils = std::make_shared(); + // Get settings from the provided json file + auto settings = readSettings(); + auto utils = std::make_shared(); - // Now we can do some actual work - generateOciBundle(settings, utils, inputPath, outputDirectory); + // Now we can do some actual work + generateOciBundle(settings, utils, inputPath, outputDirectory); - // And we're done - AICommon::termLogging(); - return EXIT_SUCCESS; -} \ No newline at end of file + // And we're done + AICommon::termLogging(); + return EXIT_SUCCESS; + } catch (const std::exception& e) { + AI_LOG_ERROR_EXIT("Unhandled exception in main: %s\n", e.what()); + exit(EXIT_FAILURE); + } +} diff --git a/client/lib/source/DobbyProxy.cpp b/client/lib/source/DobbyProxy.cpp index 26e96e14e..a98ce8602 100644 --- a/client/lib/source/DobbyProxy.cpp +++ b/client/lib/source/DobbyProxy.cpp @@ -134,9 +134,9 @@ DobbyProxy::~DobbyProxy() // can now safely stop the dispatcher thread if (mStateChangeThread.joinable()) { - std::unique_lock locker(mStateChangeLock); - mStateChangeQueue.emplace_back(StateChangeEvent::Terminate); - locker.unlock(); + {std::unique_lock locker(mStateChangeLock); + mStateChangeQueue.emplace_back(StateChangeEvent::Terminate); + } mStateChangeCond.notify_all(); mStateChangeThread.join(); diff --git a/client/tool/source/Main.cpp b/client/tool/source/Main.cpp index 1f9439e14..959e6273b 100644 --- a/client/tool/source/Main.cpp +++ b/client/tool/source/Main.cpp @@ -100,6 +100,7 @@ void containerStopCallback(int32_t cd, const std::string &containerId, if (state == IDobbyProxyEvents::ContainerState::Stopped && containerId == *id) { AI_LOG_INFO("Container %s has stopped", containerId.c_str()); + std::lock_guard locker(gLock); promise.set_value(); } } @@ -119,6 +120,7 @@ void containerWaitCallback(int32_t cd, const std::string &containerId, if (state == wp->state && containerId == wp->containerId) { AI_LOG_INFO("Wait complete"); + std::lock_guard locker(gLock); promise.set_value(); } } @@ -311,25 +313,14 @@ static void startCommand(const std::shared_ptr &dobbyProxy, int32_t cd; struct stat statbuf; - if (stat(path.c_str(), &statbuf) < 0) - { - readLine->printLnError("failed to stat '%s' (%d - %s)", - path.c_str(), errno, strerror(errno)); - return; - } - - // check if path points to a directory - if (S_ISDIR(statbuf.st_mode)) + // Replace stat + opendir TOCTOU by trying opendir() first. If opendir succeeds, + // treat as directory. If opendir fails with ENOTDIR, treat as file. If ENOENT, + // report missing path. Other errors are reported as opendir errors. + DIR *d = opendir(path.c_str()); + if (d != nullptr) { // path points to a directory check that the path contains a config file struct dirent *dir; - DIR *d = opendir(path.c_str()); - if (d == nullptr) - { - readLine->printLnError("failed to opendir '%s' (%d - %s)", - path.c_str(), errno, strerror(errno)); - return; - } bool configFound = false; while ((dir = readdir(d)) != nullptr) { @@ -352,37 +343,54 @@ static void startCommand(const std::shared_ptr &dobbyProxy, } else { -#if defined(LEGACY_COMPONENTS) - // Path does not point to a directory, check that the file in path has - // a '.json' filename extension. - if (path.find(".json") == std::string::npos) + int opendirErr = errno; + if (opendirErr == ENOENT) { - readLine->printLnError("please provide the path to a bundle or a " - "valid .json file"); + readLine->printLnError("failed to stat '%s' (%d - %s)", + path.c_str(), opendirErr, strerror(opendirErr)); return; } - - std::ifstream file(path, std::ifstream::binary); - if (!file) + else if (opendirErr == ENOTDIR) { - readLine->printLnError("failed to open '%s'", args[1].c_str()); - return; - } + // Path does not point to a directory; treat as file/spec path +#if defined(LEGACY_COMPONENTS) + // Path does not point to a directory, check that the file in path has + // a '.json' filename extension. + if (path.find(".json") == std::string::npos) + { + readLine->printLnError("please provide the path to a bundle or a " + "valid .json file"); + return; + } + + std::ifstream file(path, std::ifstream::binary); + if (!file) + { + readLine->printLnError("failed to open '%s'", args[1].c_str()); + return; + } - file.seekg(0, std::ifstream::end); - ssize_t length = file.tellg(); - file.seekg(0, std::ifstream::beg); + file.seekg(0, std::ifstream::end); + ssize_t length = file.tellg(); + file.seekg(0, std::ifstream::beg); - char *buffer = new char[length]; - file.read(buffer, length); + char *buffer = new char[length]; + file.read(buffer, length); - std::string jsonSpec(buffer, length); - delete[] buffer; - cd = dobbyProxy->startContainerFromSpec(id, jsonSpec, files, command, displaySocketPath, envVars); + std::string jsonSpec(buffer, length); + delete[] buffer; + cd = dobbyProxy->startContainerFromSpec(id, jsonSpec, files, command, displaySocketPath, envVars); #else - readLine->printLnError("please provide the path to a bundle directory"); - return; + readLine->printLnError("please provide the path to a bundle directory"); + return; #endif // defined(LEGACY_COMPONENTS) + } + else + { + readLine->printLnError("failed to opendir '%s' (%d - %s)", + path.c_str(), opendirErr, strerror(opendirErr)); + return; + } } if (cd < 0) @@ -1552,6 +1560,7 @@ static void parseArgs(const int argc, char **argv) fprintf(stderr, "Warning: Unknown option `-%c'.\n", optopt); else fprintf(stderr, "Warning: Unknown option character `\\x%x'.\n", optopt); + break; default: exit(EXIT_FAILURE); diff --git a/daemon/lib/source/Dobby.cpp b/daemon/lib/source/Dobby.cpp index 9d526124e..414533b82 100644 --- a/daemon/lib/source/Dobby.cpp +++ b/daemon/lib/source/Dobby.cpp @@ -385,10 +385,6 @@ void Dobby::logJournaldPrinter(int level, const char *file, const char *func, case AI_DEBUG_LEVEL_DEBUG: logLevel = "DBG: "; break; - default: - AI_LOG_WARN("Unknown debug level: %d", level); - logLevel = ": "; - break; } sd_journal_send("SYSLOG_IDENTIFIER=DobbyDaemon", @@ -719,7 +715,7 @@ void Dobby::ping(std::shared_ptr replySender) // If running as systemd service then also use this to wag the dog #if defined(RDK) && defined(USE_SYSTEMD) - mWorkQueue->postWork( + if (!mWorkQueue->postWork( [this]() { if (mWatchdogTimerId >= 0) @@ -730,8 +726,10 @@ void Dobby::ping(std::shared_ptr replySender) AI_LOG_SYS_ERROR(-ret, "failed to send watchdog notification"); } } - } - ); + })) + { + AI_LOG_WARN("failed to queue watchdog notification work"); + } #endif AI_LOG_FN_EXIT(); @@ -1213,13 +1211,11 @@ void Dobby::stop(std::shared_ptr replySender) // Fire off the reply if (!replySender->sendReply({ result })) - { - AI_LOG_ERROR("Failed to send reply from stop lambda"); - } - - }; + { + AI_LOG_ERROR("Failed to send reply from stop lambda"); + } - // Queue the work, if successful then we're done + }; // Queue the work, if successful then we're done if (mWorkQueue->postWork(std::move(doStopLambda))) { AI_LOG_FN_EXIT(); @@ -1271,13 +1267,11 @@ void Dobby::pause(std::shared_ptr replySender) // Fire off the reply if (!replySender->sendReply({ result })) - { - AI_LOG_ERROR("Failed to send reply from pause lambda"); - } - - }; + { + AI_LOG_ERROR("Failed to send reply from pause lambda"); + } - // Queue the work, if successful then we're done + }; // Queue the work, if successful then we're done if (mWorkQueue->postWork(std::move(doPauseLambda))) { AI_LOG_FN_EXIT(); @@ -1719,8 +1713,8 @@ void Dobby::exec(std::shared_ptr replySender) auto doExecLambda = [manager = mManager, descriptor, - options = std::move(options), - command = std::move(command), + options = std::move(options), + command = std::move(command), replySender]() { @@ -1785,9 +1779,9 @@ void Dobby::getState(std::shared_ptr replySender) // Fire off the reply if (!replySender->sendReply({ result })) - { - AI_LOG_ERROR("Failed to send reply from getState lambda"); - } + { + AI_LOG_ERROR("Failed to send reply from getState lambda"); + } }; @@ -1874,13 +1868,11 @@ void Dobby::getInfo(std::shared_ptr replySender) // Fire off the reply if (!replySender->sendReply({ result })) - { - AI_LOG_ERROR("Failed to send reply from getState lambda"); - } - - }; + { + AI_LOG_ERROR("Failed to send reply from getState lambda"); + } - // Queue the work, if successful then we're done + }; // Queue the work, if successful then we're done if (mWorkQueue->postWork(std::move(doGetInfoLambda))) { AI_LOG_FN_EXIT(); @@ -1891,7 +1883,7 @@ void Dobby::getInfo(std::shared_ptr replySender) // Fire off an error reply if (!replySender->sendReply({ "" })) { - AI_LOG_ERROR("Failed to send fallback reply from getInfo"); + AI_LOG_ERROR("Failed to send fallback reply from getInfo"); } @@ -1935,13 +1927,11 @@ void Dobby::list(std::shared_ptr replySender) } // Fire off the reply if (!replySender->sendReply({ cds, ids })) - { - AI_LOG_ERROR("Failed to send reply from list lambda"); - } - - }; + { + AI_LOG_ERROR("Failed to send reply from list lambda"); + } - // Queue the work, if successful then we're done + }; // Queue the work, if successful then we're done if (mWorkQueue->postWork(std::move(doListLambda))) { AI_LOG_FN_EXIT(); @@ -1999,8 +1989,10 @@ void Dobby::createBundle(std::shared_ptr replySender) // Try and get container stats bool result = manager->createBundle(id, spec); - // Fire off the reply - replySender->sendReply({ result }); + if (!replySender->sendReply({ result })) + { + AI_LOG_ERROR("failed to send createBundle reply"); + } }; // Queue the work, if successful then we're done @@ -2050,8 +2042,10 @@ void Dobby::getSpec(std::shared_ptr replySender) // Get the container spec std::string spec = manager->specOfContainer(descriptor); - // Fire off the reply - replySender->sendReply({ spec }); + if (!replySender->sendReply({ spec })) + { + AI_LOG_ERROR("failed to send getSpec reply"); + } }; // Queue the work, if successful then we're done @@ -2106,13 +2100,11 @@ void Dobby::getOCIConfig(std::shared_ptr replySender) // Fire off the reply if (!replySender->sendReply({ configJson })) - { - AI_LOG_ERROR("Failed to send reply from getOCIConfig lambda"); - } - - }; + { + AI_LOG_ERROR("Failed to send reply from getOCIConfig lambda"); + } - // Queue the work, if successful then we're done + }; // Queue the work, if successful then we're done if (mWorkQueue->postWork(std::move(doCreateBundleLambda))) { AI_LOG_FN_EXIT(); diff --git a/daemon/lib/source/DobbyContainer.cpp b/daemon/lib/source/DobbyContainer.cpp index c210879e7..3eec0c90f 100644 --- a/daemon/lib/source/DobbyContainer.cpp +++ b/daemon/lib/source/DobbyContainer.cpp @@ -111,6 +111,7 @@ DobbyContainer::DobbyContainer(const std::shared_ptr& _bundle , hasCurseOfDeath(false) , state(State::Starting) , mRestartOnCrash(false) + , mRestartCount(0) { } @@ -127,6 +128,7 @@ DobbyContainer::DobbyContainer(const std::shared_ptr& _bundle , hasCurseOfDeath(false) , state(State::Starting) , mRestartOnCrash(false) + , mRestartCount(0) { } diff --git a/daemon/lib/source/DobbyLogRelay.cpp b/daemon/lib/source/DobbyLogRelay.cpp index a0161cf3c..24c7a2b12 100644 --- a/daemon/lib/source/DobbyLogRelay.cpp +++ b/daemon/lib/source/DobbyLogRelay.cpp @@ -41,6 +41,8 @@ DobbyLogRelay::DobbyLogRelay(const std::string &sourceSocketPath, const std::string &destinationSocketPath) : mSourceSocketPath(sourceSocketPath), mDestinationSocketPath(destinationSocketPath), + mDestinationSocketFd(-1), + mDestinationSocketAddress{}, mBuf{} { AI_LOG_FN_ENTRY(); @@ -64,7 +66,7 @@ DobbyLogRelay::DobbyLogRelay(const std::string &sourceSocketPath, mDestinationSocketAddress = {}; mDestinationSocketAddress.sun_family = AF_UNIX; - strcpy(mDestinationSocketAddress.sun_path, mDestinationSocketPath.c_str()); + strncpy(mDestinationSocketAddress.sun_path, mDestinationSocketPath.c_str(), sizeof(mDestinationSocketAddress.sun_path) - 1); AI_LOG_INFO("Created log relay from %s to %s", mSourceSocketPath.c_str(), mDestinationSocketPath.c_str()); } @@ -196,7 +198,7 @@ int DobbyLogRelay::createDgramSocket(const std::string &path) struct sockaddr_un address = {}; address.sun_family = AF_UNIX; - strcpy(address.sun_path, path.c_str()); + strncpy(address.sun_path, path.c_str(), sizeof(address.sun_path) - 1); if (bind(sockFd, (const struct sockaddr *)&address, sizeof(address)) < 0) { diff --git a/daemon/lib/source/DobbyLogger.cpp b/daemon/lib/source/DobbyLogger.cpp index 4db0ed01c..4302763a7 100644 --- a/daemon/lib/source/DobbyLogger.cpp +++ b/daemon/lib/source/DobbyLogger.cpp @@ -90,30 +90,34 @@ DobbyLogger::DobbyLogger(const std::shared_ptr &settings) DobbyLogger::~DobbyLogger() { - AI_LOG_FN_ENTRY(); + try { + AI_LOG_FN_ENTRY(); - // Container loggers should remove themselves from the poll loop, but it doesn't really - // matter since if this class is being destructed the whole daemon is almost certainly - // shutting down - if (mJournaldRelay) - { - mJournaldRelay->removeFromPollLoop(mPollLoop); - } - if (mSyslogRelay) - { - mSyslogRelay->removeFromPollLoop(mPollLoop); - } - mPollLoop->stop(); + // Container loggers should remove themselves from the poll loop, but it doesn't really + // matter since if this class is being destructed the whole daemon is almost certainly + // shutting down + if (mJournaldRelay) + { + mJournaldRelay->removeFromPollLoop(mPollLoop); + } + if (mSyslogRelay) + { + mSyslogRelay->removeFromPollLoop(mPollLoop); + } + mPollLoop->stop(); - // Close all our open sockets - if (shutdown(mSocketFd, SHUT_RDWR) < 0) - { - AI_LOG_SYS_WARN(errno, "Failed to shutdown socket %s", mSocketPath.c_str()); - } + // Close all our open sockets + if (shutdown(mSocketFd, SHUT_RDWR) < 0) + { + AI_LOG_SYS_WARN(errno, "Failed to shutdown socket %s", mSocketPath.c_str()); + } - closeAndDeleteSocket(mSocketFd, mSocketPath); + closeAndDeleteSocket(mSocketFd, mSocketPath); - AI_LOG_FN_EXIT(); + AI_LOG_FN_EXIT(); + } catch (const std::exception& e) { + AI_LOG_SYS_ERROR(errno, "Caught Exception in ~DobbyLogger: %s", e.what()); + } } /** @@ -142,7 +146,7 @@ int DobbyLogger::createUnixSocket(const std::string path) // Set properties on the socket struct sockaddr_un address = {}; address.sun_family = AF_UNIX; - strcpy(address.sun_path, path.c_str()); + strncpy(address.sun_path, path.c_str(), sizeof(address.sun_path) - 1); // Attempt to bind the socket if (TEMP_FAILURE_RETRY(bind(sockFd, (const struct sockaddr *)&address, sizeof(address))) < 0) diff --git a/daemon/lib/source/DobbyManager.cpp b/daemon/lib/source/DobbyManager.cpp index cec6a5b3a..6a08373fb 100644 --- a/daemon/lib/source/DobbyManager.cpp +++ b/daemon/lib/source/DobbyManager.cpp @@ -482,6 +482,7 @@ void DobbyManager::cleanupContainersShutdown() { AI_LOG_FN_ENTRY(); + std::unique_lock locker(mLock); AI_LOG_INFO("Dobby shutting down - stopping %lu containers", mContainers.size()); auto it = mContainers.begin(); @@ -493,27 +494,42 @@ void DobbyManager::cleanupContainersShutdown() (it->second->state == DobbyContainer::State::Hibernated) || \ (it->second->state == DobbyContainer::State::Awakening)) { - AI_LOG_INFO("Stopping container %s", it->first.c_str()); + ContainerId id = it->first; + int32_t descriptor = it->second->descriptor; + AI_LOG_INFO("Stopping container %s", id.c_str()); + ++it; + locker.unlock(); // By calling the "proper" stop method here, any listening services will be // notified of the container stop event - if (!stopContainer(it->second->descriptor, false)) + bool stopSuccess = stopContainer(descriptor, false); + locker.lock(); + + auto eraseIt = mContainers.find(id); + if (eraseIt == mContainers.end()) + continue; + + if (!stopSuccess) { // As DobbyRunC::killCont already handles problem of masked SIGTERM in // case we failed to stop it means that it tried to SIGKILL too, so // container must be in uninterrable sleep and we cannot do anything // Remove it container from the list (even though it wasn't clean up) // to avoid repeating indefinitely. It will be cleaned on boot-up - it = mContainers.erase(it); - AI_LOG_ERROR("Failed to stop container %s. Will attempt to clean up at daemon restart", it->first.c_str()); + mContainers.erase(eraseIt); + AI_LOG_ERROR("Failed to stop container %s. Will attempt to clean up at daemon restart", id.c_str()); } else { // This would normally be done async by the runc monitor thread, but we're // shutting down so we want to run synchronously - handleContainerTerminate(it->first, it->second, 0); - it = mContainers.erase(it); + handleContainerTerminate(eraseIt->first, eraseIt->second, 0); + mContainers.erase(eraseIt); } } + else + { + ++it; + } } AI_LOG_FN_EXIT(); @@ -1632,13 +1648,13 @@ bool DobbyManager::hibernateContainer(int32_t cd, const std::string& options) } std::thread hibernateThread = - std::thread([=]() + std::thread([id, cd, dest = std::move(dest), compress, this]() { DobbyHibernate::Error ret = DobbyHibernate::Error::ErrorNone; // create a stats object for the container to get list of PIDs std::unique_lock locker(mLock); - DobbyStats stats(it->first, mEnvironment, mUtilities); - Json::Value jsonPids = DobbyStats(it->first, mEnvironment, mUtilities).stats()["pids"]; + DobbyStats stats(id, mEnvironment, mUtilities); + Json::Value jsonPids = DobbyStats(id, mEnvironment, mUtilities).stats()["pids"]; locker.unlock(); for (auto pidIt = jsonPids.begin(); pidIt != jsonPids.end(); ++pidIt) @@ -1656,7 +1672,7 @@ bool DobbyManager::hibernateContainer(int32_t cd, const std::string& options) uint32_t pid = pidIt->asUInt(); ret = DobbyHibernate::HibernateProcess(pid, DobbyHibernate::DFL_TIMEOUTE_MS, - DobbyHibernate::DFL_LOCATOR, std::move(dest), compress); + DobbyHibernate::DFL_LOCATOR, dest, compress); if (ret != DobbyHibernate::Error::ErrorNone) { AI_LOG_WARN("Error hibernating pid: '%d'", pid); @@ -3405,6 +3421,7 @@ bool DobbyManager::invalidContainerCleanupTask() mRunc->killCont(it->first, SIGKILL, true); // Did we actually kill it? Give it some time, then check the status + /* coverity[sleep : FALSE] */ std::this_thread::sleep_for(std::chrono::milliseconds(200)); DobbyRunC::ContainerStatus state = mRunc->state(it->first); diff --git a/daemon/lib/source/DobbyStats.cpp b/daemon/lib/source/DobbyStats.cpp index 9b77e84c2..37d3f72a6 100644 --- a/daemon/lib/source/DobbyStats.cpp +++ b/daemon/lib/source/DobbyStats.cpp @@ -445,7 +445,7 @@ Json::Value DobbyStats::getProcessTree(const ContainerId& id, { if (pid > std::numeric_limits::max() || pid < 0) { - AI_LOG_WARN("Invalid PID found: %ld", pid); + AI_LOG_WARN("Invalid PID found: %lld", static_cast(pid)); continue; } diff --git a/daemon/lib/source/DobbyWorkQueue.cpp b/daemon/lib/source/DobbyWorkQueue.cpp index d897cd45a..0c935ccbc 100644 --- a/daemon/lib/source/DobbyWorkQueue.cpp +++ b/daemon/lib/source/DobbyWorkQueue.cpp @@ -250,6 +250,7 @@ bool DobbyWorkQueue::postWork(WorkFunc &&work) if (std::this_thread::get_id() == mRunningThreadId) { // add to the queue + std::unique_lock locker(mWorkQueueLock); const uint64_t tag = ++mWorkCounter; mWorkQueue.emplace(tag, std::move(work)); } diff --git a/daemon/lib/source/include/DobbyWorkQueue.h b/daemon/lib/source/include/DobbyWorkQueue.h index 607bba0c7..5b53b4c88 100644 --- a/daemon/lib/source/include/DobbyWorkQueue.h +++ b/daemon/lib/source/include/DobbyWorkQueue.h @@ -62,9 +62,9 @@ class DobbyWorkQueue { } }; - uint64_t mWorkCounter; + std::atomic mWorkCounter; - bool mExitRequested; + std::atomic mExitRequested; std::atomic mRunningThreadId; AICommon::Mutex mWorkQueueLock; @@ -73,7 +73,7 @@ class DobbyWorkQueue AICommon::Mutex mWorkCompleteLock; AICommon::ConditionVariable mWorkCompleteCond; - uint64_t mWorkCompleteCounter; + std::atomic mWorkCompleteCounter; }; diff --git a/daemon/process/source/Main.cpp b/daemon/process/source/Main.cpp index 84569e679..3b612197f 100644 --- a/daemon/process/source/Main.cpp +++ b/daemon/process/source/Main.cpp @@ -230,6 +230,7 @@ static void parseArgs(int argc, char **argv) fprintf(stderr, "Warning: Unknown option `-%c'.\n", optopt); else fprintf(stderr, "Warning: Unknown option character `\\x%x'.\n", optopt); + break; default: exit(EXIT_FAILURE); @@ -496,129 +497,137 @@ static std::shared_ptr setupIpcService() */ int main(int argc, char * argv[]) { - int rc = EXIT_SUCCESS; + try { + int rc = EXIT_SUCCESS; - parseArgs(argc, argv); + parseArgs(argc, argv); - // Set our priority if requested - if (gPriority > 0) - { - struct sched_param param; - param.sched_priority = gPriority; - sched_setscheduler(0, SCHED_RR, ¶m); - } + // Set our priority if requested + if (gPriority > 0) + { + struct sched_param param; + param.sched_priority = gPriority; + sched_setscheduler(0, SCHED_RR, ¶m); + } - // Setup the AI logging stuff - unsigned logTargets = Dobby::Console; - if (gUseSyslog) - { - logTargets |= Dobby::SysLog; - } + // Setup the AI logging stuff + unsigned logTargets = Dobby::Console; + if (gUseSyslog) + { + logTargets |= Dobby::SysLog; + } - // Also log to journald on the RDK builds - if (gUseJournald) - { - logTargets |= Dobby::Journald; - } + // Also log to journald on the RDK builds + if (gUseJournald) + { + logTargets |= Dobby::Journald; + } - Dobby::setupLogging(logTargets); - __ai_debug_log_level = gLogLevel; + Dobby::setupLogging(logTargets); + __ai_debug_log_level = gLogLevel; - AI_LOG_MILESTONE("starting Dobby daemon"); + AI_LOG_MILESTONE("starting Dobby daemon"); - // Daemonise ourselves to run in the background - if (gDaemonise) - { - daemonise(); + // Daemonise ourselves to run in the background + if (gDaemonise) + { + daemonise(); - logTargets &= ~Dobby::Console; - Dobby::setupLogging(logTargets); - } - // Shutdown the console if asked to - else if (gNoConsole) - { - closeConsole(); + logTargets &= ~Dobby::Console; + Dobby::setupLogging(logTargets); + } + // Shutdown the console if asked to + else if (gNoConsole) + { + closeConsole(); - logTargets &= ~Dobby::Console; - Dobby::setupLogging(logTargets); - } + logTargets &= ~Dobby::Console; + Dobby::setupLogging(logTargets); + } - // Create object storing Dobby settings - const std::shared_ptr settings = createSettings(); + // Create object storing Dobby settings + const std::shared_ptr settings = createSettings(); - // Setup signals, this MUST be done in the main thread before any other - // threads are spawned - Dobby::configSignals(); + // Setup signals, this MUST be done in the main thread before any other + // threads are spawned + Dobby::configSignals(); - // Initialise tracing on debug builds (warning: this must be done after the - // Dobby::configSignals() call above, because it spawns threads that mess - // with the signal masks) + // Initialise tracing on debug builds (warning: this must be done after the + // Dobby::configSignals() call above, because it spawns threads that mess + // with the signal masks) #if defined(AI_ENABLE_TRACING) - PerfettoTracing::initialise(); + PerfettoTracing::initialise(); #endif - AI_LOG_INFO("starting dbus service"); + AI_LOG_INFO("starting dbus service"); #if defined(USE_SYSTEMD) - AI_LOG_INFO("Dobby built with systemd support - using sd-bus"); + AI_LOG_INFO("Dobby built with systemd support - using sd-bus"); #else - AI_LOG_INFO("Dobby built without systemd support - using libdbus"); + AI_LOG_INFO("Dobby built without systemd support - using libdbus"); #endif - AI_LOG_INFO(" dbus address '%s'", gDbusAddress.c_str()); - AI_LOG_INFO(" service name '%s'", DOBBY_SERVICE); - AI_LOG_INFO(" object name '%s'", DOBBY_OBJECT); + AI_LOG_INFO(" dbus address '%s'", gDbusAddress.c_str()); + AI_LOG_INFO(" service name '%s'", DOBBY_SERVICE); + AI_LOG_INFO(" object name '%s'", DOBBY_OBJECT); - // Create the IPC service and start it, this spawns a thread and runs the dbus - // event loop inside it. - std::shared_ptr ipcService = setupIpcService(); + // Create the IPC service and start it, this spawns a thread and runs the dbus + // event loop inside it. + std::shared_ptr ipcService = setupIpcService(); - if (!ipcService) - { - rc = EXIT_FAILURE; - } - else if (!ipcService->isServiceAvailable(DOBBY_SERVICE)) - { - // Double check we did actually make ourselves available on the bus - AI_LOG_ERROR("IPC Service initialised but service %s is not available on the bus", DOBBY_SERVICE); - rc = EXIT_FAILURE; - } - else - { - // Create the dobby object and hook into the IPC service - Dobby dobby(ipcService->getBusAddress(), ipcService, settings); + if (!ipcService) + { + rc = EXIT_FAILURE; + } + else if (!ipcService->isServiceAvailable(DOBBY_SERVICE)) + { + // Double check we did actually make ourselves available on the bus + AI_LOG_ERROR("IPC Service initialised but service %s is not available on the bus", DOBBY_SERVICE); + rc = EXIT_FAILURE; + } + else + { + // Create the dobby object and hook into the IPC service + Dobby dobby(ipcService->getBusAddress(), ipcService, settings); - // On debug builds try and detect the AI dbus addresses at startup + // On debug builds try and detect the AI dbus addresses at startup #if (AI_BUILD_TYPE == AI_DEBUG) - dobby.setDefaultAIDbusAddresses(getAIDbusAddress(true), + dobby.setDefaultAIDbusAddresses(getAIDbusAddress(true), getAIDbusAddress(false)); #endif // (AI_BUILD_TYPE == AI_DEBUG) - // Start the service, this spawns a thread and runs the dbus event - // loop inside it - ipcService->start(); + // Start the service, this spawns a thread and runs the dbus event + // loop inside it + ipcService->start(); - // Milestone - AI_LOG_MILESTONE("started Dobby daemon"); + // Milestone + AI_LOG_MILESTONE("started Dobby daemon"); - // Wait till the Dobby service is terminated, this is obviously a - // blocking call - dobby.run(); + // Wait till the Dobby service is terminated, this is obviously a + // blocking call + dobby.run(); - // Stop the service and fall out - ipcService->stop(); - } + // Stop the service and fall out + ipcService->stop(); + } - // Milestone - if (rc == EXIT_SUCCESS) - { - AI_LOG_MILESTONE("stopped Dobby daemon"); - } + // Milestone + if (rc == EXIT_SUCCESS) + { + AI_LOG_MILESTONE("stopped Dobby daemon"); + } - // And we're done - AICommon::termLogging(); - return rc; + // And we're done + AICommon::termLogging(); + return rc; + } catch (const std::exception& e) { + AI_LOG_FATAL("Unhandled exception in main: %s", e.what()); + return EXIT_FAILURE; + } catch (...) { + AI_LOG_FATAL("Unhandled unknown exception in main"); + return EXIT_FAILURE; + } } diff --git a/ipcUtils/source/DobbyIpcBus.cpp b/ipcUtils/source/DobbyIpcBus.cpp index 093a3ec4b..4ba9fae0b 100644 --- a/ipcUtils/source/DobbyIpcBus.cpp +++ b/ipcUtils/source/DobbyIpcBus.cpp @@ -61,9 +61,9 @@ DobbyIpcBus::~DobbyIpcBus() // set the terminate flag and wake the thread if (mServiceChangeThread.joinable()) { - std::unique_lock locker(mLock); - mServiceChangeQueue.emplace_back(ServiceChangeEvent::Terminate); - locker.unlock(); + {std::unique_lock locker(mLock); + mServiceChangeQueue.emplace_back(ServiceChangeEvent::Terminate); + } mServiceChangeCond.notify_all(); mServiceChangeThread.join(); diff --git a/pluginLauncher/lib/include/DobbyRdkPluginUtils.h b/pluginLauncher/lib/include/DobbyRdkPluginUtils.h index e1fdb54de..226fb759f 100644 --- a/pluginLauncher/lib/include/DobbyRdkPluginUtils.h +++ b/pluginLauncher/lib/include/DobbyRdkPluginUtils.h @@ -160,7 +160,11 @@ class DobbyRdkPluginUtils bool addAnnotation(const std::string &key, const std::string &value); bool removeAnnotation(const std::string &key); - std::map getAnnotations() const { return mAnnotations; }; + std::map getAnnotations() const + { + std::lock_guard locker(mLock); + return mAnnotations; + } int exitStatus; diff --git a/pluginLauncher/lib/source/DobbyRdkPluginManager.cpp b/pluginLauncher/lib/source/DobbyRdkPluginManager.cpp index 2014b81d8..c9bbd727c 100644 --- a/pluginLauncher/lib/source/DobbyRdkPluginManager.cpp +++ b/pluginLauncher/lib/source/DobbyRdkPluginManager.cpp @@ -170,10 +170,17 @@ bool DobbyRdkPluginManager::loadPlugins() } int directoriesCount = 0; - struct dirent **namelist; + struct dirent **namelist = nullptr; // Need to sort directories with versionsort so lib.12 would be greater than lib.2 directoriesCount = scandir(mPluginPath.c_str(), &namelist, 0, versionsort); + if (directoriesCount < 0) + { + AI_LOG_SYS_ERROR(errno, "failed to scan directory '%s'", mPluginPath.c_str()); + closedir(dir); + return false; + } + for(int i=0; i &cfg, const std::string& containerId) - : mConf(cfg) + : exitStatus(-1) + , mConf(cfg) , mContainerId(containerId) { AI_LOG_FN_ENTRY(); @@ -50,7 +51,8 @@ DobbyRdkPluginUtils::DobbyRdkPluginUtils(const std::shared_ptr DobbyRdkPluginUtils::DobbyRdkPluginUtils(const std::shared_ptr &cfg, const std::shared_ptr &startState, const std::string& containerId) - : mConf(cfg) + : exitStatus(-1) + , mConf(cfg) , mStartState(startState) , mContainerId(containerId) { @@ -62,7 +64,8 @@ DobbyRdkPluginUtils::DobbyRdkPluginUtils(const std::shared_ptr DobbyRdkPluginUtils::DobbyRdkPluginUtils(const std::shared_ptr &cfg, const std::shared_ptr &state, const std::string& containerId) - : mConf(cfg) + : exitStatus(-1) + , mConf(cfg) , mState(state) , mContainerId(containerId) { @@ -75,7 +78,8 @@ DobbyRdkPluginUtils::DobbyRdkPluginUtils(const std::shared_ptr const std::shared_ptr &state, const std::shared_ptr &startState, const std::string& containerId) - : mConf(cfg) + : exitStatus(-1) + , mConf(cfg) , mState(state) , mStartState(startState) , mContainerId(containerId) diff --git a/pluginLauncher/tool/source/Main.cpp b/pluginLauncher/tool/source/Main.cpp index 4e0530245..d4d9d26d2 100644 --- a/pluginLauncher/tool/source/Main.cpp +++ b/pluginLauncher/tool/source/Main.cpp @@ -119,6 +119,7 @@ static void parseArgs(const int argc, char **argv) fprintf(stderr, "Warning: Unknown option `-%c'.\n", optopt); else fprintf(stderr, "Warning: Unknown option character `\\x%x'.\n", optopt); + break; default: exit(EXIT_FAILURE); break; diff --git a/plugins/Common/source/ServiceMonitor.cpp b/plugins/Common/source/ServiceMonitor.cpp index cba4a5112..52c1f6884 100644 --- a/plugins/Common/source/ServiceMonitor.cpp +++ b/plugins/Common/source/ServiceMonitor.cpp @@ -148,32 +148,36 @@ void ServiceMonitor::onServiceNotification(bool added) { AI_LOG_INFO("%s service %s", mServiceName.c_str(), added ? "added" : "removed"); - std::unique_lock locker(mLock); + State newState = State::NotRunning; + std::function handler; - State newState = mState; - - if (added) - { - // move our internal state to running, this is not the same as 'ready' - // which we expect to receive shortly - if (mState == State::NotRunning) - newState = State::Running; - } - else - { - newState = State::NotRunning; - } - - // call the registered handler - if (newState != mState) { - mState = newState; - - locker.unlock(); + std::unique_lock locker(mLock); + + newState = mState; + + if (added) + { + // move our internal state to running, this is not the same as 'ready' + // which we expect to receive shortly + if (mState == State::NotRunning) + newState = State::Running; + } + else + { + newState = State::NotRunning; + } - if (mStateChangeHandler) - mStateChangeHandler(newState); + // call the registered handler + if (newState != mState) + { + mState = newState; + handler = mStateChangeHandler; + } } + + if (handler) + handler(newState); } // ----------------------------------------------------------------------------- @@ -190,20 +194,21 @@ void ServiceMonitor::onServiceNotification(bool added) void ServiceMonitor::onReadyNotification(const AI_IPC::VariantList& args) { AI_LOG_INFO("%s service is ready", mServiceName.c_str()); + bool notify = false; - std::unique_lock locker(mLock); + {std::unique_lock locker(mLock); - if (mState != State::Ready) - { - // set the state back to ready - mState = State::Ready; - - locker.unlock(); - - // call the registered handler - if (mStateChangeHandler) - mStateChangeHandler(State::Ready); + if (mState != State::Ready) + { + // set the state back to ready + mState = State::Ready; + notify = true; + } } + + // call the registered handler + if (notify && mStateChangeHandler) + mStateChangeHandler(State::Ready); } // ----------------------------------------------------------------------------- @@ -216,15 +221,17 @@ void ServiceMonitor::onReadyNotification(const AI_IPC::VariantList& args) bool ServiceMonitor::onTimer() { // take the lock and check if we think the daemon isn't there - std::unique_lock locker(mLock); - - if (mState != State::Ready) { - locker.unlock(); + std::unique_lock locker(mLock); - sendIsReadyRequest(); + if (mState == State::Ready) + { + return true; + } } + sendIsReadyRequest(); + return true; } diff --git a/plugins/EthanLog/client/cat/ethanlog-cat.cpp b/plugins/EthanLog/client/cat/ethanlog-cat.cpp index 242176e0a..365225d17 100644 --- a/plugins/EthanLog/client/cat/ethanlog-cat.cpp +++ b/plugins/EthanLog/client/cat/ethanlog-cat.cpp @@ -114,7 +114,7 @@ class PipeInput { // read as much to fill in the buffer ssize_t rc = TEMP_FAILURE_RETRY(read(mFd, mBuffer + mBufferOffset, sizeof(mBuffer) - mBufferOffset)); - if (rc <= 0) + if ((rc <= 0) || ((size_t)rc > sizeof(mBuffer) - mBufferOffset)) { mValid = false; return; @@ -214,7 +214,7 @@ class PipeInput bool mValid; char mBuffer[1024]; - int mBufferOffset; + size_t mBufferOffset; }; @@ -361,7 +361,7 @@ static void parseArgs(int argc, char **argv) fprintf(stderr, "Warning: Unknown option `-%c'.\n", optopt); else fprintf(stderr, "Warning: Unknown option character `\\x%x'.\n", optopt); - + break; default: exit(EXIT_FAILURE); } diff --git a/plugins/EthanLog/source/EthanLogClient.cpp b/plugins/EthanLog/source/EthanLogClient.cpp index 7798f8d32..57549939d 100644 --- a/plugins/EthanLog/source/EthanLogClient.cpp +++ b/plugins/EthanLog/source/EthanLogClient.cpp @@ -495,6 +495,11 @@ void EthanLogClient::processLogData() // skip empty fields if (fieldLen > 0) { + if (numFields < 0 || numFields >= maxFields) + { + ret = -1; + break; + } switch (*thisField++) { case 'L': @@ -567,7 +572,8 @@ void EthanLogClient::processLogData() if (ret < 0) break; - numFields += ret; + if (ret > 0 && numFields + ret <= maxFields && numFields + ret > 0) + numFields += ret; } thisField = nextField; diff --git a/plugins/EthanLog/source/EthanLogLoop.cpp b/plugins/EthanLog/source/EthanLogLoop.cpp index e2b340bd6..5e36047a2 100644 --- a/plugins/EthanLog/source/EthanLogLoop.cpp +++ b/plugins/EthanLog/source/EthanLogLoop.cpp @@ -333,7 +333,10 @@ void EthanLogLoop::eventLoop() sd_event_loop(loop); // clear all the clients - mClients.clear(); + { + std::lock_guard locker(mLock); + mClients.clear(); + } // free the event loop sd_event_unref(loop); diff --git a/plugins/MulticastSockets/source/MulticastSocketsPlugin.cpp b/plugins/MulticastSockets/source/MulticastSocketsPlugin.cpp index 07464a0fe..548afa223 100644 --- a/plugins/MulticastSockets/source/MulticastSocketsPlugin.cpp +++ b/plugins/MulticastSockets/source/MulticastSocketsPlugin.cpp @@ -106,17 +106,23 @@ bool MulticastSocketPlugin::postConstruction(const ContainerId &id, for (const MulticastSocket &serverSocket : serverSockets) { int socket = createServerSocket(serverSocket.ipAddress, serverSocket.portNumber); + if (socket < 0) + { + AI_LOG_ERROR("Failed to create server socket for container %s", id.c_str()); + return false; + } + int duppedSocket = startupState->addFileDescriptor(mName, socket); close(socket); //close original fd, it's already dupped and stored in startupState - if (duppedSocket == -1) + if (duppedSocket < 0) { AI_LOG_ERROR("Failed to duplicate server socket for container %s", id.c_str()); return false; } char envVar[256]; - snprintf(envVar, sizeof(envVar), "MCAST_SERVER_SOCKET_%s_FD=%u", serverSocket.name.c_str(), duppedSocket); + snprintf(envVar, sizeof(envVar), "MCAST_SERVER_SOCKET_%s_FD=%d", serverSocket.name.c_str(), duppedSocket); if (!startupState->addEnvironmentVariable(envVar)) { AI_LOG_ERROR("Failed to set env variable for container %s", id.c_str()); @@ -127,17 +133,23 @@ bool MulticastSocketPlugin::postConstruction(const ContainerId &id, for (const std::string &clientSocket : clientSockets) { int socket = createClientSocket(); + if (socket < 0) + { + AI_LOG_ERROR("Failed to create client socket for container %s", id.c_str()); + return false; + } + int duppedSocket = startupState->addFileDescriptor(mName, socket); close(socket); //close original fd, it's already dupped and stored in startupState - if (duppedSocket == -1) + if (duppedSocket < 0) { AI_LOG_ERROR("Failed to duplicate server socket for container %s", id.c_str()); return false; } char envVar[256]; - snprintf(envVar, sizeof(envVar), "MCAST_CLIENT_SOCKET_%s_FD=%u", clientSocket.c_str(), duppedSocket); + snprintf(envVar, sizeof(envVar), "MCAST_CLIENT_SOCKET_%s_FD=%d", clientSocket.c_str(), duppedSocket); if (!startupState->addEnvironmentVariable(envVar)) { AI_LOG_ERROR("Failed to set env variable for container %s", id.c_str()); @@ -212,7 +224,7 @@ std::vector MulticastSocketPlugin::parse multicastSocket.portNumber = static_cast(port.asInt()); multicastSocket.name = name.asString(); - socketsVec.push_back(multicastSocket); + socketsVec.push_back(std::move(multicastSocket)); } return socketsVec; diff --git a/plugins/OpenCDM/source/OpenCDMPlugin.cpp b/plugins/OpenCDM/source/OpenCDMPlugin.cpp index 5d9e7069b..62f82912c 100644 --- a/plugins/OpenCDM/source/OpenCDMPlugin.cpp +++ b/plugins/OpenCDM/source/OpenCDMPlugin.cpp @@ -145,21 +145,34 @@ bool OpenCDMPlugin::postConstruction(const ContainerId& id, const std::string ocdmSocketPath("/tmp/ocdm"); // sanity check the socket exists - if it doesn't then we don't mount - if (access(ocdmSocketPath.c_str(), F_OK) != 0) + // (Attempt operations directly and treat ENOENT as "missing socket") + if (chmod(ocdmSocketPath.c_str(), S_IRWXU | S_IRGRP | S_IWGRP) != 0) { - AI_LOG_ERROR("missing '%s' socket, not mounting in container", - ocdmSocketPath.c_str()); + if (errno == ENOENT) + { + AI_LOG_ERROR("missing '%s' socket, not mounting in container", ocdmSocketPath.c_str()); + } + else + { + AI_LOG_SYS_ERROR(errno, "failed to change access on socket"); + // Stricter behavior: do not attempt chown or addMount if chmod failed. + // This prevents proceeding when we couldn't set expected permissions. + } } else { - if (chmod(ocdmSocketPath.c_str(), S_IRWXU | S_IRGRP | S_IWGRP) != 0) - AI_LOG_SYS_ERROR(errno, "failed to change access on socket"); + // chmod succeeded, proceed with chown and mount only if chown also succeeds if (chown(ocdmSocketPath.c_str(), 0, mAppsGroupId) != 0) + { AI_LOG_SYS_ERROR(errno, "failed to change owner off socket"); - - // mount the socket within the container - if (!startupState->addMount(ocdmSocketPath, ocdmSocketPath, "bind", mountFlags)) - AI_LOG_ERROR("failed to add bind mount for '%s'", ocdmSocketPath.c_str()); + // Stricter behavior: do not attempt to mount if chown failed. + } + else + { + // mount the socket within the container + if (!startupState->addMount(ocdmSocketPath, ocdmSocketPath, "bind", mountFlags)) + AI_LOG_ERROR("failed to add bind mount for '%s'", ocdmSocketPath.c_str()); + } } // on newer builds we may also need the /tmp/OCDM directory @@ -200,10 +213,13 @@ bool OpenCDMPlugin::enableTmpOCDMDir(const std::shared_ptr& st return false; } - // can now add to the bind mount list - startupState->addMount(dirPath, dirPath, "bind", - (MS_BIND | MS_NOSUID | MS_NODEV | MS_NOEXEC)); - return true; + if (!startupState->addMount(dirPath, dirPath, "bind", + (MS_BIND | MS_NOSUID | MS_NODEV | MS_NOEXEC))) + { + AI_LOG_ERROR("failed to add bind mount for OCDM directory '%s'", dirPath.c_str()); + return false; + } + return true; } // ----------------------------------------------------------------------------- @@ -242,34 +258,29 @@ bool OpenCDMPlugin::writeFileIfNotExists(const std::string &filePath) const { AI_LOG_FN_ENTRY(); - // if the file already exists, don't bother recreating it - int fileExists = access(filePath.c_str(), R_OK); - - // file doesn't exist, so lets create it - if (fileExists < 0) + // Atomically attempt to create the file; if it exists, do nothing + int fd = open(filePath.c_str(), O_CLOEXEC | O_CREAT | O_EXCL | O_WRONLY, 0660); + if (fd >= 0) { - // create the file if doesn't exist - int fd = open(filePath.c_str(), O_CLOEXEC | O_CREAT | O_WRONLY, 0660); - if (fd < 0) - { - AI_LOG_SYS_ERROR(errno, "failed to create file @ '%s'", filePath.c_str()); - } - else - { - // set permissions to chmod 0660 and owner root:30000 - if (fchmod(fd, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP) != 0) - AI_LOG_SYS_ERROR(errno, "failed to change access on '%s''", filePath.c_str()); - if (fchown(fd, 0, mAppsGroupId) != 0) - AI_LOG_SYS_ERROR(errno, "failed to change owner off '%s''", filePath.c_str()); - if (close(fd) != 0) - AI_LOG_SYS_ERROR(errno, "failed to close file @ '%s'", filePath.c_str()); - } + // Set file permissions and ownership + if (fchmod(fd, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP) != 0) + AI_LOG_SYS_ERROR(errno, "Could not set permissions for '%s'", filePath.c_str()); + if (fchown(fd, 0, mAppsGroupId) != 0) + AI_LOG_SYS_ERROR(errno, "Could not set owner for '%s'", filePath.c_str()); + if (close(fd) != 0) + AI_LOG_SYS_ERROR(errno, "Could not close file '%s'", filePath.c_str()); AI_LOG_FN_EXIT(); return true; } - - AI_LOG_INFO("%s already exists, skipping creation", filePath.c_str()); + else if (errno == EEXIST) + { + AI_LOG_INFO("File '%s' already present, skipping creation", filePath.c_str()); + } + else + { + AI_LOG_SYS_ERROR(errno, "Unable to create file '%s'", filePath.c_str()); + } AI_LOG_FN_EXIT(); return false; diff --git a/rdkPlugins/AppServices/source/AppServicesRdkPlugin.cpp b/rdkPlugins/AppServices/source/AppServicesRdkPlugin.cpp index 9456ba776..dc527593f 100644 --- a/rdkPlugins/AppServices/source/AppServicesRdkPlugin.cpp +++ b/rdkPlugins/AppServices/source/AppServicesRdkPlugin.cpp @@ -32,6 +32,7 @@ AppServicesRdkPlugin::AppServicesRdkPlugin(std::shared_ptr &con mContainerConfig(containerConfig), mUtils(utils), mRootfsPath(rootfsPath), + mPluginConfig(nullptr), mNetfilter(std::make_shared()), mEnableConnLimit(false) { @@ -687,4 +688,4 @@ std::string AppServicesRdkPlugin::createMasqueradeSnatRule(const std::string &ip ipAddress.c_str()); return std::string(buf); -} \ No newline at end of file +} diff --git a/rdkPlugins/HttpProxy/source/HttpProxyPlugin.cpp b/rdkPlugins/HttpProxy/source/HttpProxyPlugin.cpp index 4151b5b68..2fa5a5de9 100644 --- a/rdkPlugins/HttpProxy/source/HttpProxyPlugin.cpp +++ b/rdkPlugins/HttpProxy/source/HttpProxyPlugin.cpp @@ -34,6 +34,7 @@ HttpProxyPlugin::HttpProxyPlugin(std::shared_ptr &cfg, const std::string &rootfsPath) : mName("HttpProxy"), mContainerConfig(cfg), + mPluginData(nullptr), mUtils(utils), mMountedCACertsPath(rootfsPath + "../ca-certificates.crt") { @@ -350,4 +351,4 @@ bool HttpProxyPlugin::cleanup() AI_LOG_FN_EXIT(); return true; -} \ No newline at end of file +} diff --git a/rdkPlugins/IONMemory/source/IonMemoryPlugin.cpp b/rdkPlugins/IONMemory/source/IonMemoryPlugin.cpp index 77d285fc1..225d808d7 100644 --- a/rdkPlugins/IONMemory/source/IonMemoryPlugin.cpp +++ b/rdkPlugins/IONMemory/source/IonMemoryPlugin.cpp @@ -39,7 +39,8 @@ IonMemoryPlugin::IonMemoryPlugin(std::shared_ptr &containerConf : mName("IonMemory"), mContainerConfig(containerConfig), mUtils(utils), - mRootfsPath(rootfsPath) + mRootfsPath(rootfsPath), + mPluginData(nullptr) { AI_LOG_FN_ENTRY(); diff --git a/rdkPlugins/Logging/source/FileSink.cpp b/rdkPlugins/Logging/source/FileSink.cpp index 4beb5fe6b..f9912e83c 100644 --- a/rdkPlugins/Logging/source/FileSink.cpp +++ b/rdkPlugins/Logging/source/FileSink.cpp @@ -41,6 +41,7 @@ FileSink::FileSink(const std::string &containerId, std::shared_ptr &containerConfig) : mContainerConfig(containerConfig), mContainerId(containerId), + mFileSizeLimit(-1), mLimitHit(false), mBuf{} { @@ -264,4 +265,4 @@ int FileSink::openFile(const std::string &pathName) } return openedFd; -} \ No newline at end of file +} diff --git a/rdkPlugins/Minidump/source/AnonymousFile.cpp b/rdkPlugins/Minidump/source/AnonymousFile.cpp index 85d259874..6aa4d4d22 100644 --- a/rdkPlugins/Minidump/source/AnonymousFile.cpp +++ b/rdkPlugins/Minidump/source/AnonymousFile.cpp @@ -130,10 +130,10 @@ bool AnonymousFile::copyContentTo(const std::string& destFile) } long fileSize = getFileSize(fp); - if (!fileSize) + if (fileSize <= 0) { - AI_LOG_DEBUG("Empty file for fd %d", mFd); - fclose(fp); + AI_LOG_DEBUG("Empty or invalid file size %ld for fd %d", fileSize, mFd); + fclose(fp); fp = nullptr; AI_LOG_FN_EXIT(); return true; @@ -143,7 +143,7 @@ bool AnonymousFile::copyContentTo(const std::string& destFile) if (!buffer) { AI_LOG_SYS_ERROR_EXIT(errno, "failed to allocate buffer for reading fd %d", mFd); - fclose(fp); + fclose(fp); fp = nullptr; return false; } @@ -152,7 +152,7 @@ bool AnonymousFile::copyContentTo(const std::string& destFile) if (elementsRead != fileSize) { AI_LOG_ERROR_EXIT("failed to read fd %d correctly", mFd); - fclose(fp); + fclose(fp); fp = nullptr; free(buffer); return false; @@ -164,7 +164,7 @@ bool AnonymousFile::copyContentTo(const std::string& destFile) if (strncmp(buffer, "MDMP", 4) != 0) { AI_LOG_WARN("Incorrect file header for fd %d", mFd); - fclose(fp); + fclose(fp); fp = nullptr; free(buffer); AI_LOG_FN_EXIT(); @@ -175,7 +175,7 @@ bool AnonymousFile::copyContentTo(const std::string& destFile) if (destFd == -1) { AI_LOG_ERROR_EXIT("Cannot open %s", destFile.c_str()); - fclose(fp); + fclose(fp); fp = nullptr; free(buffer); return false; diff --git a/rdkPlugins/Networking/source/IPAllocator.cpp b/rdkPlugins/Networking/source/IPAllocator.cpp index bfc78fbcb..5c24d0f78 100644 --- a/rdkPlugins/Networking/source/IPAllocator.cpp +++ b/rdkPlugins/Networking/source/IPAllocator.cpp @@ -225,29 +225,29 @@ bool IPAllocator::getContainerIpsFromDisk() mAllocatedIps.clear(); // Dir doesn't exist, no containers have run yet - struct stat buf; - if (stat(ADDRESS_FILE_DIR, &buf) != 0) + DIR *dir = opendir(ADDRESS_FILE_DIR); + if (!dir) { - // Create directory we will store IPs in - if (!mUtils->mkdirRecursive(ADDRESS_FILE_DIR, 0644)) + if (errno == ENOENT) + { + // Create directory we will store IPs in + if (!mUtils->mkdirRecursive(ADDRESS_FILE_DIR, 0644)) + { + AI_LOG_ERROR_EXIT("Failed to create dir @ '%s'", ADDRESS_FILE_DIR); + return false; + } + + AI_LOG_FN_EXIT(); + return true; + } + else { - AI_LOG_ERROR_EXIT("Failed to create dir @ '%s'", ADDRESS_FILE_DIR); + AI_LOG_SYS_ERROR_EXIT(errno, "Failed to open directory @ '%s'", ADDRESS_FILE_DIR); return false; } - - AI_LOG_FN_EXIT(); - return true; } // Work out what IPs are currently allocated to what containers - DIR *dir = opendir(ADDRESS_FILE_DIR); - if (!dir) - { - AI_LOG_SYS_ERROR_EXIT(errno, "Failed to open directory @ '%s", ADDRESS_FILE_DIR); - closedir(dir); - return -1; - } - // Each container gets a file in the store directory // Filename = container ID // Contents = ipaddress/veth @@ -312,4 +312,4 @@ std::string IPAllocator::ipAddressToString(const in_addr_t &ipAddress) AI_LOG_DEBUG("Converted IP %u -> %s", ipAddress, str); return std::string(str); -} \ No newline at end of file +} diff --git a/rdkPlugins/Networking/source/NetworkSetup.cpp b/rdkPlugins/Networking/source/NetworkSetup.cpp index baa4cd346..4dcd2263d 100644 --- a/rdkPlugins/Networking/source/NetworkSetup.cpp +++ b/rdkPlugins/Networking/source/NetworkSetup.cpp @@ -190,10 +190,9 @@ std::vector constructBridgeRules(const std::shared_ptrsecond.emplace_front("DobbyInputChain -p ICMPv6 -j ACCEPT"); + appendRuleSet[Netfilter::TableType::Filter].emplace_front("DobbyInputChain -p ICMPv6 -j ACCEPT"); } // add addresses to rules depending on ipVersion @@ -201,27 +200,24 @@ std::vector constructBridgeRules(const std::shared_ptrsecond.emplace_front("POSTROUTING -s %y -d 255.255.255.255/32 ! -o " BRIDGE_NAME " -j RETURN"); - appendNatRules->second.emplace_front("POSTROUTING -s %y -d 224.0.0.0/24 ! -o " BRIDGE_NAME " -j RETURN"); + appendRuleSet[Netfilter::TableType::Nat].emplace_front("POSTROUTING -s %y -d 255.255.255.255/32 ! -o " BRIDGE_NAME " -j RETURN"); + appendRuleSet[Netfilter::TableType::Nat].emplace_front("POSTROUTING -s %y -d 224.0.0.0/24 ! -o " BRIDGE_NAME " -j RETURN"); // reject with "icmp-port-unreachable" if not ACCEPTed by now - Netfilter::RuleSet::iterator appendFilterRules = appendRuleSet.find(Netfilter::TableType::Filter); - appendFilterRules->second.emplace_back("FORWARD -o " BRIDGE_NAME " -j REJECT --reject-with icmp-port-unreachable"); - appendFilterRules->second.emplace_back("FORWARD -i " BRIDGE_NAME " -j REJECT --reject-with icmp-port-unreachable"); + appendRuleSet[Netfilter::TableType::Filter].emplace_back("FORWARD -o " BRIDGE_NAME " -j REJECT --reject-with icmp-port-unreachable"); + appendRuleSet[Netfilter::TableType::Filter].emplace_back("FORWARD -i " BRIDGE_NAME " -j REJECT --reject-with icmp-port-unreachable"); bridgeAddressRange = BRIDGE_ADDRESS_RANGE "/24"; } else if (ipVersion == AF_INET6) { - Netfilter::RuleSet::iterator appendFilterRules = appendRuleSet.find(Netfilter::TableType::Filter); // add DobbyInputChain rule to accept solicited-node multicast requests from containers - appendFilterRules->second.emplace_front("DobbyInputChain -s %y -d ff02::1:ff40:b01/128 -i " BRIDGE_NAME " -j ACCEPT"); + appendRuleSet[Netfilter::TableType::Filter].emplace_front("DobbyInputChain -s %y -d ff02::1:ff40:b01/128 -i " BRIDGE_NAME " -j ACCEPT"); // reject with "icmp6-port-unreachable" if not ACCEPTed by now - appendFilterRules->second.emplace_back("FORWARD -o " BRIDGE_NAME " -j REJECT --reject-with icmp6-port-unreachable"); - appendFilterRules->second.emplace_back("FORWARD -i " BRIDGE_NAME " -j REJECT --reject-with icmp6-port-unreachable"); + appendRuleSet[Netfilter::TableType::Filter].emplace_back("FORWARD -o " BRIDGE_NAME " -j REJECT --reject-with icmp6-port-unreachable"); + appendRuleSet[Netfilter::TableType::Filter].emplace_back("FORWARD -i " BRIDGE_NAME " -j REJECT --reject-with icmp6-port-unreachable"); bridgeAddressRange = BRIDGE_ADDRESS_RANGE_IPV6 "/120"; } diff --git a/rdkPlugins/Networking/source/NetworkingPlugin.cpp b/rdkPlugins/Networking/source/NetworkingPlugin.cpp index 650e3aeb8..0035845d0 100644 --- a/rdkPlugins/Networking/source/NetworkingPlugin.cpp +++ b/rdkPlugins/Networking/source/NetworkingPlugin.cpp @@ -42,6 +42,7 @@ NetworkingPlugin::NetworkingPlugin(std::shared_ptr &cfg, mContainerConfig(cfg), mUtils(utils), mRootfsPath(rootfsPath), + mPluginData(nullptr), mNetfilter(std::make_shared()) { AI_LOG_FN_ENTRY(); diff --git a/rdkPlugins/OOMCrash/source/OOMCrashPlugin.cpp b/rdkPlugins/OOMCrash/source/OOMCrashPlugin.cpp index ee3c1d189..c09f96cca 100644 --- a/rdkPlugins/OOMCrash/source/OOMCrashPlugin.cpp +++ b/rdkPlugins/OOMCrash/source/OOMCrashPlugin.cpp @@ -54,7 +54,7 @@ unsigned OOMCrash::hookHints() const { return ( IDobbyRdkPlugin::HintFlags::PostInstallationFlag | - IDobbyRdkPlugin::HintFlags::PostHaltFlag); + IDobbyRdkPlugin::HintFlags::PostHaltFlag); } /** @@ -131,17 +131,17 @@ bool OOMCrash::postHalt() } std::string crashFile = path + "/oom_crashed_" + mUtils->getContainerId() + ".txt"; - if (stat(crashFile.c_str(), &buffer) == 0) + if (remove(crashFile.c_str()) != 0) { - if (remove(crashFile.c_str()) != 0) + if (errno != ENOENT) { perror("Failed to remove crash file"); AI_LOG_WARN("Could not remove crash file: %s (%d - %s)", crashFile.c_str(), errno, strerror(errno)); } - else - { - AI_LOG_INFO("%s file removed", crashFile.c_str()); - } + } + else + { + AI_LOG_INFO("%s file removed", crashFile.c_str()); } } @@ -258,8 +258,11 @@ void OOMCrash::createFileForOOM() if (errno != ENOENT) AI_LOG_ERROR("failed to open '%s' (%d - %s)", path.c_str(), errno, strerror(errno)); } - AI_LOG_INFO("%s file created",memoryExceedFile.c_str()); - fclose(fp); + else + { + AI_LOG_INFO("%s file created",memoryExceedFile.c_str()); + fclose(fp); + } } else { diff --git a/rdkPlugins/Storage/source/DynamicMountDetails.cpp b/rdkPlugins/Storage/source/DynamicMountDetails.cpp index 84a7bf78f..9d893d5e0 100644 --- a/rdkPlugins/Storage/source/DynamicMountDetails.cpp +++ b/rdkPlugins/Storage/source/DynamicMountDetails.cpp @@ -135,54 +135,60 @@ bool DynamicMountDetails::onCreateContainer() const if (stat(mMountProperties.source.c_str(), &buffer) == 0) { bool isDir = S_ISDIR(buffer.st_mode); - if (stat(targetPath.c_str(), &buffer) != 0) + + std::string dirPath; + // Determine path based on whether target is a directory or file + if (isDir) { - std::string dirPath; - // Determine path based on whether target is a directory or file - if (isDir) - { - dirPath = targetPath; - } + dirPath = targetPath; + } + else + { + // Mounting a file so exclude filename from directory path + std::size_t found = targetPath.find_last_of("/"); + if (found == std::string::npos) + dirPath = "."; else - { - // Mounting a file so exclude filename from directory path - std::size_t found = targetPath.find_last_of("/"); dirPath = targetPath.substr(0, found); - } + } - // Recursively create destination directory structure - if (mUtils->mkdirRecursive(dirPath, 0755) || (errno == EEXIST)) + // Recursively create destination directory structure + bool prepareOk = true; + if (!mUtils->mkdirRecursive(dirPath, 0755) && (errno != EEXIST)) + { + AI_LOG_SYS_ERROR(errno, "failed to create mount destination path '%s' in storage plugin", targetPath.c_str()); + prepareOk = false; + } + + if (prepareOk && !isDir) + { + // If mounting a file, make sure a file with the same name + // exists at the desination path prior to bind mounting. + // Otherwise the bind mount may fail if the destination path + // filesystem is read-only. + // Creating the file first ensures an inode exists for the + // bind mount to target. + int fd = open(targetPath.c_str(), O_CLOEXEC | O_CREAT | O_WRONLY, 0644); + if (fd >= 0) { - if (isDir) - { - success = true; - } - else - { - // If mounting a file, make sure a file with the same name - // exists at the desination path prior to bind mounting. - // Otherwise the bind mount may fail if the destination path - // filesystem is read-only. - // Creating the file first ensures an inode exists for the - // bind mount to target. - int fd = open(targetPath.c_str(), O_RDONLY | O_CREAT, 0644); - if ((fd == 0) || (errno == EEXIST)) - { - close(fd); - success = true; - } - else - { - AI_LOG_SYS_ERROR(errno, "failed to open or create destination '%s'", targetPath.c_str()); - } - } + close(fd); + // file creation succeeded (or opened existing file), continue } else { - AI_LOG_SYS_ERROR(errno, "failed to create mount destination path '%s' in storage plugin", targetPath.c_str()); + AI_LOG_SYS_ERROR(errno, "failed to open or create destination '%s'", targetPath.c_str()); + prepareOk = false; } } - success = addMount(); + + if (prepareOk) + { + success = addMount(); + } + else + { + success = false; + } } else { @@ -207,24 +213,20 @@ bool DynamicMountDetails::onPostStop() const bool success = false; std::string targetPath = mRootfsPath + mMountProperties.destination; - struct stat buffer; - if (stat(targetPath.c_str(), &buffer) == 0) + if (remove(targetPath.c_str()) == 0) { - if (remove(targetPath.c_str()) == 0) - { - success = true; - } - else - { - AI_LOG_SYS_ERROR(errno, "failed to remove dynamic mount '%s' in storage plugin", targetPath.c_str()); - } + success = true; } - else + else if (errno == ENOENT) { success = true; AI_LOG_INFO("Mount '%s' does not exist, dynamic mount skipped", targetPath.c_str()); } + else + { + AI_LOG_SYS_ERROR(errno, "failed to remove dynamic mount '%s' in storage plugin", targetPath.c_str()); + } AI_LOG_FN_EXIT(); return success; diff --git a/rdkPlugins/Storage/source/LoopMountDetails.cpp b/rdkPlugins/Storage/source/LoopMountDetails.cpp index bd59dac02..f92ab3295 100644 --- a/rdkPlugins/Storage/source/LoopMountDetails.cpp +++ b/rdkPlugins/Storage/source/LoopMountDetails.cpp @@ -123,6 +123,7 @@ bool LoopMountDetails::onPreCreate() AI_LOG_SYS_ERROR(errno, "failed to close loop device dir"); return false; } + loopDevFd = -1; // Prevent double close } // Reset the reference count if it isn't 0 (shouldn't happen) diff --git a/rdkPlugins/Storage/source/RefCountFile.cpp b/rdkPlugins/Storage/source/RefCountFile.cpp index 57a1e860a..2c016f5b1 100644 --- a/rdkPlugins/Storage/source/RefCountFile.cpp +++ b/rdkPlugins/Storage/source/RefCountFile.cpp @@ -24,6 +24,7 @@ #include #include #include +#include RefCountFile::RefCountFile(std::string file): mFilePath(std::move(file)), mFd(-1), mOpen(false) { @@ -108,14 +109,32 @@ int RefCountFile::Increment() int ref = Read(); - if (ref >= 0) + if (ref < 0) { - ref = Write(++ref); - AI_LOG_DEBUG("ref count: %d", ref); + AI_LOG_FN_EXIT(); + return ref; + } + + if (ref >= INT_MAX) + { + AI_LOG_ERROR("Reference count overflow at INT_MAX"); + AI_LOG_FN_EXIT(); + return -1; } + int newRef = ref + 1; + + int writtenRef = Write(newRef); + if (writtenRef < 0) + { + AI_LOG_ERROR("Invalid ref count returned from Write(): %d", writtenRef); + AI_LOG_FN_EXIT(); + return -1; + } + + AI_LOG_DEBUG("ref count: %d", writtenRef); AI_LOG_FN_EXIT(); - return ref; + return writtenRef; } // ----------------------------------------------------------------------------- @@ -210,4 +229,4 @@ int RefCountFile::Write(int ref) AI_LOG_FN_EXIT(); return ret; -} \ No newline at end of file +} diff --git a/settings/source/Settings.cpp b/settings/source/Settings.cpp index 96081033b..d88cae521 100644 --- a/settings/source/Settings.cpp +++ b/settings/source/Settings.cpp @@ -232,7 +232,7 @@ Settings::Settings(const Json::Value& settings) else if(pluginName.isObject()) { for (const auto& value : pluginName.getMemberNames()) - { + { mDefaultPlugins.push_back(value); mRdkPluginsData[value] = pluginName[value]; } @@ -820,6 +820,7 @@ std::list Settings::getPathsFromJson(const Json::Value& value) cons { AI_LOG_ERROR("failed to expand settings path string '%s'", value.asCString()); + wordfree(&exp); return std::list(); } diff --git a/utils/include/DobbyUtils.h b/utils/include/DobbyUtils.h index bade81209..18dc65919 100644 --- a/utils/include/DobbyUtils.h +++ b/utils/include/DobbyUtils.h @@ -155,7 +155,7 @@ class DobbyUtils : public virtual IDobbyUtils gid_t getGID(pid_t pid) const override; private: - std::mutex mMetaDataLock; + mutable std::mutex mMetaDataLock; std::map, int> mIntegerMetaData; std::map, std::string> mStringMetaData; diff --git a/utils/source/DobbyUtils.cpp b/utils/source/DobbyUtils.cpp index f874290f8..becbd4e94 100644 --- a/utils/source/DobbyUtils.cpp +++ b/utils/source/DobbyUtils.cpp @@ -1546,6 +1546,7 @@ int DobbyUtils::getIntegerMetaData(const ContainerId &id, const std::string &key, int defaultValue) const { + std::lock_guard locker(mMetaDataLock); auto it = mIntegerMetaData.find(std::make_pair(id, key)); if (it == mIntegerMetaData.end()) return defaultValue; @@ -1574,6 +1575,7 @@ std::string DobbyUtils::getStringMetaData(const ContainerId &id, const std::string &key, const std::string &defaultValue) const { + std::lock_guard locker(mMetaDataLock); auto it = mStringMetaData.find(std::make_pair(id, key)); if (it == mStringMetaData.end()) return defaultValue;