FIx High level Coverity issue[Test PR] by Sid2001-maker · Pull Request #112 · rdkcentral/rdkNativeScript

Sid2001-maker · 2026-02-04T11:15:44Z

Test Changes do not merge

github-actions · 2026-02-04T11:15:57Z

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

_{You can retrigger this bot by commenting recheck in this Pull Request.}_{Posted by the CLA Assistant Lite bot.}

Copilot

Pull request overview

PR aims to address high-severity Coverity findings by improving initialization, cleanup, and error handling across JS runtime utilities and lifecycle code.

Changes:

Hardened rtReadBinaryBinding with argument validation and I/O error handling.
Added additional cleanup on early-return paths in network metrics download and JS context teardown.
Fixed an uninitialized variable issue (JSON uint parsing) and improved interface safety (virtual destructor).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`src/jsc/JavaScriptUtils.cpp`	Adds validation/error handling for binary reads and adds cleanup on network metrics early returns.
`src/jsc/JavaScriptContext.cpp`	Frees `mNetworkMetricsData` during context destruction to prevent leaks.
`src/NativeJSRenderer.cpp`	Fixes test application request ID creation (previously used an uninitialized ID).
`src/JSRuntimeServer.cpp`	Initializes `res` to avoid returning an uninitialized value on error.
`include/IJavaScriptContext.h`	Adds a virtual destructor for safe polymorphic deletion.

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

Copilot · 2026-02-04T11:21:15Z

+  size_t bytesRead = fread(buffer, 1, size, ptr);
+  if (bytesRead != (size_t)size) {
+    rtLogError("Failed to read complete file. Expected %d bytes, read %zu bytes", size, bytesRead);
+    free(buffer);
+    fclose(ptr);
+    return RT_ERROR;
+  }
+
+  // 6. Set result only if buffer is valid
+  if (result) {
+    result->setString(buffer);
  }


result->setString(buffer) treats the file contents as a C-string, but buffer is not NUL-terminated and may contain embedded NULs (especially for .wasm). This can truncate data or read past the allocation.

Either allocate size + 1 and NUL-terminate for text files, or return a binary-safe type (e.g., byte array / ArrayBuffer / base64) along with the length.

Copilot · 2026-02-04T11:21:15Z

  rtArrayObject* netMetricsArray = new rtArrayObject();

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


netMetricsArray is an rtArrayObject-style object typically managed via rtRef/rtObjectRef (ref-counted). Manually delete-ing it on error paths can bypass the expected AddRef/Release lifecycle and risks double-free/use-after-free if ownership semantics change.

Prefer rtRef<rtArrayObject> netMetricsArray = new rtArrayObject(); (or rtObjectRef) and let RAII handle cleanup on early returns.

Copilot · 2026-02-04T11:21:15Z

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


Same ownership concern here: manually deleting netMetricsArray is error-prone with ref-counted rtObjects.

Use an rtRef/rtObjectRef for netMetricsArray so it is automatically released on failure paths.

Copilot · 2026-02-04T11:21:15Z

      if (!metrics) {
        rtLogError("Failed to cast stored value to NetworkMetrics structure for url: %s.", key.cString());
+        delete netMetricsArray;
        return RT_FAIL;


Same issue on this failure path: prefer RAII (rtRef/rtObjectRef) over delete netMetricsArray to match the rest of the codebase’s ref-counted object ownership model.

Copilot · 2026-02-04T11:21:16Z

+            uint32_t testId = createApplicationIdentifier();
 	    settings.enableJSDOM = mEnableTestFileDOMSupport;
-	    ApplicationRequest appRequest(id, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);
+	    ApplicationRequest appRequest(testId, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);


createApplicationIdentifier() uses a static uint32_t id without synchronization. Calling it here (outside mUserMutex) introduces a data race with other threads that call createApplication() (which also calls createApplicationIdentifier()).

Make the counter std::atomic<uint32_t> or ensure all calls (including this test-file path) are protected by the same mutex.

Copilot · 2026-02-04T11:21:16Z

+  }

-  const char *fd = "hello.wasm";
+  const char *fd = args[0].toString().cString();


fd is taken from args[0].toString().cString(), but toString() returns a temporary; storing the cString() pointer can leave fd dangling before it’s used by fopen/stat.

Store the rtString in a local variable first (or otherwise ensure the backing string outlives all uses of fd).

Suggested change

const char *fd = args[0].toString().cString();

rtString filePath = args[0].toString();

const char *fd = filePath.cString();

Copilot · 2026-02-04T11:21:16Z

+  // 1. Validate numArgs and use args for filename
+  if (numArgs < 1 || args[0].getType() != RT_stringType) {
+    rtLogError("%s: missing or invalid file path", __FUNCTION__);
+    return RT_ERROR_INVALID_ARG;
+  }

-  const char *fd = "hello.wasm";
+  const char *fd = args[0].toString().cString();
+  FILE *ptr = fopen(fd, "rb");


This binding now reads an arbitrary filesystem path provided by JS (args[0]). That enables scripts to read any readable file on the device (path traversal / sensitive file disclosure), whereas the old code only read a fixed filename.

