Fixing CID:430751[do not merge]#117
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
Fixes a medium-severity Coverity finding (CID:430751) by addressing several initialization, locking, and error-handling issues across the JS runtime executable, client/server utilities, and JSC integration.
Changes:
- Adds safer control flow/error handling (try/catch in
main, safer waits with predicates, safer JSON parsing default init). - Fixes synchronization correctness (proper mutex scoping in
dispatchPending, atomic application ID generation, dev-console queue handling). - Improves robustness of external I/O setup (validates
--display, checkscurl_easy_setoptreturn codes, adds file/stat/read checks, avoids some null dereferences).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/jsruntime.cpp | Wraps main with exception handling; validates --display argument before passing to renderer. |
| src/jsc/jsc_lib/jsc_lib.cpp | Fixes uncaught exception logging string conversion; checks curl_easy_setopt return codes; safer trailing-slash check. |
| src/jsc/JavaScriptUtils.cpp | Fixes incorrect mutex usage in dispatchPending; adds Coverity annotations; hardens file read + adds cleanup in early returns. |
| src/jsc/JavaScriptEngine.cpp | Fixes log format specifier for GC interval. |
| src/jsc/JavaScriptContext.cpp | Frees mNetworkMetricsData in destructor; null-checks gAAMPJSBindings before use. |
| src/NativeJSRenderer.cpp | Makes application ID generation thread-safe; improves locking assumptions; reduces race window in dev-console queue processing; checks curl option setting. |
| src/JSRuntimeServer.cpp | Initializes JsonWrap::getUint32 local to avoid uninitialized use. |
| src/JSRuntimeClientContainer.cpp | Wraps main with exception handling; normalizes log newline. |
| src/JSRuntimeClient.cpp | Uses predicate wait to avoid spurious wakeups; wraps main with exception handling. |
| include/JSRuntimeClient.h | Adds response-received flag + predicate wait to avoid spurious wakeups. |
| include/IJavaScriptContext.h | Adds virtual destructor for interface safety. |
💡 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.
Race condition: sendCommand() resets mResponseReceived after derived.send() returns. If onMessage() runs between send() and acquiring mResponseMutex, it can set mResponseReceived=true and notify, then sendCommand() overwrites it back to false and waits until timeout. Reset the flag (and ideally clear/track the response) before sending, or use a monotonically increasing request/response sequence number so early responses aren’t lost.
fixing medium level coverity issue CID:430751