-
Notifications
You must be signed in to change notification settings - Fork 0
Fix High and Medium Level issues #110
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses Critical and High level issues identified by Coverity scan (RDKEMW-12252). The changes focus on improving error handling, fixing memory leaks, addressing concurrency issues, and correcting format specifiers.
Changes:
- Added exception handling with try-catch blocks in main functions across multiple files
- Improved error checking for file operations, curl operations, and null pointers
- Fixed memory leaks by properly deallocating resources and adding missing delete statements
- Enhanced thread safety with atomic counters and improved mutex usage patterns
- Corrected format specifiers and log messages to match actual data types
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/jsruntime.cpp | Added exception handling, bounds checking for command-line arguments, and Coverity annotations for tainted data |
| src/jsc/jsc_lib/jsc_lib.cpp | Fixed exception value conversion, added curl error checking with lambda helper, improved string comparison |
| src/jsc/JavaScriptUtils.cpp | Improved mutex locking patterns, added file operation error handling, fixed memory leaks in network metrics |
| src/jsc/JavaScriptEngine.cpp | Corrected format specifier from %d to %f for double variable |
| src/jsc/JavaScriptContext.cpp | Added memory cleanup for mNetworkMetricsData, added null check for gAAMPJSBindings |
| src/NativeJSRenderer.cpp | Replaced non-atomic counter with atomic, improved mutex scoping, added curl error checking, fixed log messages |
| src/JSRuntimeServer.cpp | Initialized res variable to 0 in error case to prevent undefined behavior |
| src/JSRuntimeClientContainer.cpp | Added exception handling to main function, fixed log format string |
| src/JSRuntimeClient.cpp | Added predicates to condition_variable wait_for calls, added exception handling to main |
| include/JSRuntimeClient.h | Added mResponseReceived flag to prevent spurious wakeups in condition_variable |
| include/IJavaScriptContext.h | Added virtual destructor to polymorphic base class |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d8e9c66 to
aa1d794
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/jsc/JavaScriptUtils.cpp:789
- After pushing metricsMap to netMetricsArray at line 788, the metricsMap pointer is not deleted, unlike the pattern seen with headersArray (line 774) and timeMetricsArray (line 786) which are deleted after being pushed. If rtValue does not handle reference counting or take ownership, this could lead to a memory leak. Verify the ownership semantics of rtValue and add appropriate cleanup if needed.
rtMapObject* metricsMap = new rtMapObject();
metricsMap->set("url", metrics->url);
metricsMap->set("method", metrics->method);
rtArrayObject* headersArray = new rtArrayObject();
for (const auto& header : metrics->headers) {
headersArray->pushBack(rtValue(header));
}
metricsMap->set("headers", rtValue(headersArray));
delete headersArray;
rtArrayObject* timeMetricsArray = new rtArrayObject();
for (const auto& metric : metrics->timeMetricsData) {
rtObjectRef timeMetricObj = new rtMapObject();
timeMetricObj->Set(metric.first.cString(), &metric.second);
timeMetricsArray->pushBack(rtValue(timeMetricObj));
delete timeMetricObj;
timeMetricObj = nullptr;
}
metricsMap->set("timeMetricsData", rtValue(timeMetricsArray));
delete timeMetricsArray;
netMetricsArray->pushBack(rtValue(metricsMap));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aa1d794 to
3d0c78b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/jsc/JavaScriptUtils.cpp:786
- Memory leak:
metricsMapis allocated at line 763 but never deleted. After callingpushBackat line 786, themetricsMapobject is still owned by the caller and should be deleted to prevent a memory leak. The same pattern used forheadersArrayandtimeMetricsArrayshould be applied here.
rtMapObject* metricsMap = new rtMapObject();
metricsMap->set("url", metrics->url);
metricsMap->set("method", metrics->method);
rtArrayObject* headersArray = new rtArrayObject();
for (const auto& header : metrics->headers) {
headersArray->pushBack(rtValue(header));
}
metricsMap->set("headers", rtValue(headersArray));
delete headersArray;
rtArrayObject* timeMetricsArray = new rtArrayObject();
for (const auto& metric : metrics->timeMetricsData) {
rtObjectRef timeMetricObj = new rtMapObject();
timeMetricObj->Set(metric.first.cString(), &metric.second);
timeMetricsArray->pushBack(rtValue(timeMetricObj));
delete timeMetricObj;
timeMetricObj = nullptr;
}
metricsMap->set("timeMetricsData", rtValue(timeMetricsArray));
delete timeMetricsArray;
netMetricsArray->pushBack(rtValue(metricsMap));
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3d0c78b to
c88081b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c88081b to
bde12cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (result) | ||
| { | ||
| result->setString(buffer); | ||
| result->setString(buffer.get()); |
Copilot
AI
Feb 3, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffer contains binary data from a WASM file, but setString expects a null-terminated C string. Binary data may contain null bytes in the middle or lack a null terminator, which will cause undefined behavior. Consider using a method that accepts a buffer with explicit size, or ensure proper null termination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
c744166 to
72339bc
Compare
|
I have read the CLA Document and I hereby sign the CLA 2 out of 3 committers have signed the CLA. |
72339bc to
eea14a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/jsc/JavaScriptUtils.cpp
Outdated
|
|
||
| int size = buf.st_size; | ||
|
|
||
| buffer = (char*)malloc(size); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing null check after malloc. The malloc call may return NULL if memory allocation fails, but the code proceeds to use the buffer pointer without checking. This could lead to a null pointer dereference on line 342 if allocation fails. Add a null check after malloc and return RT_ERROR if allocation fails, ensuring proper cleanup of the file pointer.
src/jsc/JavaScriptUtils.cpp
Outdated
| 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); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fread call expects to read 'size' bytes as a single item, but this is backwards. The correct call should be fread(buffer, 1, size, ptr) to read 'size' items of 1 byte each. The current code reads 1 item of 'size' bytes, which will only succeed if the file size is exactly divisible by 'size' and may not read the entire file content correctly.
| 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); |
src/jsc/JavaScriptUtils.cpp
Outdated
| if (bytesRead != 1) | ||
| { | ||
| rtLogError("Failed to read file: expected 1 item, read %zu items", bytesRead); | ||
| free(buffer); | ||
| return RT_ERROR; | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation check is incorrect following the fread parameter correction. If fread(buffer, 1, size, ptr) is used as it should be, the success condition should check if bytesRead equals 'size', not 1. The current check for bytesRead != 1 would be incorrect after fixing the fread parameters.
| @@ -57,7 +58,9 @@ int main(int argc, char* argv[]) | |||
| { | |||
| appendindex = i-1; | |||
| i++; | |||
| waylanddisplay = argv[i]; | |||
| if (i < argc) { | |||
| waylanddisplay = argv[i]; | |||
| } | |||
| } | |||
| else if (strcmp(argv[i], "--enableHttp") == 0) | |||
| { | |||
| @@ -107,16 +110,11 @@ int main(int argc, char* argv[]) | |||
| } | |||
| i++; | |||
| } | |||
|
|
|||
| std::shared_ptr<NativeJSRenderer> renderer = std::make_shared<NativeJSRenderer>(waylanddisplay); | |||
| if (consoleMode) { | |||
| renderer->setEnvForConsoleMode(moduleSettings); | |||
| } | |||
| if (!renderer) | |||
| { | |||
| NativeJSLogger::log(ERROR, "Unable to run application\n"); | |||
| return -1; | |||
| } | |||
|
|
|||
| #if defined(ENABLE_JSRUNTIME_SERVER) | |||
| if (runServer == true) | |||
| @@ -172,6 +170,13 @@ int main(int argc, char* argv[]) | |||
| } | |||
|
|
|||
| return 0; | |||
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try block starting on line 40 is not properly indented. All code inside the try block should be indented consistently. Currently, the code starting from line 41 onwards has inconsistent indentation, making the code structure unclear.
src/NativeJSRenderer.cpp
Outdated
| 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()); |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential undefined behavior: accessing mContextMap[id].url when id is not in the map. The else branch at line 445 is executed when the id is not found in mContextMap (line 433), but line 447 tries to access mContextMap[id].url, which will create a new entry with default values if the id doesn't exist. This could lead to accessing uninitialized data or creating phantom entries. The error message should either not include the URL or store it before checking if the id exists.
src/jsc/JavaScriptUtils.cpp
Outdated
| rtValue keys; | ||
| if (map->Get("allKeys", &keys) != RT_OK) { | ||
| rtLogWarn("Could not retrieve url for network metrics data."); | ||
| delete netMetricsArray; //newly added |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline comment "newly added" is unnecessary and should be removed. Such comments don't add value to the code and become outdated over time. The version control system already tracks what was added and when.
src/jsc/JavaScriptUtils.cpp
Outdated
|
|
||
| if (!keysArray) { | ||
| rtLogWarn("No url found in the network metrics data."); | ||
| delete netMetricsArray; //newly added |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline comment "newly added" is unnecessary and should be removed. Such comments don't add value to the code and become outdated over time. The version control system already tracks what was added and when.
src/jsc/JavaScriptContext.cpp
Outdated
| } | ||
| mPriv->releaseAllProtected(); | ||
|
|
||
| //changed added |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline comment "changed added" is unclear and should be removed. Such comments don't add value to the code and become outdated over time. The version control system already tracks what was added and when.
| //changed added |
| if (stat(fd, &buf) != 0) | ||
| { | ||
| rtLogError("Failed to stat file: %s", fd); | ||
| fclose(ptr); | ||
| return RT_ERROR; | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempting to close a potentially null file pointer. The fopen call on line 327 may return NULL if the file cannot be opened, but the code proceeds to call fclose(ptr) without checking if ptr is null. This will cause undefined behavior if the file doesn't exist or can't be opened. Add a null check after fopen and return an error if the file couldn't be opened.
src/jsc/JavaScriptUtils.cpp
Outdated
| NetworkMetrics* metrics = (NetworkMetrics*)storedValue.toVoidPtr(); | ||
| if (!metrics) { | ||
| rtLogError("Failed to cast stored value to NetworkMetrics structure for url: %s.", key.cString()); | ||
| delete netMetricsArray; //newly added |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline comment "newly added" is unnecessary and should be removed. Such comments don't add value to the code and become outdated over time. The version control system already tracks what was added and when.
| delete netMetricsArray; //newly added | |
| delete netMetricsArray; |
eea14a4 to
c744166
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
|
||
| if (!ptr) | ||
| { | ||
| rtLogError("Failed to open file: hello.wasm"); | ||
| return RT_ERROR; | ||
| } | ||
|
|
||
| const char *fd = "hello.wasm"; | ||
| struct stat buf; | ||
|
|
||
| stat(fd, &buf); | ||
| int size = buf.st_size; | ||
| if (stat(fd, &buf) != 0) |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file pointer 'ptr' could be NULL if fopen fails. The code checks stat() result but calls fclose(ptr) without checking if ptr is NULL. This will cause undefined behavior if the file cannot be opened. Add a NULL check for ptr before attempting to close it, or better yet, check if ptr is NULL immediately after fopen and handle the error appropriately.
| return RT_ERROR; | ||
| } | ||
|
|
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a resource leak if malloc fails. When buffer allocation fails, the function continues to call fread without checking if buffer is NULL, and ptr remains open without being closed. Add a NULL check after malloc and properly handle the error by closing ptr and returning an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/NativeJSRenderer.cpp:480
NativeJSRenderer::run()holdsmUserMutexwhile dispatching each pending request. The called handlers (e.g.,runApplicationInternal()-> networkdownloadFile()+runScript()/runFile()) can be long-running, which blocks producers that needmUserMutex(create/run/terminate requests) and increases deadlock risk if any callback path tries to acquire the same mutex. Consider swappinggPendingRequestsinto a local vector under the lock, unlocking, and then processing requests without holdingmUserMutex, only re-locking around minimal shared-state updates (e.g.,mContextMap).
mUserMutex.lock();
for (int i=0; i<gPendingRequests.size(); i++)
{
ApplicationRequest& request = gPendingRequests[i];
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (derived.send(command)) | ||
| { | ||
| std::unique_lock<std::mutex> lock(mResponseMutex); | ||
| mResponseCondition.wait_for(lock, std::chrono::seconds(5)); | ||
| response = mLastResponse; | ||
| return true; | ||
| mResponseReceived = false; | ||
| bool gotResponse = mResponseCondition.wait_for(lock, std::chrono::seconds(5), |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CommandInterface::sendCommand(), derived.send(command) happens before mResponseReceived is reset under mResponseMutex. If a response arrives quickly, onMessage() can set mResponseReceived=true and notify before sendCommand() takes the lock, and then sendCommand() overwrites the flag back to false and waits the full timeout (missed wakeup). Reset the flag (or increment a sequence counter) while holding mResponseMutex before sending, and consider clearing/consuming mLastResponse in the same critical section.
| bool ret = mConsoleState->consoleContext->runScript(mConsoleState->codeToExecute.front().c_str(), false); | ||
| // Process items from local queue (no race condition) | ||
| for (const auto& code : localQueue) { | ||
| bool ret = mConsoleState->consoleContext->runScript(code.c_str(), false); |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processDevConsoleRequests() assigns the return value of runScript() to ret, but ret is never used. This will trigger an unused-variable warning (and may fail builds if warnings are treated as errors). Either remove ret or use it (e.g., log/report execution failures).
| bool ret = mConsoleState->consoleContext->runScript(code.c_str(), false); | |
| mConsoleState->consoleContext->runScript(code.c_str(), false); |
src/jsruntime.cpp
Outdated
| NativeJSLogger::log(INFO, "Received display argument: '%s'\n", argv[i]); | ||
| std::string sanitizedDisplay; | ||
| if (!sanitizeDisplayName(argv[i], sanitizedDisplay)) { | ||
| NativeJSLogger::log(WARN, "Invalid display name '%s'. Must be 1-64 characters containing only alphanumeric, hyphens, underscores, and dots\n", argv[i]); |
Copilot
AI
Feb 11, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The --display value is logged verbatim before validation/sanitization. Since this comes from argv, it can contain control characters/newlines and lead to log injection or confusing logs. Prefer logging only the sanitized value, or escape/quote non-printable characters when logging rejected input.
c2a8496 to
4de5f6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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