test-cov1: Fixing medium and high issue[DONOT MERGE]#108
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 PR targets Coverity-reported critical/high issues in the JS runtime and related components (memory safety, error handling, and resource cleanup) and adds some robustness improvements around networking and main entrypoints.
Changes:
- Adds structured exception handling around multiple
main()functions and improves logging for uncaught exceptions from JSC and WebSocket clients. - Tightens cURL usage and other resource handling (timeouts, mutexes, network metrics data) across
jsc_lib,NativeJSRenderer, and JavaScript utility bindings. - Fixes or mitigates various Coverity findings (conditional logging, missing deletes, safer mutex unlocks, virtual destructor in
IJavaScriptContext, etc.).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/jsruntime.cpp | Wraps the main runtime entrypoint in a try/catch and removes an ineffective null check after std::make_shared, improving robustness against uncaught exceptions. |
| src/jsc/jsc_lib/jsc_lib.cpp | Enhances JSC error reporting with reportUncaughtExceptionAtEventLoop and hardens downloadFile with per-option cURL error checks and early returns. |
| src/jsc/JavaScriptUtils.cpp | Fixes dispatch mutex unlocking, improves error handling for video start time and network metrics, and augments rtReadBinaryBinding (though fopen/malloc error paths still need completion). |
| src/jsc/JavaScriptEngine.cpp | Corrects the garbage collection interval log format to use %f for the double interval. |
| src/jsc/JavaScriptContext.cpp | Ensures mNetworkMetricsData is deleted in the destructor and guards gAAMPJSBindings->fnLoadJS with a null check on gAAMPJSBindings. |
| src/NativeJSRenderer.cpp | Cleans up locking in createApplicationInternal, reworks the developer console processing queue, updates format strings, and hardens downloadFile against cURL option failures (though continuing after cleanup is currently unsafe). |
| src/JSRuntimeServer.cpp | Improves JSON parsing error handling for numeric fields and initializes the res variable in JsonWrap::getUint32 on error. |
| src/JSRuntimeClientContainer.cpp | Wraps the container client main() in try/catch, improving diagnostics and safety on failure. |
| src/JSRuntimeClient.cpp | Uses a predicate-based condition variable wait in run() and wraps main() in try/catch to better handle connection and runtime errors. |
| include/IJavaScriptContext.h | Adds a virtual destructor to the IJavaScriptContext interface for safe polymorphic deletion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -85,6 +85,7 @@ class JsonWrap | |||
| if (!itm || !cJSON_IsNumber(itm)) | |||
| { | |||
| std::cerr << "Error: " << name << "is not a Uint32_t" << std::endl; | |||
There was a problem hiding this comment.
The error message in JsonWrap::getUint32 prints "Error: " << name << "is not a Uint32_t", which is missing a space before is and uses a non-standard type spelling (Uint32_t). For clarity and consistency with the actual type, consider changing the message to something like "Error: <name> is not a uint32_t".
| std::cerr << "Error: " << name << "is not a Uint32_t" << std::endl; | |
| std::cerr << "Error: " << name << " is not a uint32_t" << std::endl; |
| 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.
In terminateApplicationInternal, the else branch logs mContextMap[id].url.c_str() even though this branch is only taken when mContextMap.find(id) == mContextMap.end(), so mContextMap[id] will default-insert a new entry for a non-existent id just to log it. This both masks the "not found" condition and mutates the map unexpectedly; the log should use the iterator (mapEntry) or avoid indexing the map when the id is absent.
| 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); |
| if(!mTestFileName.empty()) | ||
| { | ||
| ModuleSettings settings; | ||
| uint32_t id; | ||
| settings.enableJSDOM = mEnableTestFileDOMSupport; | ||
| ApplicationRequest appRequest(id, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer); | ||
| NativeJSRenderer::createApplicationInternal(appRequest); |
There was a problem hiding this comment.
In NativeJSRenderer::run(), the id used to construct ApplicationRequest appRequest(id, RUN, mTestFileName, ...) is never initialized, so mId and all subsequent uses of this id (context map access, logging, etc.) rely on an indeterminate value. This is undefined behavior and can result in collisions or hard-to-reproduce failures; the id should be obtained from a defined source (e.g., a counter or createApplication-like path) before being passed into ApplicationRequest.
| } | ||
| 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.
In NativeJSRenderer::downloadFile, when any curl_easy_setopt call fails you log an error and call curl_easy_cleanup(curl) but then continue to use curl for subsequent curl_easy_setopt calls and later curl_easy_perform, which is undefined behavior because the handle has been cleaned up. After a failed curl_easy_setopt, the function should stop using the handle (e.g., return false immediately or guard the curl_easy_perform path with a success flag) instead of continuing with a freed/invalid CURL*.
| } | |
| 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; |
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