[FancyZones] Validate zone rect before adding it into layout (#6249)

* Validate zone rect before adding it into layout

* Rename variables for better code readability

src/modules/fancyzones/lib/Zone.cpp

@@ -11,6 +11,16 @@
 
 #include "common/monitors.h"
 
+namespace
+{
+    bool ValidateZoneRect(const RECT& rect)
+    {
+        int width  = rect.right - rect.left;
+        int height = rect.bottom - rect.top;
+        return rect.left >= 0 && rect.right >= 0 && rect.top >= 0 && rect.bottom >= 0 && width >= 0 && height >= 0;
+    }
+}
+
 struct Zone : winrt::implements<Zone, IZone>
 {
 public:
@@ -122,5 +132,12 @@ RECT Zone::ComputeActualZoneRect(HWND window, HWND zoneWindow) noexcept
 
 winrt::com_ptr<IZone> MakeZone(const RECT& zoneRect) noexcept
 {
+    if (ValidateZoneRect(zoneRect))
+    {
         return winrt::make_self<Zone>(zoneRect);
+    }
+    else
+    {
+        return nullptr;
+    }
 }

src/modules/fancyzones/lib/ZoneSet.cpp

@@ -484,8 +484,6 @@ bool ZoneSet::IsZoneEmpty(int zoneIndex) noexcept
 
 bool ZoneSet::CalculateFocusLayout(Rect workArea, int zoneCount) noexcept
 {
-    bool success = true;
-
     long left{ 100 };
     long top{ 100 };
     long right{ left + long(workArea.width() * 0.4) };
@@ -496,27 +494,30 @@ bool ZoneSet::CalculateFocusLayout(Rect workArea, int zoneCount) noexcept
     long focusRectXIncrement = (zoneCount <= 1) ? 0 : 50;
     long focusRectYIncrement = (zoneCount <= 1) ? 0 : 50;
 
-    if (left >= right || top >= bottom || left < 0 || right < 0 || top < 0 || bottom < 0)
-    {
-        success = false;
-    }
-
     for (int i = 0; i < zoneCount; i++)
     {
-        AddZone(MakeZone(focusZoneRect));
+        auto zone = MakeZone(focusZoneRect);
+        if (zone)
+        {
+            AddZone(zone);
+        }
+        else
+        {
+            // All zones within zone set should be valid in order to use its functionality.
+            m_zones.clear();
+            return false;
+        }
         focusZoneRect.left += focusRectXIncrement;
         focusZoneRect.right += focusRectXIncrement;
         focusZoneRect.bottom += focusRectYIncrement;
         focusZoneRect.top += focusRectYIncrement;
     }
 
-    return success;
+    return true;
 }
 
 bool ZoneSet::CalculateColumnsAndRowsLayout(Rect workArea, FancyZonesDataTypes::ZoneSetLayoutType type, int zoneCount, int spacing) noexcept
 {
-    bool success = true;
-
     long totalWidth;
     long totalHeight;
 
@@ -538,26 +539,31 @@ bool ZoneSet::CalculateColumnsAndRowsLayout(Rect workArea, FancyZonesDataTypes::
 
     // Note: The expressions below are NOT equal to total{Width|Height} / zoneCount and are done
     // like this to make the sum of all zones' sizes exactly total{Width|Height}.
-    for (int zone = 0; zone < zoneCount; zone++)
+    for (int zoneIndex = 0; zoneIndex < zoneCount; ++zoneIndex)
     {
         if (type == FancyZonesDataTypes::ZoneSetLayoutType::Columns)
         {
-            right = left + (zone + 1) * totalWidth / zoneCount - zone * totalWidth / zoneCount;
+            right = left + (zoneIndex + 1) * totalWidth / zoneCount - zoneIndex * totalWidth / zoneCount;
             bottom = totalHeight + spacing;
         }
         else
         { //Rows
             right = totalWidth + spacing;
-            bottom = top + (zone + 1) * totalHeight / zoneCount - zone * totalHeight / zoneCount;
+            bottom = top + (zoneIndex + 1) * totalHeight / zoneCount - zoneIndex * totalHeight / zoneCount;
         }
 
-        if (left >= right || top >= bottom || left < 0 || right < 0 || top < 0 || bottom < 0)
+
+        auto zone = MakeZone(RECT{ left, top, right, bottom });
+        if (zone)
         {
-            success = false;
+            AddZone(zone);
+        }
+        else
+        {
+            // All zones within zone set should be valid in order to use its functionality.
+            m_zones.clear();
+            return false;
         }
-
-        RECT focusZoneRect{ left, top, right, bottom };
-        AddZone(MakeZone(focusZoneRect));
 
         if (type == FancyZonesDataTypes::ZoneSetLayoutType::Columns)
         {
@@ -569,7 +575,7 @@ bool ZoneSet::CalculateColumnsAndRowsLayout(Rect workArea, FancyZonesDataTypes::
         }
     }
 
-    return success;
+    return true;
 }
 
 bool ZoneSet::CalculateGridLayout(Rect workArea, FancyZonesDataTypes::ZoneSetLayoutType type, int zoneCount, int spacing) noexcept
@@ -664,15 +670,20 @@ bool ZoneSet::CalculateCustomLayout(Rect workArea, int spacing) noexcept
                 int width = zone.width;
                 int height = zone.height;
 
-                if (x < 0 || y < 0 || width < 0 || height < 0)
-                {
-                    return false;
-                }
-
                 DPIAware::Convert(m_config.Monitor, x, y);
                 DPIAware::Convert(m_config.Monitor, width, height);
 
-                AddZone(MakeZone(RECT{ x, y, x + width, y + height }));
+                auto zone = MakeZone(RECT{ x, y, x + width, y + height });
+                if (zone)
+                {
+                    AddZone(zone);
+                }
+                else
+                {
+                    // All zones within zone set should be valid in order to use its functionality.
+                    m_zones.clear();
+                    return false;
+                }
             }
 
             return true;
@@ -689,8 +700,6 @@ bool ZoneSet::CalculateCustomLayout(Rect workArea, int spacing) noexcept
 
 bool ZoneSet::CalculateGridZones(Rect workArea, FancyZonesDataTypes::GridLayoutInfo gridLayoutInfo, int spacing)
 {
-    bool success = true;
-
     long totalWidth = workArea.width() - (spacing * (gridLayoutInfo.columns() + 1));
     long totalHeight = workArea.height() - (spacing * (gridLayoutInfo.rows() + 1));
     struct Info
@@ -747,17 +756,22 @@ bool ZoneSet::CalculateGridZones(Rect workArea, FancyZonesDataTypes::GridLayoutI
                 long right = columnInfo[maxCol].End;
                 long bottom = rowInfo[maxRow].End;
 
-                if (left >= right || top >= bottom || left < 0 || right < 0 || top < 0 || bottom < 0)
+                auto zone = MakeZone(RECT{ left, top, right, bottom });
+                if (zone)
                 {
-                    success = false;
+                    AddZone(zone);
+                }
+                else
+                {
+                    // All zones within zone set should be valid in order to use its functionality.
+                    m_zones.clear();
+                    return false;
                 }
-
-                AddZone(MakeZone(RECT{ left, top, right, bottom }));
             }
         }
     }
 
-    return success;
+    return true;
 }
 
 void ZoneSet::StampWindow(HWND window, size_t bitmask) noexcept

src/modules/fancyzones/tests/UnitTests/ZoneSet.Spec.cpp

@@ -85,6 +85,7 @@ namespace FancyZonesUnitTests
             TEST_METHOD (AddOne)
             {
                 winrt::com_ptr<IZone> zone = MakeZone({ 0, 0, 100, 100 });
+                Assert::IsNotNull(zone.get());
                 m_set->AddZone(zone);
                 auto zones = m_set->GetZones();
                 Assert::AreEqual((size_t)1, zones.size());
@@ -95,6 +96,7 @@ namespace FancyZonesUnitTests
             TEST_METHOD (AddManySame)
             {
                 winrt::com_ptr<IZone> zone = MakeZone({ 0, 0, 100, 100 });
+                Assert::IsNotNull(zone.get());
                 for (size_t i = 0; i < 1024; i++)
                 {
                     m_set->AddZone(zone);
@@ -110,6 +112,7 @@ namespace FancyZonesUnitTests
                 for (size_t i = 0; i < 1024; i++)
                 {
                     winrt::com_ptr<IZone> zone = MakeZone({ 0, 0, 100, 100 });
+                    Assert::IsNotNull(zone.get());
                     m_set->AddZone(zone);
                     auto zones = m_set->GetZones();
                     Assert::AreEqual(i + 1, zones.size());
@@ -122,7 +125,12 @@ namespace FancyZonesUnitTests
             {
                 for (size_t i = 0; i < 1024; i++)
                 {
-                    winrt::com_ptr<IZone> zone = MakeZone({ rand() % 10, rand() % 10, rand() % 100, rand() % 100 });
+                    int left   = rand() % 10;
+                    int top    = rand() % 10;
+                    int right  = left + 1 + rand() % 100;
+                    int bottom = top  + 1 + rand() % 100;
+                    winrt::com_ptr<IZone> zone = MakeZone({ left, top, right, bottom });
+                    Assert::IsNotNull(zone.get());
                     m_set->AddZone(zone);
                     auto zones = m_set->GetZones();
                     Assert::AreEqual(i + 1, zones.size());
@@ -131,6 +139,30 @@ namespace FancyZonesUnitTests
                 }
             }
 
+            TEST_METHOD (MakeZoneFromZeroRect)
+            {
+                winrt::com_ptr<IZone> zone = MakeZone({ 0, 0, 0, 0 });
+                Assert::IsNotNull(zone.get());
+            }
+
+            TEST_METHOD (MakeZoneFromInvalidRectWidth)
+            {
+                winrt::com_ptr<IZone> zone = MakeZone({ 100, 100, 99, 101 });
+                Assert::IsNull(zone.get());
+            }
+
+            TEST_METHOD (MakeZoneFromInvalidRectHeight)
+            {
+                winrt::com_ptr<IZone> zone = MakeZone({ 100, 100, 101, 99 });
+                Assert::IsNull(zone.get());
+            }
+
+            TEST_METHOD (MakeZoneFromInvalidRectCoords)
+            {
+                winrt::com_ptr<IZone> zone = MakeZone({ -1, -1, -1, -1 });
+                Assert::IsNull(zone.get());
+            }
+
             TEST_METHOD (ZoneFromPointEmpty)
             {
                 auto actual = m_set->ZonesFromPoint(POINT{ 0, 0 });
@@ -270,24 +302,6 @@ namespace FancyZonesUnitTests
                 compareZones(zone4, m_set->GetZones()[actual[3]]);
             }
 
-            TEST_METHOD (ZoneFromPointWithNotNormalizedRect)
-            {
-                winrt::com_ptr<IZone> zone = MakeZone({ 100, 100, 0, 0 });
-                m_set->AddZone(zone);
-
-                auto actual = m_set->ZonesFromPoint(POINT{ 50, 50 });
-                Assert::IsTrue(actual.size() == 0);
-            }
-
-            TEST_METHOD (ZoneFromPointWithZeroRect)
-            {
-                winrt::com_ptr<IZone> zone = MakeZone({ 0, 0, 0, 0 });
-                m_set->AddZone(zone);
-
-                auto actual = m_set->ZonesFromPoint(POINT{ 0, 0 });
-                Assert::IsTrue(actual.size() == 0);
-            }
-
             TEST_METHOD (ZoneIndexFromWindowUnknown)
             {
                 winrt::com_ptr<IZone> zone = MakeZone({ 0, 0, 100, 100 });