AtlasEngine: Fix a OOB read when rendering PUA chars (#16894)

When no soft fonts are set up but we're asked to draw a U+EF20, etc., character we'll currently read out-of-bounds, because we don't check whether the soft fonts array is non-empty. This PR fixes the issue by first getting a slice of the data and then checking if it's ok to use. This changeset additionally fixes a couple constinit vs. constexpr cases. I changed them to constinit at some point because I thought that it's more constexpr than constexpr by guaranteeing initialization at compile time. But nope, constinit is actually weaker in a way, because while it does guarantee that, it doesn't actually make the data constant. In other words, constinit is not `.rdata`.
microsoft · Mar 21, 2024 · 5b8e731 · 5b8e731
1 parent b9a0cae
commit 5b8e731
Show file tree

Hide file tree

Showing 10 changed files with 83 additions and 64 deletions.
diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp
@@ -5,10 +5,10 @@
 #include "Command.h"
 #include "Command.g.cpp"
 
-#include "ActionAndArgs.h"
-#include "KeyChordSerialization.h"
 #include <LibraryResources.h>
-#include "TerminalSettingsSerializationHelpers.h"
+#include <til/replace.h>
+
+#include "KeyChordSerialization.h"
 
 using namespace winrt::Microsoft::Terminal::Settings::Model;
 using namespace winrt::Windows::Foundation::Collections;
@@ -23,7 +23,6 @@ namespace winrt
 static constexpr std::string_view NameKey{ "name" };
 static constexpr std::string_view IconKey{ "icon" };
 static constexpr std::string_view ActionKey{ "command" };
-static constexpr std::string_view ArgsKey{ "args" };
 static constexpr std::string_view IterateOnKey{ "iterateOn" };
 static constexpr std::string_view CommandsKey{ "commands" };
 static constexpr std::string_view KeysKey{ "keys" };

diff --git a/src/cascadia/TerminalSettingsModel/KeyChordSerialization.cpp b/src/cascadia/TerminalSettingsModel/KeyChordSerialization.cpp
@@ -129,7 +129,7 @@ static KeyChord _fromString(std::wstring_view wstr)
     }
 
     using nameToVkeyPair = std::pair<std::wstring_view, int32_t>;
