-
Notifications
You must be signed in to change notification settings - Fork 52
RDKEMW-12282: Fix Coverity identified issues - dobby #405
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses a broad set of Coverity findings across Dobby by tightening thread-safety, fixing resource-handling edge cases, and hardening error paths.
Changes:
- Add/adjust mutex/atomic usage to fix data races and uninitialized state warnings.
- Fix resource cleanup and error-path correctness (fd/wordexp/scandir/remove/chmod, etc.).
- Improve defensive checks (bounds/overflow) and a few logging/formatting issues.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/source/DobbyUtils.cpp | Adds locking around metadata getters to avoid data races. |
| utils/include/DobbyUtils.h | Makes metadata mutex mutable to support const getters locking. |
| settings/source/Settings.cpp | Frees wordexp_t on error path to avoid leaks. |
| rdkPlugins/Storage/source/RefCountFile.cpp | Adds overflow/error checks in refcount increment. |
| rdkPlugins/Storage/source/LoopMountDetails.cpp | Prevents double-close by resetting fd after close. |
| rdkPlugins/Storage/source/DynamicMountDetails.cpp | Refactors mount destination preparation and cleanup logic. |
| rdkPlugins/OOMCrash/source/OOMCrashPlugin.cpp | Simplifies crash file removal and fixes file creation logging/close. |
| rdkPlugins/Networking/source/NetworkingPlugin.cpp | Initializes plugin member pointer to nullptr. |
| rdkPlugins/Networking/source/NetworkSetup.cpp | Uses operator[] for ruleset insertion to avoid invalid iterators. |
| rdkPlugins/Networking/source/IPAllocator.cpp | Reworks directory existence/creation logic for IP store. |
| rdkPlugins/Minidump/source/AnonymousFile.cpp | Treats non-positive file size as invalid/empty with clearer logging. |
| rdkPlugins/Logging/source/FileSink.cpp | Initializes file size limit to avoid uninitialized use. |
| rdkPlugins/IONMemory/source/IonMemoryPlugin.cpp | Initializes plugin member pointer to nullptr. |
| rdkPlugins/HttpProxy/source/HttpProxyPlugin.cpp | Initializes plugin member pointer to nullptr. |
| rdkPlugins/AppServices/source/AppServicesRdkPlugin.cpp | Initializes plugin config pointer to nullptr. |
| plugins/OpenCDM/source/OpenCDMPlugin.cpp | Removes TOCTOU patterns; makes mounting conditional on chmod/chown success; checks addMount return. |
| plugins/MulticastSockets/source/MulticastSocketsPlugin.cpp | Adds fd error checks; fixes env formatting; uses move on push_back. |
| plugins/EthanLog/source/EthanLogLoop.cpp | Locks around client list clear for thread-safety. |
| plugins/EthanLog/source/EthanLogClient.cpp | Adds bounds checks for field parsing to prevent overruns. |
| plugins/EthanLog/client/cat/ethanlog-cat.cpp | Hardens buffer offset type/checks and fixes switch fallthrough. |
| plugins/Common/source/ServiceMonitor.cpp | Removes explicit unlocks around callbacks/timer path. |
| pluginLauncher/tool/source/Main.cpp | Fixes switch fallthrough in argument parsing. |
| pluginLauncher/lib/source/DobbyRdkPluginUtils.cpp | Initializes exitStatus to a known value in constructors. |
| pluginLauncher/lib/source/DobbyRdkPluginManager.cpp | Adds scandir failure handling and avoids redundant fd close. |
| pluginLauncher/lib/include/DobbyRdkPluginUtils.h | Makes getAnnotations thread-safe by locking and returning a copy. |
| ipcUtils/source/DobbyIpcBus.cpp | Removes redundant manual unlock before notifying/joining. |
| daemon/process/source/Main.cpp | Wraps main in try/catch; fixes parseArgs switch fallthrough. |
| daemon/lib/source/include/DobbyWorkQueue.h | Converts counters/flags to atomics. |
| daemon/lib/source/DobbyWorkQueue.cpp | Adds locking in postWork path for same-thread enqueue. |
| daemon/lib/source/DobbyStats.cpp | Fixes PID logging format/cast. |
| daemon/lib/source/DobbyManager.cpp | Refactors shutdown cleanup iteration; fixes lambda capture/move behavior in hibernation path; adds Coverity annotation. |
| daemon/lib/source/DobbyLogger.cpp | Wraps destructor in try/catch; replaces unsafe strcpy with strncpy. |
| daemon/lib/source/DobbyLogRelay.cpp | Initializes members; replaces unsafe strcpy with strncpy. |
| daemon/lib/source/DobbyContainer.cpp | Initializes restart count member. |
| daemon/lib/source/Dobby.cpp | Improves error handling on async work/reply paths. |
| client/tool/source/Main.cpp | Adds locking around promise fulfillment; removes TOCTOU by using opendir-first approach; fixes parseArgs fallthrough. |
| client/lib/source/DobbyProxy.cpp | Removes redundant manual unlock before notifying/joining. |
| bundle/tool/source/Main.cpp | Wraps main in try/catch; fixes parseArgs fallthrough. |
| bundle/lib/source/DobbyTemplate.cpp | Fixes instance return after unlock; corrects prettyPrint whitespace stripping behavior. |
| bundle/lib/source/DobbySpecConfig.cpp | Initializes vars and adds lock around spec version use. |
| bundle/lib/source/DobbyRootfs.cpp | Replaces access() check with directory open + error-specific handling. |
| bundle/lib/source/DobbyConfig.cpp | Adds lock around printCommand config access. |
| bundle/lib/source/DobbyBundleConfig.cpp | Adds locks to several getters. |
| AppInfrastructure/ReadLine/source/ReadLine.cpp | Fixes format string typo in error message. |
| AppInfrastructure/Public/Common/Notifier.h | Adds Coverity annotation and removes redundant unlock. |
| AppInfrastructure/IpcService/source/sdbus/SDBusIpcService.cpp | Prevents timeout multiplication overflow via casts. |
| AppInfrastructure/Common/source/Timer.cpp | Adds try/catch around cancel in destructor. |
| AppInfrastructure/Common/source/ThreadedDispatcher.cpp | Removes explicit unlocks; rewrites flush logic and adds locking in predicate. |
| AppInfrastructure/Common/include/IDGenerator.h | Replaces rand() with random_device-based seed for initialization. |
| AppInfrastructure/Common/include/ConditionVariable.h | Removes dead/unreachable return after throw. |
Comments suppressed due to low confidence (1)
plugins/Common/source/ServiceMonitor.cpp:205
- onReadyNotification invokes mStateChangeHandler while holding mLock (explicit unlock removed). This risks deadlock/re-entrancy issues and holds the mutex across user callback code. Capture any needed state under lock, unlock, then invoke the handler.
// call the registered handler
if (mStateChangeHandler)
mStateChangeHandler(State::Ready);
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ContainerId id = it->first; | ||
| int32_t descriptor = it->second->descriptor; | ||
| AI_LOG_INFO("Stopping container %s", id.c_str()); | ||
| ++it; | ||
| locker.unlock(); |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mLock is unlocked while iterating mContainers, but the iterator 'it' (advanced before unlock) is then reused after relocking. Another thread can erase the element that 'it' points to while the lock is released, leaving 'it' dangling and causing undefined behavior. Avoid carrying iterators across unlock; collect container IDs/descriptors to stop first, or restart iteration after relocking.
| try { | ||
| cancel(); | ||
| } catch (const std::exception& e) { | ||
| } |
Copilot
AI
Jan 23, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timer::~Timer() catches std::exception but ignores it (empty catch block), which can hide real failures during destruction. Either remove the try/catch (cancel() doesn't appear to throw here) or at least log the exception so failures aren't silently swallowed.
| try { | |
| cancel(); | |
| } catch (const std::exception& e) { | |
| } | |
| cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
plugins/Common/source/ServiceMonitor.cpp:209
- onReadyNotification() invokes mStateChangeHandler while holding mLock. If the handler calls back into ServiceMonitor (or takes other locks), this can deadlock. Follow the pattern used in onServiceNotification(): capture the handler (and new state) under the lock, then release the lock before invoking it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (errno == EEXIST) | ||
| { | ||
| AI_LOG_INFO("File '%s' already present, skipping creation", filePath.c_str()); | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
writeFileIfNotExists() treats EEXIST as a non-error (logs and continues) but ultimately returns false. For a helper named "writeFileIfNotExists", an existing file is usually a success case; consider returning true on EEXIST so callers can reliably treat the operation as successful whether the file was created or already present.
| } | ||
| bool ThreadedDispatcher::hasMoreWorkOrWasStopRequested() | ||
| { | ||
| std::unique_lock<std::mutex> lock(m); |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ThreadedDispatcher::hasMoreWorkOrWasStopRequested() now takes m, but it is used as the predicate for cv.wait(lock, pred) while the caller already holds the same mutex. This will self-deadlock when the predicate tries to lock m again. Remove the internal locking from the predicate (assume the caller holds m), or provide two helpers: one that requires m held for the condition_variable predicate and another public method that locks when called externally.
| std::unique_lock<std::mutex> lock(m); | |
| // NOTE: This method is used as the predicate for cv.wait(lock, pred), | |
| // which is called while 'm' is already locked. Therefore, this method | |
| // must not attempt to lock 'm' again, and instead assumes the caller | |
| // holds 'm' before calling. |
| @@ -311,25 +313,14 @@ static void startCommand(const std::shared_ptr<IDobbyProxy> &dobbyProxy, | |||
| int32_t cd; | |||
|
|
|||
| struct stat statbuf; | |||
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startCommand() declares struct stat statbuf; but no longer uses it after switching to opendir()-based detection. This will trigger an unused-variable warning (and can fail builds with -Werror). Remove the unused variable.
| struct stat statbuf; |
| 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"]; |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hibernation thread creates a DobbyStats instance (stats) but then immediately constructs a second temporary DobbyStats to fetch pids; stats is unused (warning) and the extra construction is unnecessary. Reuse the existing stats object (e.g., stats.stats()["pids"]) and remove the unused variable.
| Json::Value jsonPids = DobbyStats(id, mEnvironment, mUtilities).stats()["pids"]; | |
| Json::Value jsonPids = stats.stats()["pids"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::lock_guard<std::mutex> locker(mLock); | ||
| return mRdkPlugins; |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as rootfsPath(): returning a const reference after taking an internal lock doesn’t protect callers once the function returns. If concurrent access is expected, return a copy of the map (or provide an API that executes a callback while holding the lock). Otherwise, drop the lock and document immutability.
| std::lock_guard<std::mutex> locker(mLock); | |
| return mRdkPlugins; | |
| // Create a thread-local copy of the plugin map so that callers | |
| // receive a reference to per-thread data that is not modified | |
| // concurrently by other threads. | |
| thread_local std::map<std::string, Json::Value> rdkPluginsCopy; | |
| { | |
| std::lock_guard<std::mutex> locker(mLock); | |
| rdkPluginsCopy = mRdkPlugins; | |
| } | |
| return rdkPluginsCopy; |
| @@ -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; | |||
|
|
|||
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleanupContainersShutdown() unlocks mLock while retaining and later reusing the iterator it (incremented before unlock). If another thread erases the element that it points to while the lock is released, it becomes invalid and the loop has undefined behavior. Prefer collecting container ids/descriptors to stop under the lock (e.g., copy to a vector), then release the lock and stop/cleanup using fresh lookups under the lock rather than keeping iterators across unlock.
| // 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; | ||
| } |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The directory permissions passed to mkdirRecursive are 0644, which lacks the execute bit required to traverse/access directories. This can result in the store directory being unusable (e.g., opendir/readdir failures) even if creation succeeds. Use a directory-appropriate mode such as 0755 (or 0700 if it must be private).
| { | ||
| readLine->printLnError("please provide the path to a bundle or a " | ||
| "valid .json file"); | ||
| readLine->printLnError("failed to stat '%s' (%d - %s)", |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ENOENT branch reports "failed to stat" even though the code now uses opendir() to probe the path. The error message is misleading for users and complicates debugging; please update it to reflect the actual operation (e.g., failed to open directory / path not found).
| readLine->printLnError("failed to stat '%s' (%d - %s)", | |
| readLine->printLnError("failed to open directory '%s' (%d - %s)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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(); | ||
|
|
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cleanupContainersShutdown, the code increments it and then releases mLock while still relying on that iterator for the subsequent loop iterations. While unlocked, other threads (e.g., onChildExit() / cleanup timers) can erase entries in mContainers, which can invalidate the saved iterator and lead to undefined behavior when the loop continues. Consider collecting a list of container IDs/descriptors to stop while holding the lock (then iterate that list without holding the lock), or re-establish the iterator after re-locking using a stable key (e.g., store nextId and use lower_bound).
| 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; |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
writeFileIfNotExists logs that the file already exists when open(...O_EXCL...) fails with EEXIST, but then returns false. For a function named "writeFileIfNotExists", EEXIST should be treated as success (return true) since the post-condition (file exists) is satisfied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
ipcUtils/source/DobbyIpcBus.cpp:69
- In the destructor, the terminate event is pushed while holding mLock, but the service-change queue is protected by mServiceChangeLock in serviceChangeThread(). This introduces a data race on mServiceChangeQueue, and since the mutex is never explicitly unlocked anymore, join() can block indefinitely if the worker thread needs mLock to drain the queue. Push the terminate event under mServiceChangeLock (not mLock), then release the lock before notify_all() and join().
if (mServiceChangeThread.joinable())
{
std::unique_lock<std::mutex> locker(mLock);
mServiceChangeQueue.emplace_back(ServiceChangeEvent::Terminate);
mServiceChangeCond.notify_all();
mServiceChangeThread.join();
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const std::string mFileName; | ||
|
|
||
| bool mValid; | ||
|
|
||
| char mBuffer[1024]; | ||
| int mBufferOffset; | ||
| size_t mBufferOffset; | ||
| }; |
Copilot
AI
Feb 4, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After changing mBufferOffset to size_t, keep related code consistent to avoid signed/unsigned issues (e.g., loops and arithmetic that still use int/ssize_t). Consider updating mBufferOffset usages to size_t throughout and casting read() results safely.
Description
RDKEMW-12282: Fix Coverity identified issues - dobby
If there is a corresponding JIRA ticket, please ensure it is in the title of the PR.
Test Procedure
How to test this PR (if applicable)
Type of Change
Requires Bitbake Recipe changes?
meta-rdk-ext/recipes-containers/dobby/dobby.bb) must be modified to support the changes in this PR (beyond updatingSRC_REV)