From 2c7b18f377ffb6f55705298609eee2742411bc5e Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 9 Dec 2021 01:38:50 +0100 Subject: [PATCH] Fix hashing of NewTerminalArgs (#11905) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 00fe2b0 made the mistake of hashing the pointer of `NewTerminalArgs` instances instead of their content. This commit calls `NewTerminalArgs::Hash` instead. ## PR Checklist * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * New test passes ✅ --- .../KeyBindingsTests.cpp | 17 ++-- .../TerminalSettingsModel/ActionArgs.h | 44 ++++++--- .../TerminalSettingsModel/ActionArgs.idl | 4 +- .../TerminalSettingsModel/ActionArgsMagic.h | 98 +++++++++---------- .../TerminalSettingsModel/ActionMap.cpp | 27 ++--- .../TerminalSettingsModel/HashUtils.h | 9 -- 6 files changed, 108 insertions(+), 91 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp index ad7a2011323..b2e77f5b4e6 100644 --- a/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/KeyBindingsTests.cpp @@ -39,24 +39,17 @@ namespace SettingsModelLocalTests TEST_METHOD(KeyChords); TEST_METHOD(ManyKeysSameAction); TEST_METHOD(LayerKeybindings); + TEST_METHOD(HashDeduplication); TEST_METHOD(UnbindKeybindings); - TEST_METHOD(LayerScancodeKeybindings); - TEST_METHOD(TestExplicitUnbind); - TEST_METHOD(TestArbitraryArgs); TEST_METHOD(TestSplitPaneArgs); - TEST_METHOD(TestStringOverload); - TEST_METHOD(TestSetTabColorArgs); - TEST_METHOD(TestScrollArgs); - TEST_METHOD(TestToggleCommandPaletteArgs); TEST_METHOD(TestMoveTabArgs); - TEST_METHOD(TestGetKeyBindingForAction); TEST_METHOD(KeybindingsWithoutVkey); }; @@ -173,6 +166,14 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); } + void KeyBindingsTests::HashDeduplication() + { + const auto actionMap = winrt::make_self(); + actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])")); + actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])")); + VERIFY_ARE_EQUAL(1u, actionMap->_ActionMap.size()); + } + void KeyBindingsTests::UnbindKeybindings() { const std::string bindings0String{ R"([ { "command": "copy", "keys": ["ctrl+c"] } ])" }; diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.h b/src/cascadia/TerminalSettingsModel/ActionArgs.h index f5cf473d5cc..e578afb4390 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.h +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.h @@ -303,9 +303,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation copy->_ColorScheme = _ColorScheme; return *copy; } - size_t Hash(uint64_t hasherState) const + size_t Hash() const + { + til::hasher h; + Hash(h); + return h.finalize(); + } + void Hash(til::hasher& h) const { - til::hasher h{ gsl::narrow_cast(hasherState) }; h.write(Commandline()); h.write(StartingDirectory()); h.write(TabTitle()); @@ -314,10 +319,27 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation h.write(Profile()); h.write(SuppressApplicationTitle()); h.write(ColorScheme()); - return h.finalize(); } }; +} + +template<> +struct til::hash_trait +{ + using M = winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs; + using I = winrt::Microsoft::Terminal::Settings::Model::implementation::NewTerminalArgs; + void operator()(hasher& h, const M& value) const noexcept + { + if (value) + { + winrt::get_self(value)->Hash(h); + } + } +}; + +namespace winrt::Microsoft::Terminal::Settings::Model::implementation +{ // New Tabs, Panes, and Windows all use NewTerminalArgs, which is more // complicated and doesn't play nice with the macro. So those we'll still // have to define manually. @@ -363,9 +385,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation copy->_TerminalArgs = _TerminalArgs.Copy(); return *copy; } - size_t Hash(uint64_t hasherState) const + size_t Hash() const { - til::hasher h{ gsl::narrow_cast(hasherState) }; + til::hasher h; h.write(TerminalArgs()); return h.finalize(); } @@ -449,9 +471,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation copy->_SplitSize = _SplitSize; return *copy; } - size_t Hash(uint64_t hasherState) const + size_t Hash() const { - til::hasher h{ gsl::narrow_cast(hasherState) }; + til::hasher h; h.write(SplitDirection()); h.write(TerminalArgs()); h.write(SplitMode()); @@ -501,9 +523,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation copy->_TerminalArgs = _TerminalArgs.Copy(); return *copy; } - size_t Hash(uint64_t hasherState) const + size_t Hash() const { - til::hasher h{ gsl::narrow_cast(hasherState) }; + til::hasher h; h.write(TerminalArgs()); return h.finalize(); } @@ -622,9 +644,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation copy->_Actions = _Actions; return *copy; } - size_t Hash(uint64_t hasherState) const + size_t Hash() const { - til::hasher h{ gsl::narrow_cast(hasherState) }; + til::hasher h; h.write(winrt::get_abi(_Actions)); return h.finalize(); } diff --git a/src/cascadia/TerminalSettingsModel/ActionArgs.idl b/src/cascadia/TerminalSettingsModel/ActionArgs.idl index b0edde61509..457feefced7 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgs.idl +++ b/src/cascadia/TerminalSettingsModel/ActionArgs.idl @@ -10,7 +10,7 @@ namespace Microsoft.Terminal.Settings.Model Boolean Equals(IActionArgs other); String GenerateName(); IActionArgs Copy(); - UInt64 Hash(UInt64 hasher); + UInt64 Hash(); }; interface IActionEventArgs @@ -128,7 +128,7 @@ namespace Microsoft.Terminal.Settings.Model Boolean Equals(NewTerminalArgs other); String GenerateName(); String ToCommandline(); - UInt64 Hash(UInt64 hasher); + UInt64 Hash(); }; [default_interface] runtimeclass ActionEventArgs : IActionEventArgs diff --git a/src/cascadia/TerminalSettingsModel/ActionArgsMagic.h b/src/cascadia/TerminalSettingsModel/ActionArgsMagic.h index 708deb93280..506083428f7 100644 --- a/src/cascadia/TerminalSettingsModel/ActionArgsMagic.h +++ b/src/cascadia/TerminalSettingsModel/ActionArgsMagic.h @@ -111,53 +111,53 @@ struct InitListPlaceholder // * NewTerminalArgs has a ToCommandline method it needs to additionally declare. // * GlobalSummonArgs has the QuakeModeFromJson helper -#define ACTION_ARG_BODY(className, argsMacro) \ - className() = default; \ - className( \ - argsMacro(CTOR_PARAMS) InitListPlaceholder = {}) : \ - argsMacro(CTOR_INIT) _placeholder{} {}; \ - argsMacro(DECLARE_ARGS); \ - \ -private: \ - InitListPlaceholder _placeholder; \ - \ -public: \ - hstring GenerateName() const; \ - bool Equals(const IActionArgs& other) \ - { \ - auto otherAsUs = other.try_as(); \ - if (otherAsUs) \ - { \ - return true argsMacro(EQUALS_ARGS); \ - } \ - return false; \ - }; \ - static FromJsonResult FromJson(const Json::Value& json) \ - { \ - auto args = winrt::make_self(); \ - argsMacro(FROM_JSON_ARGS); \ - return { *args, {} }; \ - } \ - static Json::Value ToJson(const IActionArgs& val) \ - { \ - if (!val) \ - { \ - return {}; \ - } \ - Json::Value json{ Json::ValueType::objectValue }; \ - const auto args{ get_self(val) }; \ - argsMacro(TO_JSON_ARGS); \ - return json; \ - } \ - IActionArgs Copy() const \ - { \ - auto copy{ winrt::make_self() }; \ - argsMacro(COPY_ARGS); \ - return *copy; \ - } \ - size_t Hash(uint64_t hasherState) const \ - { \ - til::hasher h{ gsl::narrow_cast(hasherState) }; \ - argsMacro(HASH_ARGS); \ - return h.finalize(); \ +#define ACTION_ARG_BODY(className, argsMacro) \ + className() = default; \ + className( \ + argsMacro(CTOR_PARAMS) InitListPlaceholder = {}) : \ + argsMacro(CTOR_INIT) _placeholder{} {}; \ + argsMacro(DECLARE_ARGS); \ + \ +private: \ + InitListPlaceholder _placeholder; \ + \ +public: \ + hstring GenerateName() const; \ + bool Equals(const IActionArgs& other) \ + { \ + auto otherAsUs = other.try_as(); \ + if (otherAsUs) \ + { \ + return true argsMacro(EQUALS_ARGS); \ + } \ + return false; \ + }; \ + static FromJsonResult FromJson(const Json::Value& json) \ + { \ + auto args = winrt::make_self(); \ + argsMacro(FROM_JSON_ARGS); \ + return { *args, {} }; \ + } \ + static Json::Value ToJson(const IActionArgs& val) \ + { \ + if (!val) \ + { \ + return {}; \ + } \ + Json::Value json{ Json::ValueType::objectValue }; \ + const auto args{ get_self(val) }; \ + argsMacro(TO_JSON_ARGS); \ + return json; \ + } \ + IActionArgs Copy() const \ + { \ + auto copy{ winrt::make_self() }; \ + argsMacro(COPY_ARGS); \ + return *copy; \ + } \ + size_t Hash() const \ + { \ + til::hasher h; \ + argsMacro(HASH_ARGS); \ + return h.finalize(); \ } diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 33d36746a3f..f332e3b220b 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -15,13 +15,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { static InternalActionID Hash(const Model::ActionAndArgs& actionAndArgs) { - static constexpr auto initialHash = til::hasher{}.finalize(); - const auto action = actionAndArgs.Action(); til::hasher hasher; + // action will be hashed last. + // This allows us to first seed a til::hasher + // with the return value of IActionArgs::Hash(). + const auto action = actionAndArgs.Action(); + if (const auto args = actionAndArgs.Args()) { - hasher = til::hasher{ gsl::narrow_cast(args.Hash(initialHash)) }; + hasher = til::hasher{ gsl::narrow_cast(args.Hash()) }; } else { @@ -29,16 +32,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Args are not defined. // Check if the ShortcutAction supports args. - switch (actionAndArgs.Action()) + switch (action) { -#define ON_ALL_ACTIONS_WITH_ARGS(action) \ - case ShortcutAction::action: \ - { \ - /* If it does, hash the default values for the args. */ \ - static const auto cachedHash = gsl::narrow_cast( \ - winrt::make_self()->Hash(initialHash)); \ - hash = cachedHash; \ - break; \ +#define ON_ALL_ACTIONS_WITH_ARGS(action) \ + case ShortcutAction::action: \ + { \ + /* If it does, hash the default values for the args. */ \ + static const auto cachedHash = gsl::narrow_cast( \ + winrt::make_self()->Hash()); \ + hash = cachedHash; \ + break; \ } ALL_SHORTCUT_ACTIONS_WITH_ARGS #undef ON_ALL_ACTIONS_WITH_ARGS diff --git a/src/cascadia/TerminalSettingsModel/HashUtils.h b/src/cascadia/TerminalSettingsModel/HashUtils.h index 9079b5de66a..8a73816be33 100644 --- a/src/cascadia/TerminalSettingsModel/HashUtils.h +++ b/src/cascadia/TerminalSettingsModel/HashUtils.h @@ -33,15 +33,6 @@ namespace til } }; - template<> - struct hash_trait - { - void operator()(hasher& h, const winrt::Microsoft::Terminal::Settings::Model::NewTerminalArgs& value) const noexcept - { - h.write(winrt::get_abi(value)); - } - }; - template<> struct hash_trait {