[SERVER-29666] Interrupt signal should cause localConnectForDbEval to fail Created: 15/Jun/17 Updated: 27/Oct/23 Resolved: 27/Oct/17 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | JavaScript |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Bug | Priority: | Trivial - P5 |
| Reporter: | Spencer Jackson | Assignee: | Spencer Jackson |
| Resolution: | Gone away | Votes: | 0 |
| Labels: | neweng, platforms-interns-2017 | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||
| Operating System: | ALL | ||||||||||||
| Participants: | |||||||||||||
| Linked BF Score: | 0 | ||||||||||||
| Description |
|
localConnectForDbEval makes multiple calls to 'exec', without checking whether they succeeded or failed. If an interrupt is triggered, the JS engine will cleanup the scope, causing one of the exec calls to fail. The next call to exec may access cleaned up resources. A failing invokation of exec should cause the whole caller to fail. |
| Comments |
| Comment by Spencer Jackson [ 27/Oct/17 ] |
|
ted.tuckman is correct. At the time of writing the exec functions in the file threw on errors, so they could not have been the JS invocations which caused the issue. A closer look at the logs reveals the following error right before the access:
MozJSImplScope calls into JS in order to parse a function's AST. "Unable to load stored JavaScript function" comes from loading system.js, the "Failed to call method" happens when Scope::loadStored calls into the chain MozJSImplScope::setElement() The changes in |
| Comment by Ted Tuckman [ 26/Jul/17 ] |
|
It seems like this has gone away. In localConnectForDbEval, we call exec with assertOnError = true. This causes exec to throw if it has a not OK status when it calls _checkErrorState. In the linked ticket, localConnect was wrapped with a function that should handle exceptions correctly and pass them on to the calling function. If exec fails it should throw and be handled now, instead of running the second exec. It is also unclear to me if the second exec would have been called before this change, seeing as an exception would have been thrown if the first one fails. If the status is not set by a failing exec and it sends a Status::OK to _checkErrorState regardless of success/failure, then it would continue and would need another solution. |