cov-test: Fixing High and medium level issues[DONOT MERGE]#106
cov-test: Fixing High and medium level issues[DONOT MERGE]#106Sid2001-maker wants to merge 1 commit into
Conversation
…tical and High issues Reason for change: Resolve Critical and high level issues in coverity Test Procedure: build should be successful Risk: low Priority: P2
There was a problem hiding this comment.
Pull request overview
This pull request addresses Coverity scan issues by fixing critical and high-level defects in the codebase. The changes focus on improving error handling, resource management, input validation, and fixing potential crashes.
Changes:
- Added exception handling wrappers in main functions across multiple files
- Fixed curl_easy_setopt error handling with proper return values after cleanup
- Added input validation for untrusted Wayland display names to prevent injection attacks
- Fixed memory leaks and null pointer checks, improved mutex handling
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/jsruntime.cpp | Added try-catch exception handlers, input validation for Wayland display, removed unnecessary null check for shared_ptr |
| src/jsc/jsc_lib/jsc_lib.cpp | Fixed curl error handling, improved exception reporting, optimized path separator check |
| src/jsc/JavaScriptUtils.cpp | Fixed mutex unlock to use lock guard, added stat/fread error handling, fixed memory leaks in metrics function |
| src/jsc/JavaScriptEngine.cpp | Fixed format specifier from %d to %f for double value |
| src/jsc/JavaScriptContext.cpp | Added null check for gAAMPJSBindings, added destructor cleanup for mNetworkMetricsData |
| src/NativeJSRenderer.cpp | Fixed curl error handling, removed premature mutex unlock, fixed string format specifier, removed uninitialized variable usage in one location but introduced it in another |
| src/JSRuntimeServer.cpp | Initialized res variable to 0 in error case |
| src/JSRuntimeClient.cpp | Added exception handlers, improved wait_for with predicate |
| src/JSRuntimeClientContainer.cpp | Added exception handling in main function |
| include/IJavaScriptContext.h | Added virtual destructor to interface class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class IJavaScriptContext | ||
| { | ||
| public: | ||
| virtual ~IJavaScriptContext() = default; //change added |
There was a problem hiding this comment.
The comment "//change added" is unclear and unnecessary. Comments should explain why code exists, not document when it was added (that's what version control is for).
| virtual ~IJavaScriptContext() = default; //change added | |
| virtual ~IJavaScriptContext() = default; |
|
|
||
| std::string ipAddress = JSRuntimeContainer::getContainerIpAddress(containerId); | ||
| if (ipAddress.empty()) { | ||
| NativeJSLogger::log(ERROR, "Failed to retrieve IP address for container"); |
There was a problem hiding this comment.
Missing newline character in the log message format string. The message should end with \n for proper logging output formatting.
| return 1; | ||
| } | ||
| catch (...) { | ||
| NativeJSLogger::log(ERROR, "Unknown exception in main"); |
There was a problem hiding this comment.
Missing newline character in the log message format string. The message should end with \n for proper logging output formatting.
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYHOST: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, true); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYPEER: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_USERAGENT, "libcurl-agent/1.0"); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_USERAGENT: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_PROXY, ""); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_PROXY: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); |
There was a problem hiding this comment.
After curl_easy_cleanup is called on error, the function continues executing with the same curl handle which is now invalid. After each error case with curl_easy_cleanup, the function should return immediately to avoid using the freed handle.
| } | |
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYHOST: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| } | |
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, true); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYPEER: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| } | |
| res = curl_easy_setopt(curl, CURLOPT_USERAGENT, "libcurl-agent/1.0"); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_USERAGENT: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| } | |
| res = curl_easy_setopt(curl, CURLOPT_PROXY, ""); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_PROXY: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| return ret; | |
| } | |
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYHOST: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| return ret; | |
| } | |
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, true); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYPEER: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| return ret; | |
| } | |
| res = curl_easy_setopt(curl, CURLOPT_USERAGENT, "libcurl-agent/1.0"); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_USERAGENT: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| return ret; | |
| } | |
| res = curl_easy_setopt(curl, CURLOPT_PROXY, ""); | |
| if (res != CURLE_OK) { | |
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_PROXY: %s\n", curl_easy_strerror(res)); | |
| curl_easy_cleanup(curl); | |
| return ret; |
| res = curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_URL: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_FOLLOWLOCATION: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, CallbackHeader); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_HEADERFUNCTION: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_HEADERDATA, (void *)&chunk); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_HEADERDATA: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, CallbackOnMemoryWrite); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_WRITEFUNCTION: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_WRITEDATA, (void *)&chunk); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_WRITEDATA: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_TIMEOUT, 30); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_TIMEOUT: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1L); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_NOSIGNAL: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, 2); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYHOST: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, true); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_SSL_VERIFYPEER: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_USERAGENT, "libcurl-agent/1.0"); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_USERAGENT: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } | ||
| res = curl_easy_setopt(curl, CURLOPT_PROXY, ""); | ||
| if (res != CURLE_OK) { | ||
| NativeJSLogger::log(ERROR, "Failed to set CURLOPT_PROXY: %s\n", curl_easy_strerror(res)); | ||
| curl_easy_cleanup(curl); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
After curl_easy_cleanup is called on error, the function continues executing with the same curl handle which is now invalid. After each error case with curl_easy_cleanup, the function should return immediately to avoid using the freed handle.
| return 0; | ||
| } | ||
| catch (const std::exception& e) { | ||
| NativeJSLogger::log(ERROR, "Exception in main: %s", e.what()); |
There was a problem hiding this comment.
Missing newline character in the log message format string. The message should end with \n for proper logging output formatting.
| if(!mTestFileName.empty()) | ||
| { | ||
| ModuleSettings settings; | ||
| uint32_t id; |
There was a problem hiding this comment.
The variable 'id' is declared but not initialized before being passed to ApplicationRequest constructor. This will result in undefined behavior as the uninitialized value is used to construct the ApplicationRequest object and subsequently used in createApplicationInternal and runApplicationInternal calls.
| uint32_t id; | |
| uint32_t id = 0; |
| else | ||
| { | ||
| NativeJSLogger::log(ERROR, "Unable to find application with id: %d and url: %s\n", id, mContextMap[id].url); | ||
| NativeJSLogger::log(ERROR, "Unable to find application with id: %d and url: %s\n", id, mContextMap[id].url.c_str()); |
There was a problem hiding this comment.
This code attempts to access mContextMap[id] in the else branch when the id was not found in the map (mapEntry == mContextMap.end()). This will either create a new invalid entry in the map or access uninitialized memory, potentially causing a crash or undefined behavior.
| NativeJSLogger::log(ERROR, "Unable to find application with id: %d and url: %s\n", id, mContextMap[id].url.c_str()); | |
| NativeJSLogger::log(ERROR, "Unable to find application with id: %d\n", id); |
| rtValue keys; | ||
| if (map->Get("allKeys", &keys) != RT_OK) { | ||
| rtLogWarn("Could not retrieve url for network metrics data."); | ||
| delete netMetricsArray; //newly added |
There was a problem hiding this comment.
The comment "//newly added" is unnecessary and should be removed. Comments should explain why code exists, not document when it was added (that's what version control is for).
|
|
||
| if (!keysArray) { | ||
| rtLogWarn("No url found in the network metrics data."); | ||
| delete netMetricsArray; //newly added |
There was a problem hiding this comment.
The comment "//newly added" is unnecessary and should be removed. Comments should explain why code exists, not document when it was added (that's what version control is for).
RDKEMW-12252: Coverity Scan Report - Analyzing and Fixing all the Critical and High issues
Reason for change: Resolve Critical and high level issues in coverity
Test Procedure: build should be successful
Risk: low
Priority: P2