-    static constinit til::static_map nameToVkey{
+    static constexpr til::static_map nameToVkey{
     // The above VKEY_NAME_PAIRS macro contains a list of key-binding names for each virtual key.
     // This god-awful macro inverts VKEY_NAME_PAIRS and creates a static map of key-binding names to virtual keys.
     // clang-format off
@@ -245,7 +245,7 @@ static KeyChord _fromString(std::wstring_view wstr)
 static std::wstring _toString(const KeyChord& chord)
 {
     using vkeyToNamePair = std::pair<int32_t, std::wstring_view>;
-    static constinit til::static_map vkeyToName{
+    static constexpr til::static_map vkeyToName{
     // The above VKEY_NAME_PAIRS macro contains a list of key-binding strings for each virtual key.
     // This macro picks the first (most preferred) name and creates a static map of virtual keys to key-binding names.
 #define GENERATOR(vkey, name1, ...) vkeyToNamePair{ vkey, name1 },

diff --git a/src/inc/til.h b/src/inc/til.h
@@ -20,8 +20,8 @@
 #include "til/color.h"
 #include "til/enumset.h"
 #include "til/pmr.h"
-#include "til/replace.h"
 #include "til/string.h"
+#include "til/type_traits.h"
 #include "til/u8u16convert.h"
 
 // Use keywords on TraceLogging providers to specify the category
@@ -54,6 +54,24 @@
 
 namespace til // Terminal Implementation Library. Also: "Today I Learned"
 {
+    template<typename T>
+    as_view_t<T> clamp_slice_abs(const T& view, size_t beg, size_t end)
+    {
+        const auto len = view.size();
+        end = std::min(end, len);
+        beg = std::min(beg, end);
+        return { view.data() + beg, end - beg };
+    }
+
+    template<typename T>
+    as_view_t<T> clamp_slice_len(const T& view, size_t start, size_t count)
+    {
+        const auto len = view.size();
+        start = std::min(start, len);
+        count = std::min(count, len - start);
+        return { view.data() + start, count };
+    }
+
     template<typename T>
     void manage_vector(std::vector<T>& vector, typename std::vector<T>::size_type requestedSize, float shrinkThreshold)
     {

diff --git a/src/inc/til/bytes.h b/src/inc/til/bytes.h
@@ -3,8 +3,6 @@
 
 #pragma once
 
-#include "type_traits.h"
-
 namespace til
 {
     template<ContiguousBytes Target>

diff --git a/src/inc/til/replace.h b/src/inc/til/replace.h
@@ -5,27 +5,6 @@
 
 namespace til
 {
-    namespace details
-    {
-        template<typename T>
-        struct view_type_oracle
-        {
-        };
-
-        template<>
-        struct view_type_oracle<std::string>
-        {
-            using type = std::string_view;
-        };
-
-        template<>
-        struct view_type_oracle<std::wstring>
-        {
-            using type = std::wstring_view;
-        };
-
-    }
-
     // Method Description:
     // - This is a function for finding all occurrences of a given string
     //   `needle` in a larger string `haystack`, and replacing them with the
@@ -39,8 +18,8 @@ namespace til
     // - <none>
     template<typename T>
     void replace_needle_in_haystack_inplace(T& haystack,
-                                            const typename details::view_type_oracle<T>::type& needle,
-                                            const typename details::view_type_oracle<T>::type& replacement)
+                                            const as_view_t<T>& needle,
+                                            const as_view_t<T>& replacement)
     {
         auto pos{ T::npos };
         while ((pos = haystack.rfind(needle, pos)) != T::npos)
@@ -63,8 +42,8 @@ namespace til
     // - a copy of `haystack` with all instances of `needle` replaced with `replacement`.`
     template<typename T>
     T replace_needle_in_haystack(const T& haystack,
-                                 const typename details::view_type_oracle<T>::type& needle,
-                                 const typename details::view_type_oracle<T>::type& replacement)
+                                 const as_view_t<T>& needle,
+                                 const as_view_t<T>& replacement)
     {
         std::basic_string<typename T::value_type> result{ haystack };
         replace_needle_in_haystack_inplace(result, needle, replacement);

diff --git a/src/inc/til/type_traits.h b/src/inc/til/type_traits.h
@@ -32,6 +32,24 @@ namespace til
         struct is_byte<std::byte> : std::true_type
         {
         };
+
+        template<typename T>
+        struct as_view
+        {
+            using type = T;
+        };
+
+        template<typename T, typename Alloc>
+        struct as_view<std::vector<T, Alloc>>
+        {
+            using type = std::span<const T>;
+        };
+
+        template<typename T, typename Traits, typename Alloc>
+        struct as_view<std::basic_string<T, Traits, Alloc>>
+        {
+            using type = std::basic_string_view<T, Traits>;
+        };
     }
 
     template<typename T>
@@ -45,4 +63,7 @@ namespace til
 
     template<typename T>
     concept TriviallyCopyable = std::is_trivially_copyable_v<T>;
+
+    template<typename T>
+    using as_view_t = typename details::as_view<T>::type;
 }
diff --git a/src/renderer/atlas/BackendD3D.cpp b/src/renderer/atlas/BackendD3D.cpp
@@ -1430,19 +1430,19 @@ BackendD3D::AtlasGlyphEntry* BackendD3D::_drawBuiltinGlyph(const RenderingPayloa
     _d2dBeginDrawing();
 
     auto shadingType = ShadingType::TextGrayscale;
+    const D2D1_RECT_F r{
+        static_cast<f32>(rect.x),
+        static_cast<f32>(rect.y),
+        static_cast<f32>(rect.x + rect.w),
+        static_cast<f32>(rect.y + rect.h),
+    };
 
     if (BuiltinGlyphs::IsSoftFontChar(glyphIndex))
     {
-        _drawSoftFontGlyph(p, rect, glyphIndex);
+        _drawSoftFontGlyph(p, r, glyphIndex);
     }
     else
     {
-        const D2D1_RECT_F r{
-            static_cast<f32>(rect.x),
-            static_cast<f32>(rect.y),
-            static_cast<f32>(rect.x + rect.w),
-            static_cast<f32>(rect.y + rect.h),
-        };
         BuiltinGlyphs::DrawBuiltinGlyph(p.d2dFactory.get(), _d2dRenderTarget.get(), _brush.get(), r, glyphIndex);
         shadingType = ShadingType::TextBuiltinGlyph;
     }
@@ -1465,15 +1465,31 @@ BackendD3D::AtlasGlyphEntry* BackendD3D::_drawBuiltinGlyph(const RenderingPayloa
     return glyphEntry;
 }
 
-void BackendD3D::_drawSoftFontGlyph(const RenderingPayload& p, const stbrp_rect& rect, u32 glyphIndex)
+void BackendD3D::_drawSoftFontGlyph(const RenderingPayload& p, const D2D1_RECT_F& rect, u32 glyphIndex)
 {
+    _d2dRenderTarget->PushAxisAlignedClip(&rect, D2D1_ANTIALIAS_MODE_ALIASED);
+    const auto restoreD2D = wil::scope_exit([&]() {
+        _d2dRenderTarget->PopAxisAlignedClip();
+    });
+
+    const auto width = static_cast<size_t>(p.s->font->softFontCellSize.width);
+    const auto height = static_cast<size_t>(p.s->font->softFontCellSize.height);
+    const auto softFontIndex = glyphIndex - 0xEF20u;
+    const auto data = til::clamp_slice_len(p.s->font->softFontPattern, height * softFontIndex, height);
+
+    if (data.empty() || data.size() != height)
+    {
+        _d2dRenderTarget->Clear();
+        return;
+    }
+
     if (!_softFontBitmap)
     {
         // Allocating such a tiny texture is very wasteful (min. texture size on GPUs
-        // right now is 64kB), but this is a seldomly used feature so it's fine...
+        // right now is 64kB), but this is a seldom used feature, so it's fine...
         const D2D1_SIZE_U size{
-            static_cast<UINT32>(p.s->font->softFontCellSize.width),
-            static_cast<UINT32>(p.s->font->softFontCellSize.height),
+            static_cast<UINT32>(width),
+            static_cast<UINT32>(height),
         };
         const D2D1_BITMAP_PROPERTIES1 bitmapProperties{
             .pixelFormat = { DXGI_FORMAT_B8G8R8A8_UNORM, D2D1_ALPHA_MODE_PREMULTIPLIED },
@@ -1484,17 +1500,11 @@ void BackendD3D::_drawSoftFontGlyph(const RenderingPayload& p, const stbrp_rect&
     }
 
     {
-        const auto width = static_cast<size_t>(p.s->font->softFontCellSize.width);
-        const auto height = static_cast<size_t>(p.s->font->softFontCellSize.height);
-
         auto bitmapData = Buffer<u32>{ width * height };
-        const auto softFontIndex = glyphIndex - 0xEF20u;
-        auto src = p.s->font->softFontPattern.begin() + height * softFontIndex;
         auto dst = bitmapData.begin();
 
-        for (size_t y = 0; y < height; y++)
+        for (auto srcBits : data)
         {
-            auto srcBits = *src++;
             for (size_t x = 0; x < width; x++)
             {
                 const auto srcBitIsSet = (srcBits & 0x8000) != 0;
@@ -1508,15 +1518,7 @@ void BackendD3D::_drawSoftFontGlyph(const RenderingPayload& p, const stbrp_rect&
     }
 
     const auto interpolation = p.s->font->antialiasingMode == AntialiasingMode::Aliased ? D2D1_INTERPOLATION_MODE_NEAREST_NEIGHBOR : D2D1_INTERPOLATION_MODE_HIGH_QUALITY_CUBIC;
-    const D2D1_RECT_F dest{
-        static_cast<f32>(rect.x),
-        static_cast<f32>(rect.y),
-        static_cast<f32>(rect.x + rect.w),
-        static_cast<f32>(rect.y + rect.h),
-    };
-
-    _d2dBeginDrawing();
-    _d2dRenderTarget->DrawBitmap(_softFontBitmap.get(), &dest, 1, interpolation, nullptr, nullptr);
+    _d2dRenderTarget->DrawBitmap(_softFontBitmap.get(), &rect, 1, interpolation, nullptr, nullptr);
 }
 
 void BackendD3D::_drawGlyphAtlasAllocate(const RenderingPayload& p, stbrp_rect& rect)

diff --git a/src/renderer/atlas/BackendD3D.h b/src/renderer/atlas/BackendD3D.h
@@ -215,7 +215,7 @@ namespace Microsoft::Console::Render::Atlas
         ATLAS_ATTR_COLD void _drawTextOverlapSplit(const RenderingPayload& p, u16 y);
         [[nodiscard]] ATLAS_ATTR_COLD AtlasGlyphEntry* _drawGlyph(const RenderingPayload& p, const ShapedRow& row, AtlasFontFaceEntry& fontFaceEntry, u32 glyphIndex);
         AtlasGlyphEntry* _drawBuiltinGlyph(const RenderingPayload& p, const ShapedRow& row, AtlasFontFaceEntry& fontFaceEntry, u32 glyphIndex);
-        void _drawSoftFontGlyph(const RenderingPayload& p, const stbrp_rect& rect, u32 glyphIndex);
+        void _drawSoftFontGlyph(const RenderingPayload& p, const D2D1_RECT_F& rect, u32 glyphIndex);
         void _drawGlyphAtlasAllocate(const RenderingPayload& p, stbrp_rect& rect);
         static AtlasGlyphEntry* _drawGlyphAllocateEntry(const ShapedRow& row, AtlasFontFaceEntry& fontFaceEntry, u32 glyphIndex);
         static void _splitDoubleHeightGlyph(const RenderingPayload& p, const ShapedRow& row, AtlasFontFaceEntry& fontFaceEntry, AtlasGlyphEntry* glyphEntry);

diff --git a/src/til/ut_til/ReplaceTests.cpp b/src/til/ut_til/ReplaceTests.cpp
@@ -4,6 +4,8 @@
 #include "precomp.h"
 #include "WexTestClass.h"
 
+#include <til/replace.h>
+
 using namespace WEX::Common;
 using namespace WEX::Logging;
 using namespace WEX::TestExecution;

diff --git a/src/types/colorTable.cpp b/src/types/colorTable.cpp
@@ -267,7 +267,7 @@ static constexpr std::array<til::color, 256> standard256ColorTable{
     til::color{ 0xEE, 0xEE, 0xEE },
 };
 
-static constinit til::presorted_static_map xorgAppVariantColorTable{
+static constexpr til::presorted_static_map xorgAppVariantColorTable{
     std::pair{ "antiquewhite"sv, std::array<til::color, 5>{ til::color{ 250, 235, 215 }, til::color{ 255, 239, 219 }, til::color{ 238, 223, 204 }, til::color{ 205, 192, 176 }, til::color{ 139, 131, 120 } } },
     std::pair{ "aquamarine"sv, std::array<til::color, 5>{ til::color{ 127, 255, 212 }, til::color{ 127, 255, 212 }, til::color{ 118, 238, 198 }, til::color{ 102, 205, 170 }, til::color{ 69, 139, 116 } } },
     std::pair{ "azure"sv, std::array<til::color, 5>{ til::color{ 240, 255, 255 }, til::color{ 240, 255, 255 }, til::color{ 224, 238, 238 }, til::color{ 193, 205, 205 }, til::color{ 131, 139, 139 } } },
@@ -348,7 +348,7 @@ static constinit til::presorted_static_map xorgAppVariantColorTable{
     std::pair{ "yellow"sv, std::array<til::color, 5>{ til::color{ 255, 255, 0 }, til::color{ 255, 255, 0 }, til::color{ 238, 238, 0 }, til::color{ 205, 205, 0 }, til::color{ 139, 139, 0 } } },
 };
 
-static constinit til::presorted_static_map xorgAppColorTable{
+static constexpr til::presorted_static_map xorgAppColorTable{
     std::pair{ "aliceblue"sv, til::color{ 240, 248, 255 } },
     std::pair{ "aqua"sv, til::color{ 0, 255, 255 } },
     std::pair{ "beige"sv, til::color{ 245, 245, 220 } },