If this API is exposed to untrusted content, restrict reads to an allowlisted directory, enforce a sandboxed root, or remove the ability to pass arbitrary paths.

Copilot · 2026-02-04T11:21:16Z

  int size = buf.st_size;
+  if (size <= 0) {
+    rtLogError("Invalid file size: %d", size);
+    fclose(ptr);
+    return RT_ERROR;
+  }

-  buffer = (char*)malloc(size);
-  fread(buffer,size,1,ptr); // read 10 bytes to our buffer
-  fclose(ptr);
+  // 4. Check malloc
+  char *buffer = (char*)malloc(size);


buf.st_size is an off_t (often 64-bit) but it’s being narrowed to int. Large files can overflow and lead to undersized allocations/reads.

Use an appropriate unsigned size type (size_t/uint64_t) and validate bounds before allocating/reading.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 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-02-04T11:26:18Z

+  // 4. Check malloc
+  char *buffer = (char*)malloc(size);
+  if (!buffer) {
+    rtLogError("Failed to allocate memory for file: %s", fd);
+    fclose(ptr);
+    return RT_ERROR;
+  }

-  if (result)
-  {
-      result->setString(buffer);
+  // 5. Robust fread
+  size_t bytesRead = fread(buffer, 1, size, ptr);
+  if (bytesRead != (size_t)size) {
+    rtLogError("Failed to read complete file. Expected %d bytes, read %zu bytes", size, bytesRead);
+    free(buffer);
+    fclose(ptr);
+    return RT_ERROR;
+  }
+
+  // 6. Set result only if buffer is valid
+  if (result) {
+    result->setString(buffer);
  }
+  free(buffer);


This reads raw bytes into buffer and passes it to setString(buffer) without ensuring NUL-termination. For binary files (e.g., wasm) this will truncate at the first '\0' and can also read past the allocation if setString expects a C-string. Consider allocating size + 1 and setting buffer[size] = '\0', or better, use an API that preserves length (e.g., construct an rtString with explicit length / return a byte array type) instead of treating binary data as a C-string.

Copilot · 2026-02-04T11:26:18Z

+  // 3. Check stat
  struct stat buf;
+  if (stat(fd, &buf) != 0) {
+    rtLogError("Failed to stat file: %s", fd);
+    fclose(ptr);
+    return RT_ERROR;
+  }


The code opens the file and then calls stat(fd, ...) on the path, which introduces a TOCTOU race (the path could change between fopen and stat). Prefer fstat(fileno(ptr), &buf) to get the size/metadata of the already-open file descriptor.

Copilot · 2026-02-04T11:26:18Z

+            uint32_t testId = createApplicationIdentifier();
 	    settings.enableJSDOM = mEnableTestFileDOMSupport;
-	    ApplicationRequest appRequest(id, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);
+	    ApplicationRequest appRequest(testId, RUN, mTestFileName, settings.enableHttp, settings.enableXHR, settings.enableWebSocket, settings.enableWebSocketEnhanced, settings.enableFetch, settings.enableJSDOM, settings.enableWindow, settings.enablePlayer);
 	    NativeJSRenderer::createApplicationInternal(appRequest);


After switching the test ApplicationRequest to use testId, the uint32_t id; declared earlier in run() is no longer used. This can trigger unused-variable warnings (and potentially fail builds under -Werror). Remove the unused id variable or use it consistently.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 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 · 2026-02-04T11:39:42Z

  rtArrayObject* netMetricsArray = new rtArrayObject();

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


netMetricsArray (and other rt*Object instances) are created with new and manually deleted on some error paths, while other parts of the codebase typically use rtRef/rtObjectRef for lifetime management (e.g. src/rtWebSocketServer.cpp:233). Consider converting netMetricsArray to an rtRef<rtArrayObject>/rtObjectRef and relying on RAII instead of manual delete to avoid mismatched ownership semantics.

Copilot · 2026-02-04T11:39:43Z

  int size = buf.st_size;
+  if (size <= 0) {
+    rtLogError("Invalid file size: %d", size);


buf.st_size is an off_t and may be 64-bit; assigning it to int size can truncate/overflow for large files and break allocation/read logic. Use off_t/size_t (and validate bounds) for the size variable.

test commit don't merge

7c4aed4

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

Copilot AI review requested due to automatic review settings February 4, 2026 11:15

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

Copilot AI reviewed Feb 4, 2026

View reviewed changes

Sid2001-maker requested a review from Copilot February 4, 2026 11:22

Copilot started reviewing on behalf of Sid2001-maker February 4, 2026 11:23 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

Sid2001-maker requested a review from Copilot February 4, 2026 11:34

Copilot started reviewing on behalf of Sid2001-maker February 4, 2026 11:34 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

Sid2001-maker closed this Feb 9, 2026

github-actions Bot locked and limited conversation to collaborators Feb 9, 2026

Sid2001-maker deleted the temp/RDKEMW-12252 branch February 9, 2026 06:42

	const char *fd = args[0].toString().cString();
	rtString filePath = args[0].toString();
	const char *fd = filePath.cString();

Uh oh!

Conversation

Sid2001-maker commented Feb 4, 2026

Uh oh!

github-actions Bot commented Feb 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants