Checking the coverity issue[Do Not Merge] by Sid2001-maker · Pull Request #109 · rdkcentral/rdkNativeScript

Sid2001-maker · 2026-01-20T11:15:46Z

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

Copilot

Pull request overview

This PR addresses Critical and High level issues identified by Coverity scan analysis. The changes focus on improving error handling, fixing resource leaks, correcting format specifiers, improving concurrency safety, and adding proper exception handling.

Changes:

Added exception handling to main functions with try-catch blocks
Fixed resource leaks by adding proper cleanup in error paths
Improved error handling for curl operations with return value checking
Fixed concurrency issues with improved mutex handling and condition variable predicates
Corrected format specifiers and added missing null pointer checks

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/jsruntime.cpp	Added try-catch exception handling, removed unnecessary null check for shared_ptr, commented out tainted string validation
src/jsc/jsc_lib/jsc_lib.cpp	Added error checking for all curl_easy_setopt calls, fixed exception value logging, improved path separator handling
src/jsc/JavaScriptUtils.cpp	Fixed mutex handling with lock_guard, added error checking for file operations, fixed memory leaks in error paths
src/jsc/JavaScriptEngine.cpp	Fixed format specifier from %d to %f for double value
src/jsc/JavaScriptContext.cpp	Added memory cleanup for mNetworkMetricsData, added null check before using gAAMPJSBindings
src/NativeJSRenderer.cpp	Removed extraneous mutex unlock, fixed variable scope, improved processDevConsoleRequests concurrency, added curl error handling, fixed format string
src/JSRuntimeServer.cpp	Initialized res variable to prevent use of uninitialized value
src/JSRuntimeClientContainer.cpp	Added try-catch exception handling to main function
src/JSRuntimeClient.cpp	Added predicates to condition variable wait_for calls, added exception handling to main
include/JSRuntimeClient.h	Added mResponseReceived flag to track response state for condition variable
include/IJavaScriptContext.h	Added virtual destructor to prevent undefined behavior with polymorphic deletion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-20T11:20:26Z

  buffer = (char*)malloc(size);
-  fread(buffer,size,1,ptr); // read 10 bytes to our buffer
+  size_t bytesRead = fread(buffer, size, 1, ptr);


Memory leak when malloc fails. If malloc at line 335 returns nullptr (memory allocation failure), the code will dereference a null pointer in the fread call at line 336, leading to a crash. The code should check if buffer is not null before proceeding with fread.

Copilot · 2026-01-20T11:20:26Z

+    /*
+    //CID:430751:Use of untrusted string value (TAINTED_STRING)
+    // Validate waylanddisplay to prevent injection attacks
+    if (!waylanddisplay.empty()) {
+        if (waylanddisplay.length() > 256) {
+            NativeJSLogger::log(ERROR, "Invalid Wayland display name: too long\n");
+            waylanddisplay = "";
+        } else {
+            bool valid = true;
+            for (char c : waylanddisplay) {
+                if (!isalnum(c) && c != '-' && c != '_' && c != '/' && c != '.') {
+                    valid = false;
+                    break;
+                }
+            }
+            if (!valid) {
+                NativeJSLogger::log(ERROR, "Invalid Wayland display name: contains invalid characters\n");
+                waylanddisplay = "";
+            }
+        }
+    }
+    */
+
+


The commented-out validation code for waylanddisplay should either be implemented or removed. Leaving large blocks of commented code reduces maintainability and creates confusion about whether this security validation is actually needed. If this validation is required to address the Coverity issue CID:430751 (TAINTED_STRING), it should be uncommented and properly implemented. If it's not needed, the comment block should be removed entirely.

Suggested change

/*

//CID:430751:Use of untrusted string value (TAINTED_STRING)

// Validate waylanddisplay to prevent injection attacks

if (!waylanddisplay.empty()) {

if (waylanddisplay.length() > 256) {

NativeJSLogger::log(ERROR, "Invalid Wayland display name: too long\n");

waylanddisplay = "";

} else {

bool valid = true;

for (char c : waylanddisplay) {

if (!isalnum(c) && c != '-' && c != '_' && c != '/' && c != '.') {

valid = false;

break;

}

}

if (!valid) {

NativeJSLogger::log(ERROR, "Invalid Wayland display name: contains invalid characters\n");

waylanddisplay = "";

}

}

}

*/

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-21T10:39:30Z

