[release/10.0] Handle unterminated string data in DataObject (#13988)

Manually look for the terminator and return empty string if there is no terminator. Throw for bad HGLOBALs.

Needed to add some more code for testing nested private generic classes.
---------

Co-authored-by: Jeremy Kuhne <jeremy.kuhne@microsoft.com>

Author: github-actions[bot]

src/Common/tests/TestUtilities/ReflectionHelper.cs

@@ -47,4 +47,43 @@ private static bool IsPublicNonAbstract<T>(Type type)
     {
         return !type.IsAbstract && type.IsPublic && typeof(T).IsAssignableFrom(type);
     }
+
+    /// <summary>
+    ///  Gets a nested type, and if it's a generic type definition, makes it a full type using the parent type's generic arguments.
+    /// </summary>
+    /// <param name="parentType">The parent type.</param>
+    /// <param name="nestedTypeName">The name of the nested type.</param>
+    /// <param name="genericTypes">Additional nested type parameters, if any.</param>
+    /// <returns>Nested types.</returns>
+    /// <exception cref="ArgumentException">Could not find the <paramref name="nestedTypeName"/>.</exception>
+    /// <exception cref="NotImplementedException">An additional case still needs implemented.</exception>
+    public static Type GetFullNestedType(
+        this Type parentType,
+        string nestedTypeName,
+        params Span<Type> genericTypes)
+    {
+        Type nestedType = parentType.GetNestedType(nestedTypeName, BindingFlags.Public | BindingFlags.NonPublic)
+            ?? throw new ArgumentException($"Could not find {nestedTypeName} in {parentType.Name}");
+
+        if (!nestedType.IsTypeDefinition)
+        {
+            return nestedType;
+        }
+
+        if (parentType.IsGenericType)
+        {
+            Type[] parentTypes = parentType.GenericTypeArguments;
+            Type[] nestedTypes = nestedType.GenericTypeArguments;
+
+            if (nestedTypes.Length == 0)
+            {
+                // Only the parent types are needed.
+                Type fullType = nestedType.MakeGenericType(parentTypes);
+                return fullType;
+            }
+        }
+
+        // Implementing the other cases is relatively trivial, leaving them until we have concrete usage.
+        throw new NotImplementedException("Implement other cases as they occur");
+    }
 }

src/Common/tests/TestUtilities/TestAccessor.cs

@@ -133,7 +133,16 @@ public override bool TryInvokeMember(InvokeMemberBinder binder, object?[]? args,
             if (methodInfo is null)
                 return false;
 
-            result = methodInfo.Invoke(_instance, args);
+            try
+            {
+                result = methodInfo.Invoke(_instance, args);
+            }
+            catch (TargetInvocationException ex) when (ex.InnerException is not null)
+            {
+                // Unwrap the inner exception to make it easier for callers to handle.
+                throw ex.InnerException;
+            }
+
             return true;
         }
 

src/Common/tests/TestUtilities/TestAccessors.cs

@@ -46,6 +46,12 @@ public static partial class TestAccessors
     ///    Assert.Equal(version2, accessor.Parse("4.1")));
     ///   ]]>
     ///  </code>
+    ///  <para>
+    ///   When attempting to get nested private types that are generic (nested types in a generic type
+    ///   are always generic, and inherit the type specifiers of the the parent type), use the extension
+    ///   <see cref="ReflectionHelper.GetFullNestedType(Type, string, Span{Type})"/> to get a fully
+    ///   instantiated type for the nested type, then pass that Type to this method.
+    ///  </para>
     /// </remarks>
     public static ITestAccessor TestAccessor(this object instanceOrType)
     {

src/System.Private.Windows.Core/src/System/Private/Windows/Ole/Composition.NativeToManagedAdapter.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.ComponentModel;
 using System.Reflection.Metadata;
 using System.Runtime.InteropServices;
 using System.Runtime.InteropServices.ComTypes;
@@ -146,7 +147,7 @@ private static unsafe MemoryStream ReadByteStreamFromHGLOBAL(HGLOBAL hglobal, ou
 
             try
             {
-                int size = (int)PInvokeCore.GlobalSize(hglobal);
+                int size = checked((int)PInvokeCore.GlobalSize(hglobal));
                 byte[] bytes = GC.AllocateUninitializedArray<byte>(size);
                 Marshal.Copy((nint)buffer, bytes, 0, size);
                 int index = 0;
@@ -169,28 +170,76 @@ private static unsafe MemoryStream ReadByteStreamFromHGLOBAL(HGLOBAL hglobal, ou
 
         private static unsafe string ReadStringFromHGLOBAL(HGLOBAL hglobal, bool unicode)
         {
-            string? stringData = null;
-
             void* buffer = PInvokeCore.GlobalLock(hglobal);
+            if (buffer is null)
+            {
+                throw new Win32Exception();
+            }
+
+            // https://learn.microsoft.com/windows/win32/dataxchg/standard-clipboard-formats
+            // https://learn.microsoft.com/windows/win32/shell/clipboard#cfstr_filename
+            //
+            // CF_TEXT, CF_OEMTEXT, CF_UNICODETEXT, and CFSTR_FILENAME are supposed to have a null terminator.
+            // If we cannot find one in the buffer, assume it is corrupted and return an empty string.
+            //
+            // Can't find the explicit docs for CF_RTF, but we've always treated it as null terminated.
+            // The RichText control itself null terminates but looks like it doesn't require it.
+            // Given our prior and "normal" behavior, we'll continue to expect a null terminator.
+
             try
             {
-                stringData = unicode ? new string((char*)buffer) : new string((sbyte*)buffer);
+                int size = checked((int)PInvokeCore.GlobalSize(hglobal));
+                if (size == 0)
+                {
+                    throw new Win32Exception();
+                }
+
+                if (unicode)
+                {
+                    ReadOnlySpan<char> chars = new((char*)buffer, size / sizeof(char));
+                    int nullIndex = chars.IndexOf('\0');
+                    if (nullIndex < 0)
+                    {
+                        // Malformed, return empty string.
+                        return string.Empty;
+                    }
+
+                    chars = chars[..nullIndex];
+                    return new string(chars);
+                }
+                else
+                {
+                    ReadOnlySpan<byte> bytes = new((byte*)buffer, size);
+                    int nullIndex = bytes.IndexOf((byte)0);
+                    if (nullIndex < 0)
+                    {
+                        // Malformed, return empty string.
+                        return string.Empty;
+                    }
+
+                    return new string((sbyte*)buffer, 0, nullIndex);
+                }
             }
             finally
             {
                 PInvokeCore.GlobalUnlock(hglobal);
             }
-
-            return stringData;
         }
 
         private static unsafe string ReadUtf8StringFromHGLOBAL(HGLOBAL hglobal)
         {
             void* buffer = PInvokeCore.GlobalLock(hglobal);
+            if (buffer is null)
+            {
+                throw new Win32Exception();
+            }
+
             try
             {
-                int size = (int)PInvokeCore.GlobalSize(hglobal);
-                return Encoding.UTF8.GetString((byte*)buffer, size - 1);
+                int size = checked((int)PInvokeCore.GlobalSize(hglobal));
+                return size == 0
+                    ? throw new Win32Exception()
+                    : Encoding.UTF8.GetString((byte*)buffer, size - 1);
             }
             finally
             {

src/System.Private.Windows.Core/tests/System.Private.Windows.Core.Tests/System/Private/Windows/Ole/NativeToManagedAdapterTests.cs

@@ -1,11 +1,14 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.ComponentModel;
 using System.Formats.Nrbf;
 using System.Private.Windows.BinaryFormat;
 using System.Text.Json;
 using Windows.Win32;
+using Windows.Win32.Foundation;
 using Windows.Win32.System.Com;
+using Windows.Win32.System.Memory;
 
 using Composition = System.Private.Windows.Ole.Composition<
     System.Private.Windows.Ole.MockOleServices<System.Private.Windows.Ole.NativeToManagedAdapterTests>,
@@ -108,4 +111,74 @@ public void GetData_CustomType_BinaryFormattedJson_AsSerializationRecord()
         composition.TryGetData(nameof(NativeToManagedAdapterTests), out SerializationRecord? data).Should().BeTrue();
         data!.TypeName.AssemblyQualifiedName.Should().Be("System.Private.Windows.JsonData, System.Private.Windows.VirtualJson");
     }
+
+    [Theory]
+    [BoolData]
+    public void ReadStringFromHGLOBAL_InvalidHGLOBAL_Throws(bool unicode)
+    {
+        Type type = typeof(Composition).GetFullNestedType("NativeToManagedAdapter");
+
+        Action action = () =>
+        {
+            string result = type.TestAccessor().Dynamic.ReadStringFromHGLOBAL(HGLOBAL.Null, unicode);
+        };
+
+        action.Should().Throw<Win32Exception>().And.HResult.Should().Be((int)HRESULT.E_FAIL);
+    }
+
+    [Theory]
+    [BoolData]
+    public void ReadStringFromHGLOBAL_NoTerminator_ReturnsEmptyString(bool unicode)
+    {
+        Type type = typeof(Composition).GetFullNestedType("NativeToManagedAdapter");
+
+        // There is no way to create a zero-length HGLOBAL, GlobalAlloc will always allocate at least some memory.
+        HGLOBAL global = PInvokeCore.GlobalAlloc(GLOBAL_ALLOC_FLAGS.GMEM_MOVEABLE, 6);
+        nuint size = PInvokeCore.GlobalSize(global);
+
+        try
+        {
+            using (GlobalBuffer buffer = new(global, (uint)size))
+            {
+                Span<byte> span = buffer.AsSpan();
+                // Fill spaces or daggers
+                span.Fill(0x20);
+            }
+
+            string result = type.TestAccessor().Dynamic.ReadStringFromHGLOBAL(global, unicode);
+            result.Should().BeEmpty();
+        }
+        finally
+        {
+            PInvokeCore.GlobalFree(global);
+        }
+    }
+
+    [Theory]
+    [BoolData]
+    public void ReadStringFromHGLOBAL_Terminator_ReturnsString(bool unicode)
+    {
+        Type type = typeof(Composition).GetFullNestedType("NativeToManagedAdapter");
+
+        // There is no way to create a zero-length HGLOBAL, GlobalAlloc will always allocate at least some memory.
+        HGLOBAL global = PInvokeCore.GlobalAlloc(GLOBAL_ALLOC_FLAGS.GMEM_MOVEABLE | GLOBAL_ALLOC_FLAGS.GMEM_ZEROINIT, 6);
+        nuint size = PInvokeCore.GlobalSize(global);
+
+        try
+        {
+            using (GlobalBuffer buffer = new(global, (uint)size))
+            {
+                Span<byte> span = buffer.AsSpan();
+                // Fill spaces or daggers, leave the last two bytes as zero
+                span[..^2].Fill(0x20);
+            }
+
+            string result = type.TestAccessor().Dynamic.ReadStringFromHGLOBAL(global, unicode);
+            result.Should().NotBeEmpty();
+        }
+        finally
+        {
+            PInvokeCore.GlobalFree(global);
+        }
+    }
 }

src/System.Windows.Forms.Design/tests/UnitTests/System/Windows/Forms/Design/ListViewGroupCollectionEditorTests.cs

@@ -2,7 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.ComponentModel;
-using System.Reflection;
 using Moq;
 
 namespace System.Windows.Forms.Design.Tests;
@@ -56,14 +55,4 @@ public void CreateInstance_CreatesListViewGroupWithUniqueName()
         result?.Name.Should().StartWith("ListViewGroup");
         result?.GetType().Should().Be(typeof(ListViewGroup));
     }
-
-    [Fact]
-    public void CreateInstance_ThrowsException_WhenEditValueIsNull()
-    {
-        _mockEditor.Object.TestAccessor().Dynamic._editValue = null;
-
-        Action action = () => _mockEditor.Object.TestAccessor().Dynamic.CreateInstance(typeof(ListViewGroup));
-
-        action.Should().Throw<TargetInvocationException>();
-    }
 }

src/test/unit/System.Windows.Forms/System/Windows/Forms/DataGridViewLinkCellTests.cs

@@ -2,7 +2,6 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.Drawing;
-using System.Reflection;
 
 namespace System.Windows.Forms.Tests;
 
@@ -655,7 +654,7 @@ public void Paint_ThrowsArgumentNullException_WhenCellStyleIsNull()
         using Bitmap bitmap = new(10, 10);
         using Graphics graphics = Graphics.FromImage(bitmap);
 
-        TargetInvocationException ex = ((Action)(() =>
+        ((Action)(() =>
             _cell.TestAccessor().Dynamic.Paint(
                 graphics,
                 new Rectangle(0, 0, 10, 10),
@@ -669,9 +668,6 @@ public void Paint_ThrowsArgumentNullException_WhenCellStyleIsNull()
                 null,
                 DataGridViewPaintParts.All
             )
-        )).Should().Throw<TargetInvocationException>().Subject.First();
-
-        ex.InnerException.Should().BeOfType<ArgumentNullException>();
-        ex.InnerException!.Message.Should().Contain("cellStyle");
+        )).Should().Throw<ArgumentNullException>().And.Message.Should().Contain("cellStyle");
     }
 }