RDKEMW-12282: Fix Coverity identified issues - dobby#408
RDKEMW-12282: Fix Coverity identified issues - dobby#408dkumar798 wants to merge 37 commits intordkcentral:topic/RDKEMW-13941from
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses Coverity static analysis issues identified in the Dobby container runtime. The changes primarily focus on fixing concurrency issues, resource management problems, TOCTOU (time-of-check-time-of-use) vulnerabilities, buffer overflows, and other code quality issues.
Changes:
- Fixed concurrency and thread safety issues by adding proper mutex locking, using atomic variables where appropriate, and ensuring locks are released before callbacks
- Addressed resource leaks and proper initialization of member variables across multiple plugins
- Replaced TOCTOU-prone patterns (access/stat followed by open/opendir) with direct operations
- Fixed buffer overflow vulnerabilities by using strncpy instead of strcpy and adding bounds checking
- Added exception handling in destructors and main functions to prevent unexpected terminations
- Fixed iterator invalidation issues and improved error handling throughout the codebase
Reviewed changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/source/DobbyUtils.cpp | Added mutex locking for thread-safe access to metadata maps in const methods |
| utils/include/DobbyUtils.h | Made metadata mutex mutable to allow locking in const methods |
| settings/source/Settings.cpp | Fixed resource leak by calling wordfree and corrected indentation |
| rdkPlugins/Storage/source/RefCountFile.cpp | Added overflow checks for reference counter and improved error handling |
| rdkPlugins/Storage/source/LoopMountDetails.cpp | Prevented double close by resetting fd after close |
| rdkPlugins/Storage/source/DynamicMountDetails.cpp | Fixed file creation logic and improved error handling |
| rdkPlugins/OOMCrash/source/OOMCrashPlugin.cpp | Improved file operation error handling and fixed indentation |
| rdkPlugins/Networking/source/NetworkingPlugin.cpp | Initialized mPluginData pointer to nullptr |
| rdkPlugins/Networking/source/NetworkSetup.cpp | Fixed iterator usage to avoid dangling iterator issues |
| rdkPlugins/Networking/source/IPAllocator.cpp | Replaced stat with opendir to fix TOCTOU vulnerability |
| rdkPlugins/Minidump/source/AnonymousFile.cpp | Fixed file size check and corrected indentation |
| rdkPlugins/Logging/source/FileSink.cpp | Initialized mFileSizeLimit member variable |
| rdkPlugins/IONMemory/source/IonMemoryPlugin.cpp | Initialized mPluginData pointer to nullptr |
| rdkPlugins/HttpProxy/source/HttpProxyPlugin.cpp | Initialized mPluginData pointer to nullptr |
| rdkPlugins/AppServices/source/AppServicesRdkPlugin.cpp | Initialized mPluginConfig pointer to nullptr |
| plugins/OpenCDM/source/OpenCDMPlugin.cpp | Replaced access() with direct operations and improved error handling |
| plugins/MulticastSockets/source/MulticastSocketsPlugin.cpp | Added socket validation, fixed format specifier, and used move semantics |
| plugins/EthanLog/source/EthanLogLoop.cpp | Added lock guard for thread-safe access to mClients |
| plugins/EthanLog/source/EthanLogClient.cpp | Added bounds checking to prevent buffer overflow |
| plugins/EthanLog/client/cat/ethanlog-cat.cpp | Fixed buffer overflow check and changed mBufferOffset type to size_t |
| plugins/Common/source/ServiceMonitor.cpp | Improved lock management for state change notifications |
| pluginLauncher/tool/source/Main.cpp | Added missing break statement in switch case |
| pluginLauncher/lib/source/DobbyRdkPluginUtils.cpp | Initialized exitStatus member variable |
| pluginLauncher/lib/source/DobbyRdkPluginManager.cpp | Added scandir error handling and removed incorrect close call |
| pluginLauncher/lib/include/DobbyRdkPluginUtils.h | Added lock guard for thread-safe access to annotations |
| ipcUtils/source/DobbyIpcBus.cpp | Fixed lock guard scoping |
| daemon/process/source/Main.cpp | Added exception handling in main and missing break statement |
| daemon/lib/source/include/DobbyWorkQueue.h | Changed counters to atomic types |
| daemon/lib/source/DobbyWorkQueue.cpp | Added missing lock guard |
| daemon/lib/source/DobbyStats.cpp | Fixed format specifier for long long type |
| daemon/lib/source/DobbyManager.cpp | Fixed iterator invalidation, lambda captures, and added exception annotations |
| daemon/lib/source/DobbyLogger.cpp | Added exception handling in destructor and replaced strcpy with strncpy |
| daemon/lib/source/DobbyLogRelay.cpp | Initialized member variables and replaced strcpy with strncpy |
| daemon/lib/source/DobbyContainer.cpp | Initialized mRestartCount member variable |
| daemon/lib/source/Dobby.cpp | Improved error handling for reply sends and removed unreachable code |
| client/tool/source/Main.cpp | Fixed TOCTOU vulnerability, added missing break, and lock guards |
| client/lib/source/DobbyProxy.cpp | Fixed lock guard scoping |
| bundle/tool/source/Main.cpp | Added exception handling in main and missing break statement |
| bundle/lib/source/DobbyTemplate.cpp | Fixed potential use-after-unlock by copying pointer before unlock |
| bundle/lib/source/DobbySpecConfig.cpp | Initialized variables and added lock guard |
| bundle/lib/source/DobbyRootfs.cpp | Replaced access with open to fix TOCTOU vulnerability |
| bundle/lib/source/DobbyConfig.cpp | Added lock guard and fixed indentation |
| bundle/lib/source/DobbyBundleConfig.cpp | Added lock guards for const methods and fixed indentation |
| bundle/lib/include/DobbyBundleConfig.h | Fixed indentation |
| AppInfrastructure/ReadLine/source/ReadLine.cpp | Fixed escape sequence in format string |
| AppInfrastructure/Public/Common/Notifier.h | Removed extra unlock and added Coverity annotation |
| AppInfrastructure/IpcService/source/sdbus/SDBusIpcService.cpp | Fixed integer overflow in timeout calculations |
| AppInfrastructure/Common/source/Timer.cpp | Added exception handling in destructor |
| AppInfrastructure/Common/source/ThreadedDispatcher.cpp | Improved lock management and fixed indentation |
| AppInfrastructure/Common/include/IDGenerator.h | Replaced rand() with random_device for better randomness |
| AppInfrastructure/Common/include/ConditionVariable.h | Removed unreachable return statement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 51 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
rdkPlugins/Minidump/source/AnonymousFile.cpp:140
getFileSize()can return a negative value on error (ftellreturns -1). The newfileSize <= 0check treats this as success and returnstrue, silently skipping error handling. HandlefileSize < 0as a failure (log and returnfalse), and only treatfileSize == 0as the empty-file success case.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
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)
rdkPlugins/Minidump/source/AnonymousFile.cpp:140
getFileSize()returns-1onftell()failure, butcopyContentTo()now treatsfileSize <= 0as success and returnstrue. That means real errors (e.g.ftell/fseekfailures) will be silently ignored and the minidump won’t be persisted. Distinguish empty (== 0) from error (< 0) and returnfalse(or log + fail) on error.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (path.length() >= sizeof(address.sun_path)) | ||
| { | ||
| AI_LOG_ERROR("Socket path too long: %s", path.c_str()); | ||
| return -1; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Handle variable "sockFd" going out of scope leaks the handle.
High Impact, CWE-404
RESOURCE_LEAK
| if (path.length() >= sizeof(address.sun_path)) | ||
| { | ||
| AI_LOG_ERROR("Socket path too long: %s", path.c_str()); | ||
| return -1; |
There was a problem hiding this comment.
Coverity Issue - Resource leak
Handle variable "sockFd" going out of scope leaks the handle.
High Impact, CWE-404
RESOURCE_LEAK
There was a problem hiding this comment.
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.
| if (ret < 0) | ||
| break; | ||
|
|
||
| numFields += ret; | ||
| if (ret > 0 && numFields + ret <= maxFields && numFields + ret > 0) | ||
| numFields += ret; |
There was a problem hiding this comment.
fields is declared as struct iovec fields[maxFields + 2], and processPid() can return 2 fields. The new guard compares against maxFields (16), so when numFields == 15 and ret == 2 it will not advance numFields even though two iov entries were written. This can lead to subsequent fields overwriting prior entries and sd_journal_sendv() being called with an incorrect numFields. Use the actual capacity (e.g., std::size(fields)) for the bounds check and treat “insufficient remaining space for ret fields” as a hard parse failure (break/return) rather than silently skipping the increment.
| 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; |
There was a problem hiding this comment.
For file bind mounts, the destination is opened with O_CREAT | O_WRONLY. If the destination file already exists but is not writable (common if the rootfs is read-only), this open will fail even though the code comment says it only needs an inode to exist for the bind mount. Prefer opening with read access (e.g., O_RDONLY | O_CREAT | O_CLOEXEC) or another non-writing approach so existing read-only files don’t cause mount preparation to fail.
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/recipes-containers/dobby/dobby.inc) must be modified to support the changes in this PR (beyond updatingSRC_REV)