+        return 0;
+    }
+    catch (const std::exception& e) {
+        NativeJSLogger::log(ERROR, "Exception in main: %s", e.what());


Missing format string argument. The log message uses a format specifier '%s' but doesn't provide the exception message as an argument. It should be: NativeJSLogger::log(ERROR, "Exception in main: %s", e.what());

Suggested change

NativeJSLogger::log(ERROR, "Exception in main: %s", e.what());

NativeJSLogger::log(ERROR, std::string("Exception in main: ") + e.what());

Copilot · 2026-01-21T10:39:31Z

  rtValue keys;
  if (map->Get("allKeys", &keys) != RT_OK) {
    rtLogWarn("Could not retrieve url for network metrics data.");
+    delete netMetricsArray; //newly added


The comment "//newly added" should be removed. Comments should explain why the code exists, not when it was added. Version control systems track when changes were made.

Copilot · 2026-01-21T10:39:32Z

            appendindex = i-1;
 	    i++;
-            waylanddisplay = argv[i];
+            if (i < argc && argv[i] != nullptr && strlen(argv[i]) > 0) {


The nullptr check for argv[i] is redundant. Since argv is guaranteed to be null-terminated and the check 'i < argc' already ensures the index is valid, argv[i] cannot be nullptr at this point. The strlen check is still useful to catch empty strings.

Suggested change

if (i < argc && argv[i] != nullptr && strlen(argv[i]) > 0) {

if (i < argc && strlen(argv[i]) > 0) {

Copilot · 2026-01-21T10:39:33Z


  if (!keysArray) {
    rtLogWarn("No url found in the network metrics data.");
+    delete netMetricsArray; //newly added


The comment "//newly added" should be removed. Comments should explain why the code exists, not when it was added. Version control systems track when changes were made.

trupthi1403 · 2026-01-22T07:23:28Z

Coverity Incremental Scan Results

No new Coverity issues found.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.

Comments suppressed due to low confidence (1)

src/NativeJSRenderer.cpp:511

Potential race condition: createApplicationInternal and runApplicationInternal at lines 510-511 are called without holding mUserMutex, but they access shared state (mContextMap). This could lead to race conditions if other threads are accessing the context map. Consider holding the mutex or ensuring thread safety for these operations.

	    NativeJSRenderer::createApplicationInternal(appRequest);
	    NativeJSRenderer::runApplicationInternal(appRequest);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T07:23:40Z

  buffer = (char*)malloc(size);
-  fread(buffer,size,1,ptr); // read 10 bytes to our buffer
+  size_t bytesRead = fread(buffer, size, 1, ptr);


Missing null check after malloc. If malloc fails and returns NULL, the subsequent fread will cause undefined behavior. A null check should be added after line 341 to handle allocation failure.

Copilot · 2026-01-22T07:23:40Z

  rtValue keys;
  if (map->Get("allKeys", &keys) != RT_OK) {
    rtLogWarn("Could not retrieve url for network metrics data.");
+    delete netMetricsArray; //newly added


The comment "newly added" should be removed as it doesn't add value to the code and will become outdated. Code comments should explain why something is done, not document when it was added (that's what version control is for).

Copilot · 2026-01-22T07:23:41Z


  if (!keysArray) {
    rtLogWarn("No url found in the network metrics data.");
+    delete netMetricsArray; //newly added


The comment "newly added" should be removed as it doesn't add value to the code and will become outdated. Code comments should explain why something is done, not document when it was added (that's what version control is for).

Copilot · 2026-01-22T07:23:41Z

 	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());


Critical bug: Accessing mContextMap[id] when the id was not found (in the else branch) will either create a new map entry or cause undefined behavior. The error message cannot access the url because the id doesn't exist in the map. Consider removing the url from this error message or storing it from a different source.

Suggested change

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);

trupthi1403 · 2026-01-22T07:34:03Z

Coverity Incremental Scan Results

No new Coverity issues found.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-22T09:46:55Z

+        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;
+        }


Adding error checking for curl_easy_setopt calls is good practice. However, the extensive repetition suggests this could be refactored into a helper function to reduce code duplication and improve maintainability. Consider creating a wrapper function that checks the result and logs errors.

Copilot · 2026-01-22T09:46:56Z

+
+    // Allow common wayland display formats: empty, wayland-N, wayland-N.M, or alphanumeric socket names
+    // Pattern is anchored and uses simple character classes to prevent ReDoS
+    static const std::regex kDisplayRe(R"(^(|wayland-[0-9]+(\.[0-9]+)?|[a-zA-Z0-9_.\-]+)$)");


The regex pattern allows empty strings which means validateWaylandDisplay will accept empty input. However, waylanddisplay is initialized to an empty string and may never be set via command line. This could pass an empty display string to NativeJSRenderer. Consider either rejecting empty strings in validation or ensuring NativeJSRenderer handles empty display strings appropriately.

Suggested change

// Allow common wayland display formats: empty, wayland-N, wayland-N.M, or alphanumeric socket names

// Pattern is anchored and uses simple character classes to prevent ReDoS

static const std::regex kDisplayRe(R"(^(|wayland-[0-9]+(\.[0-9]+)?|[a-zA-Z0-9_.\-]+)$)");

// Reject empty display names to avoid passing an unusable display string downstream

if (s.empty()) {

throw std::invalid_argument("WAYLAND_DISPLAY cannot be empty");

}

// Allow common wayland display formats: wayland-N, wayland-N.M, or alphanumeric socket names

// Pattern is anchored and uses simple character classes to prevent ReDoS

static const std::regex kDisplayRe(R"(^(wayland-[0-9]+(\.[0-9]+)?|[a-zA-Z0-9_.\-]+)$)");

Copilot · 2026-01-22T09:46:56Z

+    // Input validated - construct sanitized string
+    std::string result(s.data(), s.size());
+
+    // Explicitly mark as sanitized for Coverity
+    __coverity_tainted_data_sanitize__(&result);
+
+    // coverity[tainted_string_return_content]
+    // coverity[tainted_data_return]
+    return result;


Creating a new string from s.data() and s.size() is redundant since 's' is already a validated string. This operation doesn't add any sanitization value. Simply return 's' directly after validation.

Suggested change

// Input validated - construct sanitized string

std::string result(s.data(), s.size());

// Explicitly mark as sanitized for Coverity

__coverity_tainted_data_sanitize__(&result);

// coverity[tainted_string_return_content]

// coverity[tainted_data_return]

return result;

// Input validated - mark as sanitized for Coverity

__coverity_tainted_data_sanitize__(&s);

// coverity[tainted_string_return_content]

// coverity[tainted_data_return]

return s;

Copilot · 2026-01-22T09:46:56Z

+        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, HeaderCallback);
+        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, WriteMemoryCallback);
+        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;
+        }


