From ee014f89c09c1804af960c26279c46188963c2b6 Mon Sep 17 00:00:00 2001 From: vladimir-krestov <49272759+vladimir-krestov@users.noreply.github.com> Date: Tue, 14 Apr 2020 01:39:40 +0300 Subject: [PATCH] [Regression] Fixing MonthCalendar NRE when MinDate is set (#3041) Add protection for MonthCalendarAccessibleObject against incorrect parameters of methods to avoid exception throwing Related Issues #2912 and #2475 Related PR #2911 --- src/Common/src/UnsafeNativeMethods.cs | 3 + .../System/Windows/Forms/DateTimePicker.cs | 10 +- ...thCalendar.CalendarBodyAccessibleObject.cs | 6 + ...hCalendar.CalendarChildAccessibleObject.cs | 6 +- ...endar.CalendarGridChildAccessibleObject.cs | 7 +- ...hCalendar.MonthCalendarAccessibleObject.cs | 36 +++-- ...ndar.MonthCalendarAccessibleObjectTests.cs | 23 ++++ .../tests/UnitTests/DateTimePickerTests.cs | 10 ++ .../tests/UnitTests/MonthCalendarTests.cs | 123 ++++++++++++++++++ .../System/Windows/Forms/ApplicationTests.cs | 18 ++- 10 files changed, 225 insertions(+), 17 deletions(-) create mode 100644 src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/MonthCalendar.MonthCalendarAccessibleObjectTests.cs diff --git a/src/Common/src/UnsafeNativeMethods.cs b/src/Common/src/UnsafeNativeMethods.cs index a8c89992027..3ab5e92bf73 100644 --- a/src/Common/src/UnsafeNativeMethods.cs +++ b/src/Common/src/UnsafeNativeMethods.cs @@ -579,6 +579,9 @@ public static StringBuilder GetModuleFileNameLongPath(HandleRef hModule) [DllImport(ExternDll.User32, CharSet = CharSet.Auto)] public static extern IntPtr SendMessage(HandleRef hWnd, int msg, int wParam, NativeMethods.SYSTEMTIMEARRAY lParam); + [DllImport(ExternDll.User32, CharSet = CharSet.Auto)] + public static extern IntPtr SendMessage(HandleRef hWnd, int msg, int wParam, NativeMethods.NMSELCHANGE lParam); + [DllImport(ExternDll.User32, CharSet = CharSet.Auto)] public static extern IntPtr SendMessage(HandleRef hWnd, int msg, int wParam, ref NativeMethods.LOGFONTW lParam); diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/DateTimePicker.cs b/src/System.Windows.Forms/src/System/Windows/Forms/DateTimePicker.cs index d474ae2eaf6..4a303236f1e 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/DateTimePicker.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/DateTimePicker.cs @@ -1108,9 +1108,9 @@ public event EventHandler DropDown } /// - /// Constructs the new instance of the accessibility object for this control. Subclasses + /// Constructs the new instance of the accessibility object for this control. Subclasses /// should not call base.CreateAccessibilityObject. - /// + /// protected override AccessibleObject CreateAccessibilityInstance() { return new DateTimePickerAccessibleObject(this); @@ -1767,6 +1767,12 @@ internal static Kernel32.SYSTEMTIME DateTimeToSysTime(DateTime time) /// internal static DateTime SysTimeToDateTime(Kernel32.SYSTEMTIME s) { + if (s.wYear <= 0 || s.wMonth <= 0 || s.wDay <= 0) + { + Debug.WriteLine("Incorrect SYSTEMTIME info!"); + return DateTime.MinValue; + } + return new DateTime(s.wYear, s.wMonth, s.wDay, s.wHour, s.wMinute, s.wSecond); } diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarBodyAccessibleObject.cs b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarBodyAccessibleObject.cs index eee7e774721..b4bb115f14c 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarBodyAccessibleObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarBodyAccessibleObject.cs @@ -54,6 +54,12 @@ public CalendarChildAccessibleObject GetFromPoint(ComCtl32.MCHITTESTINFO hitTest case ComCtl32.MCHT.CALENDARDATE: AccessibleObject rowAccessibleObject = _calendarAccessibleObject.GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.CalendarRow, this, hitTestInfo.iRow); + + if (rowAccessibleObject == null) + { + return null; + } + return _calendarAccessibleObject.GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.CalendarCell, rowAccessibleObject, hitTestInfo.iCol); } diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarChildAccessibleObject.cs b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarChildAccessibleObject.cs index a601d49dc10..74655210123 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarChildAccessibleObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarChildAccessibleObject.cs @@ -15,13 +15,13 @@ public partial class MonthCalendar /// internal abstract class CalendarChildAccessibleObject : AccessibleObject { - protected MonthCalendarAccessibleObject _calendarAccessibleObject; + protected readonly MonthCalendarAccessibleObject _calendarAccessibleObject; protected int _calendarIndex; protected CalendarChildType _itemType; public CalendarChildAccessibleObject(MonthCalendarAccessibleObject calendarAccessibleObject, int calendarIndex, CalendarChildType itemType) { - _calendarAccessibleObject = calendarAccessibleObject; + _calendarAccessibleObject = calendarAccessibleObject ?? throw new ArgumentNullException(nameof(calendarAccessibleObject)); _calendarIndex = calendarIndex; _itemType = itemType; } @@ -64,7 +64,7 @@ public void RaiseMouseClick() return; } - var rectangle = CalculateBoundingRectangle(); + RECT rectangle = CalculateBoundingRectangle(); int x = rectangle.left + ((rectangle.right - rectangle.left) / 2); int y = rectangle.top + ((rectangle.bottom - rectangle.top) / 2); diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarGridChildAccessibleObject.cs b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarGridChildAccessibleObject.cs index d216e2ce86d..1ce027adac2 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarGridChildAccessibleObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.CalendarGridChildAccessibleObject.cs @@ -11,11 +11,16 @@ public partial class MonthCalendar /// internal abstract class CalendarGridChildAccessibleObject : CalendarChildAccessibleObject { - protected AccessibleObject _parentAccessibleObject; + protected readonly AccessibleObject _parentAccessibleObject; public CalendarGridChildAccessibleObject(MonthCalendarAccessibleObject calendarAccessibleObject, int calendarIndex, CalendarChildType itemType, AccessibleObject parentAccessibleObject, int itemIndex) : base(calendarAccessibleObject, calendarIndex, itemType) { + if (parentAccessibleObject == null) + { + throw new ArgumentNullException(nameof(parentAccessibleObject)); + } + _parentAccessibleObject = parentAccessibleObject; } diff --git a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.MonthCalendarAccessibleObject.cs b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.MonthCalendarAccessibleObject.cs index 711a616d12e..5b5cf418d39 100644 --- a/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.MonthCalendarAccessibleObject.cs +++ b/src/System.Windows.Forms/src/System/Windows/Forms/MonthCalendar.MonthCalendarAccessibleObject.cs @@ -25,7 +25,7 @@ internal class MonthCalendarAccessibleObject : ControlAccessibleObject public MonthCalendarAccessibleObject(Control owner) : base(owner) { - _owner = owner as MonthCalendar; + _owner = (MonthCalendar)owner; } public int ControlType => @@ -290,7 +290,7 @@ internal override UnsafeNativeMethods.IRawElementProviderFragment ElementProvide case ComCtl32.MCHT.CALENDARDATE: // Get calendar body's child. CalendarBodyAccessibleObject calendarBodyAccessibleObject = (CalendarBodyAccessibleObject)GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.CalendarBody); - return calendarBodyAccessibleObject.GetFromPoint(hitTestInfo); + return calendarBodyAccessibleObject?.GetFromPoint(hitTestInfo); case ComCtl32.MCHT.TODAYLINK: return GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.TodayLink); @@ -365,7 +365,8 @@ public string GetCalendarChildName(int calendarIndex, CalendarChildType calendar private CalendarCellAccessibleObject GetCalendarCell(int calendarIndex, AccessibleObject parentAccessibleObject, int columnIndex) { - if (columnIndex < 0 || + if (parentAccessibleObject == null || + columnIndex < 0 || columnIndex >= MAX_DAYS || columnIndex >= ColumnCount) { @@ -418,7 +419,8 @@ private string GetCalendarCellName(DateTime endDate, DateTime startDate, string private CalendarRowAccessibleObject GetCalendarRow(int calendarIndex, AccessibleObject parentAccessibleObject, int rowIndex) { - if ((HasHeaderRow ? rowIndex < -1 : rowIndex < 0) || + if (parentAccessibleObject == null || + (HasHeaderRow ? rowIndex < -1 : rowIndex < 0) || rowIndex >= RowCount) { return null; @@ -443,7 +445,7 @@ private CalendarRowAccessibleObject GetCalendarRow(int calendarIndex, Accessible SelectionRange cellsRange = _owner.GetDisplayRange(false); - if (cellsRange.Start > DateTimePicker.SysTimeToDateTime(endDate) || cellsRange.End < DateTimePicker.SysTimeToDateTime(startDate)) + if (cellsRange == null || cellsRange.Start > DateTimePicker.SysTimeToDateTime(endDate) || cellsRange.End < DateTimePicker.SysTimeToDateTime(startDate)) { // Do not create row if the row's first cell is out of the current calendar's view range. return null; @@ -588,7 +590,7 @@ public void RaiseMouseClick(int x, int y) { POINT previousPosition = new POINT(); bool setOldCursorPos = UnsafeNativeMethods.GetPhysicalCursorPos(ref previousPosition); - + bool mouseSwapped = User32.GetSystemMetrics(User32.SystemMetric.SM_SWAPBUTTON) != 0; SendMouseInput(x, y, User32.MOUSEEVENTF.MOVE | User32.MOUSEEVENTF.ABSOLUTE); @@ -669,13 +671,23 @@ public void RaiseAutomationEventForChild(int automationEventId, DateTime selecti private AccessibleObject GetCalendarChildAccessibleObject(DateTime selectionStart, DateTime selectionEnd) { - int columnCount = ColumnCount; - AccessibleObject bodyAccessibleObject = GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.CalendarBody); + + if (bodyAccessibleObject == null) + { + return null; + } + for (int row = 0; row < RowCount; row++) { AccessibleObject rowAccessibleObject = GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.CalendarRow, bodyAccessibleObject, row); - for (int column = 0; column < columnCount; column++) + + if (rowAccessibleObject == null) + { + continue; + } + + for (int column = 0; column < ColumnCount; column++) { bool success = GetCalendarGridInfo( ComCtl32.MCGIF.DATE, @@ -724,6 +736,12 @@ internal override UnsafeNativeMethods.IRawElementProviderSimple[] GetColumnHeade UnsafeNativeMethods.IRawElementProviderSimple[] headers = new UnsafeNativeMethods.IRawElementProviderSimple[MonthCalendarAccessibleObject.MAX_DAYS]; AccessibleObject headerRowAccessibleObject = GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.CalendarRow, this, -1); + + if (headerRowAccessibleObject == null) + { + return null; + } + for (int columnIndex = 0; columnIndex < MonthCalendarAccessibleObject.MAX_DAYS; columnIndex++) { headers[columnIndex] = GetCalendarChildAccessibleObject(_calendarIndex, CalendarChildType.CalendarCell, headerRowAccessibleObject, columnIndex); diff --git a/src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/MonthCalendar.MonthCalendarAccessibleObjectTests.cs b/src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/MonthCalendar.MonthCalendarAccessibleObjectTests.cs new file mode 100644 index 00000000000..2e7e3f4e3fb --- /dev/null +++ b/src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/MonthCalendar.MonthCalendarAccessibleObjectTests.cs @@ -0,0 +1,23 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Reflection; +using Xunit; +using static System.Windows.Forms.MonthCalendar; + +namespace System.Windows.Forms.Tests.AccessibleObjects +{ + public class MonthCalendarAccessibleObjectTests + { + [WinFormsFact] + public void MonthCalendarAccessibleObject_GetCalendarCell_DoesntThrowException_If_ParentAccessibleObject_IsNull() + { + using MonthCalendar monthCalendar = new MonthCalendar(); + MonthCalendarAccessibleObject accessibleObject = (MonthCalendarAccessibleObject)monthCalendar.AccessibilityObject; + Type type = typeof(MonthCalendarAccessibleObject); + MethodInfo method = type.GetMethod("GetCalendarCell", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.Null(method.Invoke(accessibleObject, new object[] { 0, /*parentAccessibleObject*/ null, 0 })); + } + } +} diff --git a/src/System.Windows.Forms/tests/UnitTests/DateTimePickerTests.cs b/src/System.Windows.Forms/tests/UnitTests/DateTimePickerTests.cs index 806f7f53e0a..4e7ab0c90b0 100644 --- a/src/System.Windows.Forms/tests/UnitTests/DateTimePickerTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/DateTimePickerTests.cs @@ -16,5 +16,15 @@ public void DateTimePicker_Constructor() Assert.NotNull(dtp); Assert.Equal(DateTimePickerFormat.Long, dtp.Format); } + + [WinFormsFact] + public void DateTimePicker_SysTimeToDateTime_DoesnThrowException_If_SYSTEMTIME_IsIncorrect() + { + // An empty SYSTEMTIME has year, month and day as 0, but DateTime can't have these parameters. + // So an empty SYSTEMTIME is incorrect in this case. + Interop.Kernel32.SYSTEMTIME systemTime = new Interop.Kernel32.SYSTEMTIME(); + DateTime dateTime = DateTimePicker.SysTimeToDateTime(systemTime); + Assert.Equal(DateTime.MinValue, dateTime); + } } } diff --git a/src/System.Windows.Forms/tests/UnitTests/MonthCalendarTests.cs b/src/System.Windows.Forms/tests/UnitTests/MonthCalendarTests.cs index 913c574cea2..c26c0326837 100644 --- a/src/System.Windows.Forms/tests/UnitTests/MonthCalendarTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/MonthCalendarTests.cs @@ -2,7 +2,14 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Threading; using Xunit; +using System.Runtime.InteropServices; +using System.Collections.Generic; +using System.Reflection; +using static Interop.Kernel32; +using static System.Windows.Forms.NativeMethods; +using static Interop; namespace System.Windows.Forms.Tests { @@ -16,5 +23,121 @@ public void MonthCalendar_Constructor() Assert.NotNull(mc); Assert.True(mc.TabStop); } + + public static IEnumerable MonthCalendar_SettingDate_DoesntCrashApplication_TestData() + { + yield return new object[] { new MonthCalendar { MinDate = new DateTime(2020, 4, 9) } }; + yield return new object[] { new MonthCalendar { MaxDate = new DateTime(2020, 4, 22) } }; + yield return new object[] { new MonthCalendar() }; + } + + /// + /// We have to check that UseVisualStyles state isn't changed. + /// This test name is used to make sure the test will be executed + /// BEFORE test. + /// + [Fact] + public void Application_UseVisualStyles_IsNotSet() + { + Assert.False(Application.UseVisualStyles); + } + + [Theory] + [MemberData(nameof(MonthCalendar_SettingDate_DoesntCrashApplication_TestData))] + public void MonthCalendar_SettingDate_DoesntCrashApplication(MonthCalendar calendar) + { + Assert.False(Application.UseVisualStyles); + Exception exception = null; + ThreadExceptionEventHandler getException = + (object sender, ThreadExceptionEventArgs e) => exception = e.Exception; + + try + { + /// EnableVisualStyles method changes UseVisualStyles state. + /// It mustn't affect other tests, + /// so we have to check it in the test. + Application.EnableVisualStyles(); + Application.ThreadException += getException; + + // We need to check if the calendar works well when selecting some date + // if the calendar has a min/max date or not. + // In this case, a min/max date is chosen so that the calendar grid doesn't have some dates. + // This could potentially lead to exceptions when creating the calendar AccessibleObject, + // so we have to check these cases. + // Application running and TestForm using are necessary + // because otherwise there is no way to get a correct result when getting MCGRIDINFO from the calendar. + Application.Run(new TestForm(calendar)); + } + finally + { + Application.ExitThread(); + + // Return UseVisualStyles as it was before. + PropertyInfo prop = typeof(Application).GetProperty(nameof(Application.UseVisualStyles)); + prop?.SetValue(null, false, BindingFlags.NonPublic | BindingFlags.Static, null, null, null); + + // Verify the process is finished correctly + Assert.False(Application.UseVisualStyles); + Assert.Null(exception); + Assert.False(Application.MessageLoop); + } + } + + class TestForm : Form + { + private readonly MonthCalendar _monthCalendar; + + public TestForm(MonthCalendar calendar) + { + _monthCalendar = calendar; + Controls.Add(_monthCalendar); + Load += Form_Load; + } + + private void Form_Load(object sender, EventArgs e) + { + // Simulate the Windows notification sending about changing a selected date. + try + { + DateTime selectedDate = new DateTime(2020, 4, 10); + + SYSTEMTIME date = new SYSTEMTIME + { + wYear = (short)selectedDate.Year, + wMonth = (short)selectedDate.Month, + wDay = (short)selectedDate.Day + }; + + Assert.NotEqual(IntPtr.Zero, _monthCalendar.Handle); + + NMSELCHANGE lParam = new NMSELCHANGE + { + nmhdr = new NMHDR + { + code = MCN_SELCHANGE, + }, + stSelStart = date, + stSelEnd = date, + }; + + UnsafeNativeMethods.SendMessage(new HandleRef(_monthCalendar, _monthCalendar.Handle), WindowMessages.WM_REFLECT + WindowMessages.WM_NOTIFY, 0, lParam); + } + finally + { + Close(); + } + } + } + + /// + /// We have to check that UseVisualStyles state isn't changed. + /// This test name is used to make sure the test will be executed + /// AFTER test. + /// + [Fact] + public void MonthCalendar_SettingDate_UseVisualStyles_IsNotSet() + { + Assert.False(Application.UseVisualStyles); + } } } diff --git a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ApplicationTests.cs b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ApplicationTests.cs index f0f420a60bf..3065cc88852 100644 --- a/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ApplicationTests.cs +++ b/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ApplicationTests.cs @@ -4,16 +4,30 @@ using Xunit; using System.Runtime.InteropServices; +using System.Reflection; namespace System.Windows.Forms.Tests { public class ApplicationTests { + private readonly object _locker = new object(); + [Fact] public void Application_EnableVisualStyles_GetUseVisualStyles_ReturnsTrue() { - Application.EnableVisualStyles(); - Assert.True(Application.UseVisualStyles, "New Visual Styles will not be applied on Winforms app. This is a high priority bug and must be looked into"); + lock (_locker) + { + Assert.False(Application.UseVisualStyles); + + Application.EnableVisualStyles(); + Assert.True(Application.UseVisualStyles, "New Visual Styles will not be applied on Winforms app. This is a high priority bug and must be looked into"); + + // Return UseVisualStyles as it was before. + PropertyInfo prop = typeof(Application).GetProperty(nameof(Application.UseVisualStyles)); + prop?.SetValue(null, false, BindingFlags.NonPublic | BindingFlags.Static, null, null, null); + } + + Assert.False(Application.UseVisualStyles); } [Fact]