Fix ExpressionKeyVisitor to produce unique keys for anon types

[mkasperski]

Fixes #1526

[fredericDelaporte]

The test works, demonstrating the bug before the fix, provided the type here keeps the same naming than the one in GH1526. Otherwise it will always succeed, bug fixed or not.

[fredericDelaporte]

I would rather have this file moved to NHibernate.DomainModel\NHSpecific folder and so the AnonymousTypeExpressionProviderFromNHibernateDomainModelAssembly moved to NHibernate.DomainModel.NHSpecific namespace.

[fredericDelaporte]

This expression must remain identical to the one into GH1526. Otherwise it will always succeed, bug fixed or not.

For this reason and the previous comments, I would change this file to something like:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;

// Simulates compiler-generated, non-namespaced anonymous type
internal class AnonymousType1
{
	public string Name { get; set; }
}

namespace NHibernate.DomainModel.NHSpecific
{
	public static class AnonymousTypeExpressionProvider
	{
		public static System.Type GetAnonymousTypeFromNHibernateDomainModelAssembly()
		{
			return typeof(AnonymousType1);
		}

		// Produces an Expression that has the above AnonymousType1 embedded in it
		public static Expression GetExpressionFromNHibernateDomainModelAssembly()
		{
			return GetExpression<AnonymousType1>();
		}

		public static Expression GetExpression<T>() where T : new ()
		{
			Expression<Func<IList<T>>> exp = () =>
				Enumerable.Empty<object>()
					.Select(c => new T())
					.ToList();

			return exp;
		}
	}
}

Then in GH1526:

using System.Linq.Expressions;
using NHibernate.DomainModel.NHSpecific;

// Simulates compiler-generated, non-namespaced anonymous type
internal class AnonymousType1
{
	public string Name { get; set; }
}

namespace NHibernate.Test.NHSpecificTest.GH1526
{
	// Produces an Expression that has the above AnonymousType1 embedded in it
	public static class AnonymousTypeExpressionProviderFromNHibernateTestAssembly
	{
		public static System.Type GetAnonymousType()
		{
			return typeof(AnonymousType1);
		}

		public static Expression GetExpression()
		{
			return AnonymousTypeExpressionProvider.GetExpression<AnonymousType1>();
		}
	}
}

With:

using NHibernate.DomainModel.NHSpecific;
using NHibernate.Linq.Visitors;
using NUnit.Framework;

namespace NHibernate.Test.NHSpecificTest.GH1526
{
	[TestFixture]
	public class Fixture
	{
		[Test]
		public void ShouldCreateDifferentKeyForAnonymousTypesFromDifferentAssemblies()
		{
			var anon1 = AnonymousTypeExpressionProviderFromNHibernateTestAssembly.GetAnonymousType();
			var anon2 = AnonymousTypeExpressionProvider.GetAnonymousTypeFromNHibernateDomainModelAssembly();
			Assert.That(anon1.FullName, Is.EqualTo(anon2.FullName), "The two tested types must have the same FullName for demonstrating the bug.");

			var exp1 = AnonymousTypeExpressionProviderFromNHibernateTestAssembly.GetExpression();
			var exp2 = AnonymousTypeExpressionProvider.GetExpressionFromNHibernateDomainModelAssembly();

			var key1 = ExpressionKeyVisitor.Visit(exp1, null);
			var key2 = ExpressionKeyVisitor.Visit(exp2, null);

			Assert.That(key1, Is.Not.EqualTo(key2));
		}
	}
}

[hazzik]

I think this fix needs to be applied to all types in the ExpressionKeyVisitor and not only to MethodCalls

[mkasperski]

I was gonna suggest (after fixing this case) even wider approach, in which any occurence of FullName is inspected to see whether its usage is based on the same false assumption demonstrated in this PR ("equal FullName implies equal type"). I'm not that experienced in NH codebase though to analyze each case, so with this PR I just wanted to "plant a seed of suspicion" and present a setup in which such cases could possibly be tested.

As for more cases inside ExpressionKeyVisitor, I did notice other visiting methods that use type metadata, I just didn't have a proof that they are vulnerable or even possible to trigger (an unary expression on anonymous type?). Let me clean up this PR after current review and then I'll see if I can find more linq queries that still produce colliding cache keys. If you have suggestions for them already, that would be very helpful.

[fredericDelaporte]

The only other FullName in ExpressionKeyVisitor is for the TypeBinaryExpression visitor, which should be visited on expressions like someThing is someType ? value1 : value2. Such expressions can be useful with models using inheritance. It is likely to have the same vulnerability. Of course with actual anonymous types, there is no reason to have inheritance involved. But the bug here is a bit wider, since it occurs for types having the same full name, which can occur on non anonymous types too.

There are also some Type.Name usages, in VisitBinary and VisitNew.

I can easily produce a collision with VisitNew by using the expression (new AnonymousType1()).Name, although I do not see much practical usages in an actual query for this expression. Still it can be fixed too for being on the safe side.

About VisiteBinary, a collision is doable too but even more contrived. (It implies defining an operator on the "anonymous type" class.) But likewise, it can also be fixed.

[fredericDelaporte]

Better provide a non-null second argument(new Dictionary<ConstantExpression, NamedParameter>()). Although it lacks some pre-condition checks, ExpressionKeyVisitor does not actually support receiving it as null, and will fail on some expressions if it was null.

[mkasperski]

I've followed with your remark in the new commit. However, I believe the real change should be in ExpressionKeyVisitor itself, to either a.) make the second arg optional, or b.) default to new Dictionary<...> upon receiving null in Visit/ctor.

a.) would be cleaner, although would be a breaking change, since the type is public (does it have to be public, though?)
b.) would be the opposite: non-breaking, but leaving the method's api somewhat dirty

The method is called more times with null than with real dictionary, so some kind of cleanup there seems due. Separate PR, though, I reckon.

[fredericDelaporte]

The method is called more times with null than with real dictionary,

Indeed. Not a good thing since it may then throw a null ref exception.

I am not for b., this just hides the trouble. Well, a. maybe too. I would rather instead, if some change is done (but it is not the subject of this PR), throw an InvalidOperationException in VisitConstant if _constantToParameterMap is null.

[fredericDelaporte]

#1536 added for this subject.

[fredericDelaporte]

VisitBinary case is not handled by this PR, but I do not think any supported Linq expression could cause a collision due to this. So I am approving this PR in its current state.