Adding error checking for curl_easy_setopt calls is good practice. However, the extensive repetition suggests this could be refactored into a helper function to reduce code duplication and improve maintainability. Consider creating a wrapper function that checks the result and logs errors.

Copilot · 2026-01-22T09:46:56Z

+  size_t bytesRead = fread(buffer, size, 1, ptr);
  fclose(ptr);

+  if (bytesRead != 1)
+  {
+    rtLogError("Failed to read file: expected 1 item, read %zu items", bytesRead);


The fread parameters are incorrect. The second parameter should be 1 (size of each item) and third parameter should be 'size' (number of items), but they are reversed. This should be fread(buffer, 1, size, ptr) to read 'size' bytes. The current code tries to read one item of 'size' bytes, which may work but is semantically incorrect and could cause issues with the return value check.

Suggested change

size_t bytesRead = fread(buffer, size, 1, ptr);

fclose(ptr);

if (bytesRead != 1)

{

rtLogError("Failed to read file: expected 1 item, read %zu items", bytesRead);

size_t bytesRead = fread(buffer, 1, size, ptr);

fclose(ptr);

if (bytesRead != static_cast<size_t>(size))

{

rtLogError("Failed to read file: expected %d bytes, read %zu bytes", size, bytesRead);

Copilot · 2026-01-22T09:46:57Z

@@ -321,17 +329,32 @@ rtError rtReadBinaryBinding(int numArgs, const rtValue* args, rtValue* result, v
  const char *fd = "hello.wasm";


The hardcoded filename "hello.wasm" suggests this is test or placeholder code. Using a hardcoded filename in production code is problematic. Consider making this configurable or removing this function if it's not intended for production use.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/NativeJSRenderer.cpp:511

Potential race condition: createApplicationInternal and runApplicationInternal are called on lines 510-511 without holding mUserMutex, but these functions access mContextMap which is shared state that needs synchronization. While this code path appears to be for test scenarios only (checking mTestFileName), there's still a risk of data races if other threads are accessing mContextMap concurrently. Consider holding mUserMutex when calling these functions, or document why synchronization is not needed here.

	    NativeJSRenderer::createApplicationInternal(appRequest);
	    NativeJSRenderer::runApplicationInternal(appRequest);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T11:11:39Z

 #ifndef UNIT_TEST_BUILD
 int main(int argc, char* argv[])
 {
+    try {


The PR title contains "Do Not Merge" which indicates this PR should not be merged. This is a significant discrepancy between the PR title and the fact that the PR is being reviewed for potential merge.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T11:42:41Z

+        auto setOptWithCheck = [&curl](CURLoption option, auto value, const char* optionName) -> bool {
+            CURLcode res = curl_easy_setopt(curl, option, value);
+            if (res != CURLE_OK) {
+                NativeJSLogger::log(ERROR, "Failed to set %s: %s\n", optionName, curl_easy_strerror(res));
+                curl_easy_cleanup(curl);
+                return false;
+            }
+            return true;
+        };


The curl handle is already cleaned up in the lambda at line 158 when setOptWithCheck fails. After the early return at lines 164-175, curl_easy_cleanup will not be called again at line 182 because the function exits. However, there's a subtle issue: the lambda captures curl by reference, but after calling curl_easy_cleanup(curl), the local pointer still holds the invalid value. While this doesn't cause a double-free since we return immediately, it's better to set curl to nullptr after cleanup for defensive programming. Consider adding 'curl = nullptr;' after curl_easy_cleanup in the lambda.

Copilot · 2026-01-30T11:42:42Z

+        auto setOptWithCheck = [&curl](CURLoption option, auto value, const char* optionName) -> bool {
+            CURLcode res = curl_easy_setopt(curl, option, value);
+            if (res != CURLE_OK) {
+                NativeJSLogger::log(ERROR, "Failed to set %s: %s\n", optionName, curl_easy_strerror(res));
+                curl_easy_cleanup(curl);
+                return false;
+            }
+            return true;
+        };


The same curl handle cleanup issue exists here as in jsc_lib.cpp. The lambda captures curl by reference and calls curl_easy_cleanup, but doesn't set the pointer to nullptr. While this doesn't cause a double-free due to immediate return, consider setting curl to nullptr after cleanup for defensive programming consistency.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T12:00:11Z

+    return RT_ERROR;
+  }
+
  int size = buf.st_size;


The variable size is declared as int but stat returns st_size as off_t which can be very large. If st_size is larger than INT_MAX, assigning it to int will cause integer overflow. Consider declaring size as off_t or checking that buf.st_size fits in an int before the assignment.

…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

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T12:06:58Z

+        // Helper lambda to check curl_easy_setopt results
+        auto setOptWithCheck = [&curl](CURLoption option, auto value, const char* optionName) -> bool {
+            CURLcode res = curl_easy_setopt(curl, option, value);
+            if (res != CURLE_OK) {
+                NativeJSLogger::log(ERROR, "Failed to set %s: %s\n", optionName, curl_easy_strerror(res));
+                curl_easy_cleanup(curl);
+                curl = nullptr;
+                return false;
+            }
+            return true;
+        };


The lambda captures curl by reference and then sets it to nullptr inside the lambda when an error occurs. This creates a problem because after setting curl to nullptr, subsequent calls to setOptWithCheck will still use the captured reference which is now nullptr, leading to a crash. After the first failure, curl_easy_cleanup is called with a valid handle and curl is set to nullptr, but the reference 'curl' in the outer scope becomes nullptr. This means if curl_easy_perform is reached later (though it won't be due to early returns), it would try to use a null pointer.

Consider either: 1) Not setting curl to nullptr in the lambda, or 2) Check if curl is nullptr before each subsequent call.

Copilot · 2026-01-30T12:06:58Z

 	    ApplicationRequest appRequest(id, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);
+	    mUserMutex.lock();
 	    NativeJSRenderer::createApplicationInternal(appRequest);
 	    NativeJSRenderer::runApplicationInternal(appRequest);


The ApplicationRequest created here uses the RUN type but calls both createApplicationInternal and runApplicationInternal. This is inconsistent with the normal flow where a CREATE type request would be used for createApplicationInternal. This means the appRequest has type RUN when it's passed to createApplicationInternal, which might cause issues if createApplicationInternal logic depends on the request type.

Suggested change

ApplicationRequest appRequest(id, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);

mUserMutex.lock();

NativeJSRenderer::createApplicationInternal(appRequest);

NativeJSRenderer::runApplicationInternal(appRequest);

ApplicationRequest createRequest(id, CREATE, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);

ApplicationRequest runRequest(id, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);

mUserMutex.lock();

NativeJSRenderer::createApplicationInternal(createRequest);

NativeJSRenderer::runApplicationInternal(runRequest);

Copilot · 2026-01-30T12:06:58Z

            			context->runScript(window.str().c_str(),true, url, nullptr, true);
 			}
-			NativeJSLogger::log(INFO, "nativeJS application thunder execution url: %s, result: %d\n", url.c_str(), ret ? 1 : 0);
+			NativeJSLogger::log(INFO, "nativeJS application thunder execution url: %s\n", url.c_str());


The log message on line 367 was changed to log only the URL without the result. However, the very next line (369) logs the result anyway with the same information. This creates redundant logging where the URL is logged twice - once on line 367 without the result, and then again on line 369 with the result. Consider removing line 367 entirely since line 369 provides all the necessary information.

Suggested change

NativeJSLogger::log(INFO, "nativeJS application thunder execution url: %s\n", url.c_str());

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-30T12:19:07Z

  if (result)
  {
      result->setString(buffer);
  }
+
+  free(buffer);


The buffer is passed to setString and then immediately freed. Verify that rtValue::setString makes a deep copy of the string data. If setString only stores a pointer without copying, this will cause a use-after-free bug. Consider moving the free call after confirming setString's behavior, or restructure to use RAII.

Sid2001-maker requested a review from a team as a code owner January 20, 2026 11:15

Copilot AI review requested due to automatic review settings January 20, 2026 11:15

Copilot started reviewing on behalf of Sid2001-maker January 20, 2026 11:16 View session

Copilot AI reviewed Jan 20, 2026

View reviewed changes

Sid2001-maker force-pushed the topic/RDKEMW-12252 branch 2 times, most recently from c976066 to 81b3718 Compare January 21, 2026 10:34

Copilot AI review requested due to automatic review settings January 21, 2026 10:34

Copilot started reviewing on behalf of Sid2001-maker January 21, 2026 10:35 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

Sid2001-maker force-pushed the topic/RDKEMW-12252 branch from 81b3718 to 11e0cf0 Compare January 22, 2026 06:42

Copilot AI review requested due to automatic review settings January 22, 2026 07:18

Sid2001-maker force-pushed the topic/RDKEMW-12252 branch from 11e0cf0 to 7aeefc3 Compare January 22, 2026 07:18

Copilot started reviewing on behalf of Sid2001-maker January 22, 2026 07:19 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

Sid2001-maker force-pushed the topic/RDKEMW-12252 branch 2 times, most recently from d5ae500 to 476eb66 Compare January 22, 2026 09:40

Copilot AI review requested due to automatic review settings January 22, 2026 09:40

Copilot started reviewing on behalf of Sid2001-maker January 22, 2026 09:40 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

Sid2001-maker force-pushed the topic/RDKEMW-12252 branch from 476eb66 to 4c45a0d Compare January 22, 2026 10:09

Copilot AI review requested due to automatic review settings January 22, 2026 10:33

Sid2001-maker force-pushed the topic/RDKEMW-12252 branch from 4c45a0d to c710692 Compare January 22, 2026 10:33

Copilot started reviewing on behalf of Sid2001-maker January 22, 2026 10:33 View session

Copilot AI reviewed Jan 22, 2026

Sid2001-maker requested a review from Copilot January 30, 2026 09:42

Copilot started reviewing on behalf of Sid2001-maker January 30, 2026 09:42 View session