From cd35bc5989305c191299e576e23b59db10403451 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett" Date: Thu, 9 Jun 2022 14:48:28 -0500 Subject: [PATCH 1/2] Restore OpenConsoleProxyStub's dependency on the CRT to fix QI (#13254) When #13160 introduced a new interface to the IConsoleHandoff idl, it changed midl's RPC proxy stub lookup algorithm from a direct GUID comparison to an unrolled binary search. Now, that would ordinarily not be a problem... However, in #11610, we took a shortcut and replaced `memcmp` -- used only by RPC for GUID comparison -- with a direct GUID-only equality comparator. This worked totally fine, and ordinarily would not be a problem... The unrolled binary search unfortunately _relies on memcmp's contract_: it uses memcmp to match against a fully sorted set. Our memcmp only returned 0 or 1 (equal or not), and it knew nothing about ordering. When a package that contains a PackagedCOM proxy stub is installed, it is selected as the primary proxy stub for any interfaces it can proxy. After all, interfaces are immutable, so it doesn't matter whose proxy you're using. Now, given that we installed a *broken* proxy... *all* IIDs that got whacked by our memcmp issue broke for every consumer. To fix it: instead of implementing memcmp ourselves, we're just going to take a page out of WinAppSDK's book and link this binary using the "Hybrid CRT" model. It will statically link any parts of the STL it uses (none) and dynamically link the ucrt (which is guaranteed to be present on Windows.) Sure, the binary size goes up from 8k to 24k, but... the cost is never having to worry about this again. Closes #13251 --- .github/actions/spelling/expect/expect.txt | 2 - src/host/proxy/Host.Proxy.vcxproj | 56 ++++++++++++++++------ src/host/proxy/Host.Proxy.vcxproj.filters | 3 -- src/host/proxy/nodefaultlib_shim.h | 8 ---- 4 files changed, 41 insertions(+), 28 deletions(-) delete mode 100644 src/host/proxy/nodefaultlib_shim.h diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index dd4342d9d25..6e4f259dcea 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -959,7 +959,6 @@ guardxfg guc gui guidatom -guiddef GValue GWL GWLP @@ -1567,7 +1566,6 @@ NOCOLOR NOCOMM NOCONTEXTHELP NOCOPYBITS -nodefaultlib nodiscard NODUP noexcept diff --git a/src/host/proxy/Host.Proxy.vcxproj b/src/host/proxy/Host.Proxy.vcxproj index dd0551e104d..fc8ff483073 100644 --- a/src/host/proxy/Host.Proxy.vcxproj +++ b/src/host/proxy/Host.Proxy.vcxproj @@ -21,7 +21,7 @@ - REGISTER_PROXY_DLL;WIN32;%(PreprocessorDefinitions) NotUsing - Default - false - false - nodefaultlib_shim.h;%(ForcedIncludeFiles) OpenConsoleProxy.def - - - true - DllMain + + + + + + MultiThreadedDebug + + + + %(IgnoreSpecificDefaultLibraries);libucrtd.lib + %(AdditionalOptions) /defaultlib:ucrtd.lib + + + + + + MultiThreaded + + + + %(IgnoreSpecificDefaultLibraries);libucrt.lib + %(AdditionalOptions) /defaultlib:ucrt.lib + + + diff --git a/src/host/proxy/Host.Proxy.vcxproj.filters b/src/host/proxy/Host.Proxy.vcxproj.filters index 1cdf7ca2512..df6bcbd10c6 100644 --- a/src/host/proxy/Host.Proxy.vcxproj.filters +++ b/src/host/proxy/Host.Proxy.vcxproj.filters @@ -41,9 +41,6 @@ Header Files - - Header Files - diff --git a/src/host/proxy/nodefaultlib_shim.h b/src/host/proxy/nodefaultlib_shim.h deleted file mode 100644 index 4face4e49f2..00000000000 --- a/src/host/proxy/nodefaultlib_shim.h +++ /dev/null @@ -1,8 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -#pragma once - -#include - -#define memcmp(a, b, c) (!InlineIsEqualGUID(a, b)) From 730eb5fafd1914fd615b0a87446756162eb2f82b Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 9 Jun 2022 21:55:04 +0200 Subject: [PATCH 2/2] Fix signed/unsigned arithmetic bug during scrolling (#13256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ed27737 contains a regression where (pseudocode) ```c unsigned long ulActualDelta; short ScreenInfo.WheelDelta; delta *= (ScreenInfo.WheelDelta / (short)ulActualDelta); // ^^^^^^^ ``` was changed to ```c delta *= (ScreenInfo.WheelDelta / ulActualDelta); ``` Due to `ulActualDelta` being unsigned, the new code casts the signed integer to a unsigned one first, before doing the division. This causes scrolling downwards (`WheelDelta` is negative) to appear as a large positive `delta`. ## PR Checklist * [x] Closes #13253 * [x] I work here ## Validation Steps Performed * Scrolling up/down works in OpenConsole again ✅ --- src/host/scrolling.cpp | 18 +++++++++--------- src/host/scrolling.hpp | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/host/scrolling.cpp b/src/host/scrolling.cpp index 6d211a0fd3d..8e7b122733b 100644 --- a/src/host/scrolling.cpp +++ b/src/host/scrolling.cpp @@ -13,13 +13,13 @@ using Microsoft::Console::VirtualTerminal::StateMachine; using namespace Microsoft::Console::Interactivity; using namespace Microsoft::Console::Types; -ULONG Scrolling::s_ucWheelScrollLines = 0; -ULONG Scrolling::s_ucWheelScrollChars = 0; +til::CoordType Scrolling::s_ucWheelScrollLines = 0; +til::CoordType Scrolling::s_ucWheelScrollChars = 0; void Scrolling::s_UpdateSystemMetrics() { - s_ucWheelScrollLines = ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollLines(); - s_ucWheelScrollChars = ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollCharacters(); + s_ucWheelScrollLines = ::base::saturated_cast(ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollLines()); + s_ucWheelScrollChars = ::base::saturated_cast(ServiceLocator::LocateSystemConfigurationProvider()->GetNumberOfWheelScrollCharacters()); } bool Scrolling::s_IsInScrollMode() @@ -112,7 +112,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel, if (isMouseWheel && s_ucWheelScrollLines > 0) { // Rounding could cause this to be zero if gucWSL is bigger than 240 or so. - const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollLines, 1ul); + const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollLines, 1); // If we change direction we need to throw away any remainder we may have in the other direction. if ((ScreenInfo.WheelDelta > 0) == (wheelDelta > 0)) @@ -124,7 +124,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel, ScreenInfo.WheelDelta = wheelDelta; } - if ((ULONG)abs(ScreenInfo.WheelDelta) >= ulActualDelta) + if (abs(ScreenInfo.WheelDelta) >= ulActualDelta) { /* * By default, SHIFT + WM_MOUSEWHEEL will scroll 1/2 the @@ -134,7 +134,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel, til::CoordType delta = 1; if (hasShift) { - delta = std::max((ScreenInfo.GetViewport().Height() * ScreenInfo.ScrollScale) / 2, 1u); + delta = std::max((ScreenInfo.GetViewport().Height() * ::base::saturated_cast(ScreenInfo.ScrollScale)) / 2, 1); // Account for scroll direction changes by adjusting delta if there was a direction change. delta *= (ScreenInfo.WheelDelta < 0 ? -1 : 1); @@ -161,7 +161,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel, } else if (isMouseHWheel && s_ucWheelScrollChars > 0) { - const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollChars, 1ul); + const auto ulActualDelta = std::max(WHEEL_DELTA / s_ucWheelScrollChars, 1); if ((ScreenInfo.HWheelDelta > 0) == (wheelDelta > 0)) { @@ -172,7 +172,7 @@ void Scrolling::s_HandleMouseWheel(_In_ bool isMouseWheel, ScreenInfo.HWheelDelta = wheelDelta; } - if ((ULONG)abs(ScreenInfo.HWheelDelta) >= ulActualDelta) + if (abs(ScreenInfo.HWheelDelta) >= ulActualDelta) { til::CoordType delta = 1; diff --git a/src/host/scrolling.hpp b/src/host/scrolling.hpp index a082fa21a15..63956660fa1 100644 --- a/src/host/scrolling.hpp +++ b/src/host/scrolling.hpp @@ -41,6 +41,6 @@ class Scrolling private: static BOOL s_IsPointInRectangle(const til::rect* prc, const til::point pt); - static ULONG s_ucWheelScrollLines; - static ULONG s_ucWheelScrollChars; + static til::CoordType s_ucWheelScrollLines; + static til::CoordType s_ucWheelScrollChars; };