diff --git a/.gitignore b/.gitignore index 27896dcc..26cb01a0 100644 --- a/.gitignore +++ b/.gitignore @@ -238,3 +238,4 @@ _Pvt_Extensions /coverage.xml /dynamic-coverage-*.xml /test/**/coverage.net8.0.opencover.xml +.nuget/ diff --git a/src/System.Linq.Dynamic.Core/DynamicClassFactory.cs b/src/System.Linq.Dynamic.Core/DynamicClassFactory.cs index 1b4a1eef..e84d508d 100644 --- a/src/System.Linq.Dynamic.Core/DynamicClassFactory.cs +++ b/src/System.Linq.Dynamic.Core/DynamicClassFactory.cs @@ -289,11 +289,16 @@ private static Type EmitType(IList 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.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 @@ -301,12 +306,14 @@ private static Type EmitType(IList properties, bool createParam 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); @@ -314,6 +321,7 @@ private static Type EmitType(IList properties, bool createParam 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); @@ -419,7 +427,33 @@ private static Type EmitType(IList properties, bool createParam EmitEqualityOperators(typeBuilder, equals); - return typeBuilder.CreateType(); + return typeBuilder.CreateType()!; + } + + /// + /// 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<T> from a dynamic assembly without causing + /// a at runtime. + /// + private static bool IsTypePubliclyAccessible(Type typeInfo) + { + // Check if the type itself is public + if (!typeInfo.GetTypeInfo().IsPublic && !typeInfo.GetTypeInfo().IsNestedPublic) + { + return false; + } + + // For constructed generic types (e.g., List), + // 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 (typeInfo.GetTypeInfo().IsGenericType && !typeInfo.GetTypeInfo().IsGenericTypeDefinition) + { + return typeInfo.GetGenericArguments().All(IsTypePubliclyAccessible); + } + + return true; } private static void EmitEqualityOperators(TypeBuilder typeBuilder, MethodBuilder equals) diff --git a/src/System.Linq.Dynamic.Core/Parser/ExpressionHelper.cs b/src/System.Linq.Dynamic.Core/Parser/ExpressionHelper.cs index a9b31876..2b830a1e 100644 --- a/src/System.Linq.Dynamic.Core/Parser/ExpressionHelper.cs +++ b/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++) diff --git a/src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs b/src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs index 20524d36..f9179323 100644 --- a/src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs +++ b/src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs @@ -928,7 +928,7 @@ private AnyOf 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(); var expressions = new List(); + var propertyNames = new HashSet(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)); diff --git a/test/System.Linq.Dynamic.Core.Tests/Issues/Issue973.cs b/test/System.Linq.Dynamic.Core.Tests/Issues/Issue973.cs new file mode 100644 index 00000000..87721100 --- /dev/null +++ b/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; + +/// +/// Tests for Issue #973: Multiple unhandled exceptions from malformed expression strings (5 crash sites found via fuzzing). +/// All 5 bugs should throw instead of unhandled runtime exceptions. +/// +public partial class QueryableTests +{ + + /// + /// Bug 1: ParseStringLiteral IndexOutOfRangeException for empty single-quoted string literal ''. + /// + [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(); + } + + /// + /// Bug 2: TryGenerateAndAlsoNotNullExpression IndexOutOfRangeException for malformed np() call. + /// + [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(); + } + + /// + /// Bug 3: Compiled expression IndexOutOfRangeException for string indexer with out-of-bounds constant index. + /// + [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(); + } + + /// + /// Bug 3 (valid case): String indexer with in-bounds index should work correctly. + /// + [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); + } + + /// + /// Bug 4: NullReferenceException in compiled Select projection with mismatched brace/paren in new(). + /// + [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(); + } + + /// + /// Bug 5: MethodAccessException from $ identifier in GroupBy due to duplicate property names. + /// + [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(); + } +}