maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix crash on exit introduced in ac265aa (#10042)

Browse source 
## Summary of the Pull Request

ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer,
which TermControl owns. We must ensure that we first destroy the
ControlCore before the UiaEngine instance (both owned by TermControl).
Otherwise a deallocated IRenderEngine is accessed when
ControlCore calls Renderer::TriggerTeardown.

## References

This crash was introduced in #10031.

## PR Checklist
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed

* Run accevent.exe to cause a UiaEngine to be attached to a TermControl.
* Close the current tab
* Ensured no crashes occur
This commit is contained in:
Leonard Hecker 2021-05-12 01:03:08 +02:00 committed by GitHub
parent 440b626ee3
commit 43040ef9d0
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
3 changed files with 21 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
src/cascadia/TerminalControl/ControlCore.cpp
View file

@ -101,27 +101,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
Close();
// Before destroying this instance we must ensure that we destroy the _renderer
// before the _renderEngine, as well as calling _renderer->TriggerTeardown().
// _renderEngine will be destroyed naturally after this ~destructor() returns.
decltype(_renderer) renderer;
if (_renderer)
{
// GH#8734:
// We lock the terminal here to make sure it isn't still being
// used in the connection thread before we destroy the renderer.
// However, we must unlock it again prior to triggering the
// teardown, to avoid the render thread being deadlocked. The
// renderer may be waiting to acquire the terminal lock, while
// we're waiting for the renderer to finish.
auto lock = _terminal->LockForWriting();
_renderer.swap(renderer);
}
if (renderer)
{
renderer->TriggerTeardown();
_renderer->TriggerTeardown();
}
}

7
src/cascadia/TerminalControl/ControlCore.h
View file

@ -172,8 +172,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
std::unique_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };
std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr };
// NOTE: _renderEngine must be ordered before _renderer.
//
// As _renderer has a dependency on _renderEngine (through a raw pointer)
// we must ensure the _renderer is deallocated first.
// (C++ class members are destroyed in reverse order.)
std::unique_ptr<::Microsoft::Console::Render::DxEngine> _renderEngine{ nullptr };
std::unique_ptr<::Microsoft::Console::Render::Renderer> _renderer{ nullptr };
IControlSettings _settings{ nullptr };

22
src/cascadia/TerminalControl/TermControl.h
View file

@ -130,22 +130,26 @@ namespace winrt::Microsoft::Terminal::Control::implementation
private:
friend struct TermControlT<TermControl>; // friend our parent so it can bind private event handlers
winrt::com_ptr<ControlCore> _core;
winrt::com_ptr<ControlInteractivity> _interactivity;
bool _initializedTerminal;
winrt::com_ptr<SearchBoxControl> _searchBox;
// NOTE: _uiaEngine must be ordered before _core.
//
// ControlCore::AttachUiaEngine receives a IRenderEngine as a raw pointer, which we own.
// We must ensure that we first destroy the ControlCore before the UiaEngine instance
// in order to safely resolve this unsafe pointer dependency. Otherwise a deallocated
// IRenderEngine is accessed when ControlCore calls Renderer::TriggerTeardown.
// (C++ class members are destroyed in reverse order.)
std::unique_ptr<::Microsoft::Console::Render::UiaEngine> _uiaEngine;
winrt::com_ptr<ControlCore> _core;
winrt::com_ptr<ControlInteractivity> _interactivity;
winrt::com_ptr<SearchBoxControl> _searchBox;
IControlSettings _settings;
bool _focused;
std::atomic<bool> _closing;
bool _focused;
bool _initializedTerminal;
std::shared_ptr<ThrottledFunc<>> _tsfTryRedrawCanvas;
std::shared_ptr<ThrottledFunc<>> _updatePatternLocations;
std::shared_ptr<ThrottledFunc<>> _playWarningBell;
struct ScrollBarUpdate