Fixing the issue medium level issue[Do not merge]#118
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
|
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
There was a problem hiding this comment.
Pull request overview
This PR appears to be a “hardening” pass across the JS runtime and related utilities (CLI parsing, curl setup, threading/mutex usage, and a few safety fixes).
Changes:
- Add input validation/sanitization for the
--displayCLI flag and wrap some entrypoints intry/catch. - Improve concurrency safety in several places (mutex usage in dispatch, atomic application IDs, condition-variable predicates).
- Add more defensive checks/cleanup (curl option error checks, metrics cleanup, null-guard before calling dynamic player bindings).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/jsruntime.cpp | Adds --display sanitization and exception handling around main. |
| src/jsc/jsc_lib/jsc_lib.cpp | Improves uncaught-exception logging and adds checked curl option setting; minor path handling tweak. |
| src/jsc/JavaScriptUtils.cpp | Fixes dispatch locking pattern; improves file read error handling; adds some cleanup on early returns. |
| src/jsc/JavaScriptEngine.cpp | Fixes log formatting for a floating-point value. |
| src/jsc/JavaScriptContext.cpp | Adds cleanup for mNetworkMetricsData and guards dynamic binding invocation. |
| src/NativeJSRenderer.cpp | Makes app identifier generation atomic; adjusts locking assumptions; improves console request queue handling; adds checked curl option setting. |
| src/JSRuntimeServer.cpp | Initializes a local variable in JSON parsing helper. |
| src/JSRuntimeClientContainer.cpp | Wraps main in exception handling and tightens error logging. |
| src/JSRuntimeClient.cpp | Uses predicate-based waits and wraps main in exception handling. |
| include/JSRuntimeClient.h | Adds response-received tracking for request/response waiting. |
| include/IJavaScriptContext.h | Adds a virtual destructor for safe polymorphic deletion. |
Comments suppressed due to low confidence (1)
src/JSRuntimeServer.cpp:89
JsonWrap::getUint32usesstd::cerrfor error reporting while the rest of the class usesNativeJSLogger::log, and the message is missing a space ("name << "is""). For consistency and easier operational debugging, use the same logger here and fix the message formatting.
uint32_t res = 0;
cJSON *itm = cJSON_GetObjectItem(mPtr, name);
if (!itm || !cJSON_IsNumber(itm))
{
std::cerr << "Error: " << name << "is not a Uint32_t" << std::endl;
err = true;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool sendCommand(std::string command, std::string &response) | ||
| { | ||
| Derived &derived = static_cast<Derived &>(*this); | ||
| 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), | ||
| [this]() { return mResponseReceived; }); | ||
|
|
||
| if (gotResponse) | ||
| { | ||
| response = mLastResponse; | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
CommandInterface::sendCommand resets mResponseReceived after calling derived.send(). If a response arrives quickly, onMessage() can set/notify before sendCommand() acquires the mutex, and then sendCommand() will overwrite the flag back to false and wait the full timeout (lost wakeup). To fix, set the flag (and ideally clear mLastResponse) while holding mResponseMutex before sending, or hold the lock across the send+wait, or use a monotonically increasing sequence/correlation id instead of a shared boolean flag.
| 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]); |
There was a problem hiding this comment.
The raw --display argument is logged before validation (and also logged again on invalid input). Since the logger uses printf-style formatting, an attacker can inject newlines/control characters into logs even though the value will later be rejected by sanitizeDisplayName(). Consider only logging the sanitized value, or escaping the raw value before logging, and avoid echoing invalid input verbatim.
| 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]); | |
| std::string sanitizedDisplay; | |
| if (!sanitizeDisplayName(argv[i], sanitizedDisplay)) { | |
| NativeJSLogger::log(WARN, "Invalid display name provided. Must be 1-64 characters containing only alphanumeric, hyphens, underscores, and dots\n"); |
test commit do not merge