UAF in gin_helper::CallbackHolderBase::SecondWeakCallback (integration test crash) #192119
Comments
bpasero
added
the
freeze-slow-crash-leak
|
deepak1556
added
bug
deepak1556
changed the title
roblourens
changed the title
|
Crash keys added to help narrow down the source of crash https://domoreexp.visualstudio.com/Teamspace/_git/electron-build/pullrequest/901475 |
|
Had a new failure with Electron 29 as well, refs https://dev.azure.com/monacotools/a6d41577-0fa3-498e-af22-257312ff0545/_build/results?buildId=277418 Sadly I missed porting the gin annotations to 29 branch |
|
Seems like the crash rate of this particular one has increased again, adding back the crash keys again. https://dev.azure.com/monacotools/a6d41577-0fa3-498e-af22-257312ff0545/_build/results?buildId=285035 |
|
Added crash keys and triggered test builds to debug further. Work can be tracked in https://github.com/microsoft/vscode/tree/robo/debug_integration_crash |
|
I have now tracked down the source of crash, but before I get down into the final details lets walk through the nature of the crash and understand what is happening. The crash started appearing a year ago in our CI mainly with integration tests across all 3 platforms inconsistently. Sometimes the crash would disappear after runtime updates but recently it has been pretty consistent in our Windows CI. Although I was unsuccessful to repro it locally but given its frequent presence in CI I decided to rely on Crash keys which are meant for cases like this. First, where are these crashes happening ?On shutdown of the main process, the runtime eventually has to gracefully cleanup the Node.js environment and its associated tasks from the event loop https://github.com/electron/electron/blob/15db63e26df3e3d59ce6281f030624f746518511/shell/browser/electron_browser_main_parts.cc#L595-L601 which in rare cases lead to UAF in certain The initial reports point to the resource cleanup in gin_helper::CallbackHolderBase::SecondWeakCallback Newer reports point to the resource cleanup in gin::WrappableBase::SecondWeakCallback So what are these affected classes and the weak callbacks ?The runtime exposes JS api that are backed by C++ implementations through V8. const app = {
isHidden: false,
hide: () => {
isHidden = true;
}
}
const a = Object.create(app);This is how we would do the same in C++ and expose it through V8 class App : gin::Wrappable<App> {
public:
static gin::WrapperInfo kWrapperInfo; // important will be covered below
gin::ObjectTemplateBuilder GetObjectTemplateBuilder(v8::Isolate* isolate) override {
return gin::Wrappable<App>::GetObjectTemplateBuilder(isolate)
.setMethod("hide", &App::Hide)
.setProperty("isHidden", &App::isHidden);
}
private:
bool isHidden() const;
void Hide();
}The work to construct an object instance from this C++ class will eventually be performed by gin::Wrappable::GetWrapperImpl.
Now going back to the implementation of What could cause these use-after-free ?
The order of destruction between The initial reports which don't appear now could be attributed to consuming the fix from https://chromium-review.googlesource.com/c/chromium/src/+/5104930 and electron/electron#41534. I can only speculate about this as there is not much we can understand from the old reports.
But there is another scenario of destruction happening during shutdown that is not captured. This is where crash keys come in handy. The keys I added are in https://gist.github.com/deepak1556/f9682fb1b94aa53d9cf37e83638c226d
ResultAfter a couple of retries in the CI, I was able to trigger the crash https://monacotools.visualstudio.com/Monaco/_build/results?buildId=295068&view=results. As expected I had a bunch of dumps from 
Run
So we had a class in Electron that worked in theory similar to Proposed fix
WrappableBase* wrappable = data.GetParameter() // can now be nullptr when weakness is cleared via https://source.chromium.org/chromium/chromium/src/+/main:v8/src/handles/global-handles.cc;l=521-527
if (wrappable) {
delete wrappable;
}What is the future ?If you have made it this far, hope you enjoyed this debugging session! Now this disjoint between the C++ heap and Javascript heap leading to these uaf issues is not new. The V8 team had already worked on addressing it for the Blink rendering engine via the project called Oilpan and it is also available as a standalone library for embedding in any C++ project that uses V8 https://github.com/oilpan-gc/cppgc. Through this library, the trace based garbage collector can now view both the C++ heap and Javascript heap together to help make lifetime management simpler. IIUC, |
vs-code-engineering
bot
added
unreleased
https://dev.azure.com/vscode/VSCode/_build/results?buildId=102349&view=results
crash-dump-linux-x64-integration-1.zip
The text was updated successfully, but these errors were encountered: