Fix 5 unhandled exceptions from malformed expression strings (Issue #973)

Agent-Logs-Url: https://github.com/zzzprojects/System.Linq.Dynamic.Core/sessions/f8cf5eb5-b143-4a52-b59a-624b4cf8a47f

Co-authored-by: StefH <249938+StefH@users.noreply.github.com>

Author: Copilot

src/System.Linq.Dynamic.Core/DynamicClassFactory.cs

@@ -289,31 +289,40 @@ private static Type EmitType(IList<DynamicProperty> properties, bool createParam
         {
             var fieldName = properties[i].Name;
             var fieldType = properties[i].Type;
-            var equalityComparerT = EqualityComparer.MakeGenericType(fieldType);
+
+            // Use object-based equality comparer for types that are not publicly accessible from
+            // the dynamic assembly (e.g., compiler-generated anonymous types). Calling
+            // EqualityComparer<T>.get_Default() for a non-public T will throw MethodAccessException.
+            var fieldTypeIsAccessible = IsTypePubliclyAccessible(fieldType);
+            var equalityType = fieldTypeIsAccessible ? fieldType : typeof(object);
+            var equalityComparerT = EqualityComparer.MakeGenericType(equalityType);
 
             // Equals()
             MethodInfo equalityComparerTDefault = equalityComparerT.GetMethod("get_Default", BindingFlags.Static | BindingFlags.Public)!;
-            MethodInfo equalityComparerTEquals = equalityComparerT.GetMethod(nameof(EqualityComparer.Equals), BindingFlags.Instance | BindingFlags.Public, null, [fieldType, fieldType], null)!;
+            MethodInfo equalityComparerTEquals = equalityComparerT.GetMethod(nameof(EqualityComparer.Equals), BindingFlags.Instance | BindingFlags.Public, null, [equalityType, equalityType], null)!;
 
             // Illegal one-byte branch at position: 9. Requested branch was: 143.
             // So replace OpCodes.Brfalse_S to OpCodes.Brfalse
             ilgeneratorEquals.Emit(OpCodes.Brfalse, equalsLabel);
             ilgeneratorEquals.Emit(OpCodes.Call, equalityComparerTDefault);
             ilgeneratorEquals.Emit(OpCodes.Ldarg_0);
             ilgeneratorEquals.Emit(OpCodes.Ldfld, fieldBuilders[i]);
+            if (!fieldTypeIsAccessible) ilgeneratorEquals.Emit(OpCodes.Box, fieldType);
             ilgeneratorEquals.Emit(OpCodes.Ldloc_0);
             ilgeneratorEquals.Emit(OpCodes.Ldfld, fieldBuilders[i]);
+            if (!fieldTypeIsAccessible) ilgeneratorEquals.Emit(OpCodes.Box, fieldType);
             ilgeneratorEquals.Emit(OpCodes.Callvirt, equalityComparerTEquals);
 
             // GetHashCode();
-            MethodInfo equalityComparerTGetHashCode = equalityComparerT.GetMethod(nameof(EqualityComparer.GetHashCode), BindingFlags.Instance | BindingFlags.Public, null, [fieldType], null)!;
+            MethodInfo equalityComparerTGetHashCode = equalityComparerT.GetMethod(nameof(EqualityComparer.GetHashCode), BindingFlags.Instance | BindingFlags.Public, null, [equalityType], null)!;
             ilgeneratorGetHashCode.Emit(OpCodes.Stloc_0);
             ilgeneratorGetHashCode.Emit(OpCodes.Ldc_I4, -1521134295);
             ilgeneratorGetHashCode.Emit(OpCodes.Ldloc_0);
             ilgeneratorGetHashCode.Emit(OpCodes.Mul);
             ilgeneratorGetHashCode.Emit(OpCodes.Call, equalityComparerTDefault);
             ilgeneratorGetHashCode.Emit(OpCodes.Ldarg_0);
             ilgeneratorGetHashCode.Emit(OpCodes.Ldfld, fieldBuilders[i]);
+            if (!fieldTypeIsAccessible) ilgeneratorGetHashCode.Emit(OpCodes.Box, fieldType);
             ilgeneratorGetHashCode.Emit(OpCodes.Callvirt, equalityComparerTGetHashCode);
             ilgeneratorGetHashCode.Emit(OpCodes.Add);
 
@@ -422,6 +431,32 @@ private static Type EmitType(IList<DynamicProperty> properties, bool createParam
         return typeBuilder.CreateType();
     }
 
+    /// <summary>
+    /// Determines whether a type is publicly accessible from an external (dynamic) assembly.
+    /// Non-public types (e.g., compiler-generated anonymous types) cannot be used as generic
+    /// type arguments in EqualityComparer&lt;T&gt; from a dynamic assembly without causing
+    /// a <see cref="MethodAccessException"/> at runtime.
+    /// </summary>
+    private static bool IsTypePubliclyAccessible(Type type)
+    {
+        // Check if the type itself is public
+        if (!type.IsPublic && !type.IsNestedPublic)
+        {
+            return false;
+        }
+
+        // For constructed generic types (e.g., List<MyPrivateType>),
+        // all type arguments must also be publicly accessible.
+        // Generic type definitions (e.g., List<>) have unbound type parameters
+        // which are not concrete types and should not be checked.
+        if (type.IsGenericType && !type.IsGenericTypeDefinition)
+        {
+            return type.GetGenericArguments().All(IsTypePubliclyAccessible);
+        }
+
+        return true;
+    }
+
     private static void EmitEqualityOperators(TypeBuilder typeBuilder, MethodBuilder equals)
     {
         // Define the '==' operator

src/System.Linq.Dynamic.Core/Parser/ExpressionHelper.cs

@@ -344,6 +344,13 @@ public bool TryGenerateAndAlsoNotNullExpression(Expression sourceExpression, boo
             .Select(expression => Expression.NotEqual(expression, _nullExpression))
             .ToArray();
 
+        // If no nullable expressions were found, return false (nothing to null-check)
+        if (binaryExpressions.Length == 0)
+        {
+            generatedExpression = sourceExpression;
+            return false;
+        }
+
         // Convert all binary expressions into `AndAlso(...)`
         generatedExpression = binaryExpressions[0];
         for (int i = 1; i < binaryExpressions.Length; i++)

src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs

@@ -928,7 +928,7 @@ private AnyOf<Expression, Type> ParseStringLiteral(bool forceParseAsString)
 
         if (_textParser.CurrentToken.Text[0] == '\'')
         {
-            if (parsedStringValue.Length > 1)
+            if (parsedStringValue.Length != 1)
             {
                 throw ParseError(Res.InvalidCharacterLiteral);
             }
@@ -1458,12 +1458,18 @@ private Expression ParseNew()
             arrayInitializer = true;
         }
 
+        // Track the opening token to enforce matching close token
+        var openTokenId = _textParser.CurrentToken.Id;
         _textParser.NextToken();
 
+        // Determine the expected closing token based on the opening token
+        var closeTokenId = openTokenId == TokenId.OpenParen ? TokenId.CloseParen : TokenId.CloseCurlyParen;
+
         var properties = new List<DynamicProperty>();
         var expressions = new List<Expression>();
+        var propertyNames = new HashSet<string>(StringComparer.Ordinal);
 
-        while (_textParser.CurrentToken.Id != TokenId.CloseParen && _textParser.CurrentToken.Id != TokenId.CloseCurlyParen)
+        while (_textParser.CurrentToken.Id != closeTokenId)
         {
             int exprPos = _textParser.CurrentToken.Pos;
             Expression expr = ParseConditionalOperator();
@@ -1483,7 +1489,7 @@ private Expression ParseNew()
                             && methodCallExpression.Arguments.Count == 1
                             && methodCallExpression.Arguments[0] is ConstantExpression methodCallExpressionArgument
                             && methodCallExpressionArgument.Type == typeof(string)
-                            && properties.All(x => x.Name != (string?)methodCallExpressionArgument.Value))
+                            && !propertyNames.Contains((string?)methodCallExpressionArgument.Value ?? string.Empty))
                         {
                             propName = (string?)methodCallExpressionArgument.Value;
                         }
@@ -1496,6 +1502,11 @@ private Expression ParseNew()
 
                 if (!string.IsNullOrEmpty(propName))
                 {
+                    if (!propertyNames.Add(propName!))
+                    {
+                        throw ParseError(exprPos, Res.DuplicateIdentifier, propName);
+                    }
+
                     properties.Add(new DynamicProperty(propName!, expr.Type));
                 }
             }
@@ -1510,7 +1521,7 @@ private Expression ParseNew()
             _textParser.NextToken();
         }
 
-        if (_textParser.CurrentToken.Id != TokenId.CloseParen && _textParser.CurrentToken.Id != TokenId.CloseCurlyParen)
+        if (_textParser.CurrentToken.Id != closeTokenId)
         {
             throw ParseError(Res.CloseParenOrCommaExpected);
         }
@@ -2393,7 +2404,24 @@ private Expression ParseElementAccess(Expression expr)
                         : args[i];
                 }
 
-                return Expression.Call(expr, indexMethod, indexArgumentExpressions);
+                var callExpr = Expression.Call(expr, indexMethod, indexArgumentExpressions);
+
+                // For constant expressions with constant arguments, evaluate at parse time to
+                // produce a ParseException instead of a runtime exception (e.g., index out of bounds).
+                if (expr is ConstantExpression && args.All(a => a is ConstantExpression))
+                {
+                    try
+                    {
+                        var value = Expression.Lambda(callExpr).Compile().DynamicInvoke();
+                        return Expression.Constant(value, callExpr.Type);
+                    }
+                    catch (Exception ex)
+                    {
+                        throw ParseError(errorPos, (ex.InnerException ?? ex).Message);
+                    }
+                }
+
+                return callExpr;
 
             default:
                 throw ParseError(errorPos, Res.AmbiguousIndexerInvocation, TypeHelper.GetTypeName(expr.Type));

test/System.Linq.Dynamic.Core.Tests/Issues/Issue973.cs

@@ -0,0 +1,115 @@
+using System.Linq.Dynamic.Core.Exceptions;
+using FluentAssertions;
+using Xunit;
+
+namespace System.Linq.Dynamic.Core.Tests;
+
+/// <summary>
+/// Tests for Issue #973: Multiple unhandled exceptions from malformed expression strings (5 crash sites found via fuzzing).
+/// All 5 bugs should throw <see cref="ParseException"/> instead of unhandled runtime exceptions.
+/// </summary>
+public partial class QueryableTests
+{
+
+    /// <summary>
+    /// Bug 1: ParseStringLiteral IndexOutOfRangeException for empty single-quoted string literal ''.
+    /// </summary>
+    [Fact]
+    public void Issue973_Bug1_EmptyCharLiteral_ShouldThrowParseException()
+    {
+        // Arrange
+        var items = new[] { new { Id = 1, Name = "Alice" } }.AsQueryable();
+
+        // Act
+        Action act = () => items.Where("''");
+
+        // Assert
+        act.Should().Throw<ParseException>();
+    }
+
+    /// <summary>
+    /// Bug 2: TryGenerateAndAlsoNotNullExpression IndexOutOfRangeException for malformed np() call.
+    /// </summary>
+    [Fact]
+    public void Issue973_Bug2_MalformedNpCall_ShouldThrowParseException()
+    {
+        // Arrange
+        var items = new[] { new { Id = 1, Name = "Alice" } }.AsQueryable();
+
+        // Act
+        Action act = () => items.Where("-np(--9999999)9999T--99999999999)99999");
+
+        // Assert
+        act.Should().Throw<ParseException>();
+    }
+
+    /// <summary>
+    /// Bug 3: Compiled expression IndexOutOfRangeException for string indexer with out-of-bounds constant index.
+    /// </summary>
+    [Fact]
+    public void Issue973_Bug3_StringIndexerOutOfBounds_ShouldThrowParseException()
+    {
+        // Arrange
+        var items = new[] { new { Id = 1, Name = "Alice" } }.AsQueryable();
+
+        // Act
+        Action act = () => items.OrderBy("\"ab\"[3]").ToList();
+
+        // Assert
+        act.Should().Throw<ParseException>();
+    }
+
+    /// <summary>
+    /// Bug 3 (valid case): String indexer with in-bounds index should work correctly.
+    /// </summary>
+    [Fact]
+    public void Issue973_Bug3_StringIndexerInBounds_ShouldWork()
+    {
+        // Arrange
+        var items = new[] { new { Id = 1, Name = "Alice" } }.AsQueryable();
+
+        // Act - "ab"[0] = 'a', "ab"[1] = 'b' are valid
+        var result0 = items.OrderBy("\"ab\"[0]").ToList();
+        var result1 = items.OrderBy("\"ab\"[1]").ToList();
+
+        // Assert
+        result0.Should().HaveCount(1);
+        result1.Should().HaveCount(1);
+    }
+
+    /// <summary>
+    /// Bug 4: NullReferenceException in compiled Select projection with mismatched brace/paren in new().
+    /// </summary>
+    [Fact]
+    public void Issue973_Bug4_MismatchedBraceInNewExpression_ShouldThrowParseException()
+    {
+        // Arrange
+        var items = new[] { new { Id = 1, Name = "Alice", Age = 30, Score = 85.5, Active = true } }.AsQueryable();
+
+        // Act
+        Action act = () => items.Select("new(Name }. AgAkQ & 1111555555+55555-5555555+555555+55555-55555").ToDynamicList();
+
+        // Assert
+        act.Should().Throw<ParseException>();
+    }
+
+    /// <summary>
+    /// Bug 5: MethodAccessException from $ identifier in GroupBy due to duplicate property names.
+    /// </summary>
+    [Fact]
+    public void Issue973_Bug5_DollarIdentifierWithDuplicatePropertyNames_ShouldThrowParseException()
+    {
+        // Arrange
+        var items = new[]
+        {
+            new { Id = 1, Name = "Alice", Age = 30, Score = 85.5, Active = true },
+            new { Id = 2, Name = "Bob", Age = 25, Score = 92.0, Active = false },
+        }.AsQueryable();
+
+        // Act
+        Action act = () => items.GroupBy("new($ as DoubleAge, Age + 2 as DoubleAge)").ToDynamicList();
+
+        // Assert - duplicate property name 'DoubleAge' should cause a ParseException
+        act.Should().Throw<ParseException>();
+    }
+}