Skip to content

FIx High level Coverity issue[Test PR] #112

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Sid2001-maker wants to merge 1 commit into from
Closed

FIx High level Coverity issue[Test PR] #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/IJavaScriptContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
class IJavaScriptContext
{
public:
virtual ~IJavaScriptContext() = default;
virtual bool runScript(const char *script, bool isModule=true, std::string name="", const char *args = nullptr, bool isApplication=false) = 0;
virtual bool runFile(const char *file, const char* args, bool isApplication=false) = 0;
virtual std::string getUrl() = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/JSRuntimeServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class JsonWrap

uint32_t getUint32(const char *name, bool &err)
{
uint32_t res;
uint32_t res = 0;
cJSON *itm = cJSON_GetObjectItem(mPtr, name);
if (!itm || !cJSON_IsNumber(itm))
{
Expand Down
3 changes: 2 additions & 1 deletion src/NativeJSRenderer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,8 +506,9 @@ void NativeJSRenderer::run()
if(!mTestFileName.empty())
{
ModuleSettings settings;
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);
Comment on lines +509 to +511

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
NativeJSRenderer::createApplicationInternal(appRequest);
Comment on lines +509 to 512

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
NativeJSRenderer::runApplicationInternal(appRequest);
mTestFileName = "";
Expand Down
4 changes: 4 additions & 0 deletions src/jsc/JavaScriptContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ if (mModuleSettings.enablePlayer)
mPriv->releaseAllProtected();
JSGlobalContextRelease(mContext);
JSContextGroupRelease(mContextGroup);
if (mNetworkMetricsData) {
delete mNetworkMetricsData;
mNetworkMetricsData = nullptr;
}
rtLogInfo("%s end", __FUNCTION__);
}

Expand Down
62 changes: 51 additions & 11 deletions src/jsc/JavaScriptUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,24 +314,61 @@ rtError rtSetVideoStartTimeBinding(int numArgs, const rtValue* args, rtValue* re

rtError rtReadBinaryBinding(int numArgs, const rtValue* args, rtValue* result, void* context)
{
char *buffer = nullptr;
FILE *ptr = nullptr;
ptr = fopen("hello.wasm","rb"); // r for read, b for binary
UNUSED_PARAM(context);

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

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
FILE *ptr = fopen(fd, "rb");
Comment on lines +319 to +326

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

// 2. Check fopen
if (!ptr) {
rtLogError("Failed to open file: %s", fd);
return RT_ERROR;
}

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

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

stat(fd, &buf);
int size = buf.st_size;
if (size <= 0) {
rtLogError("Invalid file size: %d", size);
Comment on lines 342 to +344

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
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);
Comment on lines 342 to +350

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
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);
}
Comment on lines +358 to 369

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
free(buffer);
Comment on lines +349 to +370

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
fclose(ptr);
return RT_OK;
}

Expand Down Expand Up @@ -693,13 +730,15 @@ rtError rtJSRuntimeDownloadMetrics(int numArgs, const rtValue* args, rtValue* re
rtValue keys;
if (map->Get("allKeys", &keys) != RT_OK) {
rtLogWarn("Could not retrieve url for network metrics data.");
delete netMetricsArray;
return RT_FAIL;
}
rtObjectRef objRef = keys.toObject();
rtArrayObject* keysArray = static_cast<rtArrayObject*>(objRef.getPtr());

if (!keysArray) {
rtLogWarn("No url found in the network metrics data.");
delete netMetricsArray;
return RT_FAIL;
Comment on lines 739 to 742

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}

Expand All @@ -715,6 +754,7 @@ rtError rtJSRuntimeDownloadMetrics(int numArgs, const rtValue* args, rtValue* re
NetworkMetrics* metrics = (NetworkMetrics*)storedValue.toVoidPtr();
if (!metrics) {
rtLogError("Failed to cast stored value to NetworkMetrics structure for url: %s.", key.cString());
delete netMetricsArray;
return RT_FAIL;
Comment on lines 755 to 758

Copilot AI Feb 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
rtMapObject* metricsMap = new rtMapObject();
Expand Down
Loading