NH-3954 - Dynamic proxy cache may yield a wrong proxy #561

fredericDelaporte · 2017-03-03T04:08:37Z

Fixes NH-3954 - Dynamic proxy cache may yield a wrong proxy.

It relies on a dictionary cache, using a key which was implementing equals as hashcode equality.
But hascode are not unique, and two different objects, even of the same class with different properties/members values, may have the same hashcode. This is documented on msdn.

Quite unlikely, but it happened to me this night. While testing some other changes, StatelessSessionFetchingTest.DynamicFetch was unexpectedly failing. And this was because some other proxy got cached with the same hashcode as the proxy required by this test.

hazzik · 2017-03-03T04:17:24Z

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs

+			if (_uniqueInterfaces == null)
+				_uniqueInterfaces = new HashSet<System.Type>(Interfaces);
+			if (that._uniqueInterfaces == null)
+				that._uniqueInterfaces = new HashSet<System.Type>(Interfaces);


this shall be

that._uniqueInterfaces = new HashSet<System.Type>(that.Interfaces);

(note that. in front of Interfaces)

Damn, I should not code that late...

Fixed, squashed, ... I think this Serializable attribute is indeed unneeded for current usage of that class. Removing it would allow a simpler implementation. Maybe I have been too conservative here.

fredericDelaporte · 2017-03-03T04:53:27Z

I forget to mention in my PR message: on Jira side in a comment, I have listed a bunch of other classes having the same defect in NHibernate. This PR does not fix them.

fredericDelaporte · 2017-03-03T08:48:37Z

The TeamCity failure occured for MySql build, new test failure in ExpressionSessionLeakTest.SessionGetsCollected.

It appears that it has not fail on the fisrt build for this PR, executed on code before the fix I have done in response to Hazzik's review.

Due to the last change being very small, I doubt this failure has something to do with the change. Is it a test case known for unexpected failures?
(Furthermore, debugging the test and setting breakpoint in ProxyCache, it appears this test does not use this feature.)

fredericDelaporte · 2017-03-04T12:13:59Z

I have made changes to tests: added direct tests on Equals, set cache tests as only run explicitly since a bit unpredictable, and simplified test implementation. (Tests locally re-tested both with original code and fixed code.)

fredericDelaporte · 2017-03-16T16:08:31Z

Updated: was re-implementing set equality, used .Net built-in method instead. And re-based.

fredericDelaporte · 2017-03-19T10:05:49Z

Rebased for resolving a conflict on test project.

hazzik · 2017-03-19T22:35:58Z

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs

+			if (that._uniqueInterfaces == null)
+				that._uniqueInterfaces = new HashSet<System.Type>(that.Interfaces);
+
+			return _uniqueInterfaces.SetEquals(that._uniqueInterfaces);


SetEquals does not require the argument to be ISet<>, so the second if (if (that._uniqueInterfaces == null)) is not required

hazzik · 2017-03-19T22:39:51Z

There is a possibility that two instances of ProxyCacheEntry will be semantically equal (have the same base type and interfaces) but falsely declared unequal due the fact that set is not ordered, especially after deserialization. Could you please check the deserialization case?

hazzik

Please also implement IEquatable<>

hazzik · 2017-03-19T22:46:56Z

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs

@@ -43,15 +44,26 @@ public ProxyCacheEntry(System.Type baseType, System.Type[] interfaces)
 		public System.Type BaseType { get; private set; }
 		public System.Type[] Interfaces { get; private set; }


As we are allowed to break everything in NH5, Please make this IReadOnlyCollection<System.Type> without setter.

Fixed. I was talking about setter, of cource

hazzik · 2017-03-19T22:47:11Z

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs

@@ -43,15 +44,26 @@ public ProxyCacheEntry(System.Type baseType, System.Type[] interfaces)
 		public System.Type BaseType { get; private set; }


Please remove setter as per below.

hazzik · 2017-03-19T22:49:05Z

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs

@@ -24,6 +24,7 @@ public ProxyCacheEntry(System.Type baseType, System.Type[] interfaces)
 			}
 			BaseType = baseType;
 			Interfaces = interfaces ?? new System.Type[0];
+			_uniqueInterfaces = new HashSet<System.Type>(Interfaces);


I think, what we need is presort Interfaces (eg. interfaces.Distinct().OrderBy(x=>x.FullName).ToArray()).

SetEquals is not sensible to order.

This method ignores the order of elements and any duplicate elements in other.

So I do not expect any order mismatch to cause an issue.

Order mismatch can (?) happen in the calculation of hashCode

Ok, no it can not, as hashCode is calculated using just xor (which is strange as there will be lots of collisions)

But there is the hashcode computation too. Am I wrong thinking xor is permutable? (Will yield same result whatever the evaluation order.) I am not completly sure about that, and do not find a reference for it currently. But I can not find an exemple for which it would be false. (And I have a test case which test types permutation and succeed.)

There is a second HashSet used on the one I have introduced, which is now useless and could be replaced by a simple List. I will change that too.

Ok, no it can not, as hashCode is calculated using just xor (which is strange as there will be lots of collisions)

When used to combine hashcode from a set (so different objects), it should not be that bad. It is not like & or | which ends up zeroing or "one-ing" all bits. It seems it is even a common practice in Java.

Now if we want to change it for a more classical approach like multiplying by a prime then adding next value while overflowing, we will need to take care of the order.

hazzik · 2017-03-19T22:49:51Z

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs

 		public override bool Equals(object obj)
 		{
 			var that = obj as ProxyCacheEntry;
-			if (ReferenceEquals(null, that))
+			if (ReferenceEquals(null, that) ||
+				hashCode != that.hashCode ||


I think hashCode check is redundant here, but it's ok to leave it as is.

I left it as an easy early "exit case". It will most of time speed up non equal cases, while being a very minor overhead for equal cases. I should add a comment for warning the hashcode is only good for early false yielding, not for telling it is equal.

fredericDelaporte · 2017-03-19T23:13:34Z

Could you please check the deserialization case?

About this serialization, I was wondering:

I think this Serializable attribute is indeed unneeded for current usage of that class. Removing it would allow a simpler implementation. Maybe I have been too conservative here.

Is there a known reason for having this class serializable? It may be something inherited from the LinFu library but maybe useless for NHibernate.

I'll take another look at this.

fredericDelaporte · 2017-03-20T01:02:01Z

A more convoluted test case is added, with many interfaces, different order, duplicates, resolving to same set.

Code changed as suggested (with a bit more c#6/7 syntax, and more comments).

Additionally, I have tested the 5 thousands tests without this [Serializable] on ProxyEntryCache, no failure. (I have not committed this removal.)

fredericDelaporte · 2017-03-20T01:08:43Z

Hum, team city build failed. Maybe using C#7 features is too early. I removed the "throw expression".

hazzik · 2017-03-20T02:56:03Z

Yep, no C#7 until #569 merged

fredericDelaporte · 2017-03-26T10:40:17Z

Rebased (& commit reworded). I do not merge as you were willing to check if serialization support was important. (I personally do not understand in which cases we need this to be serializable. It looks to me this attribute has been put on many NHibernate classes "just in case", without actual needs.)

hazzik

We need to remove [Serializable] and simplify implementation.

fredericDelaporte · 2017-03-26T21:28:20Z

Rebased and done.

hazzik · 2017-03-26T22:21:25Z

src/NHibernate/Proxy/DynamicProxy/ProxyCacheEntry.cs


-			if (Interfaces.Length == 0)
+			if (Interfaces.Count == 0)


Please change this to _uniqueInterfaces.Count == 0

fredericDelaporte · 2017-03-27T00:10:00Z

Well, I just discover following test: Legacy.MasterDetailTests.Serialize. It does serialize a whole ISession, deserialize it and goes on using it... So if we want to maintain this possibility, cache entries should remain serializable. But I would still keep a simplified implementation of it, the hashtable can be serialized. I would just add back that [Serializable] attribute.

hazzik · 2017-03-27T00:18:46Z

Does it fail?

fredericDelaporte · 2017-03-27T00:47:03Z

No, but very likely because it does not try to serialize a session having cached some dynamic proxy.

fredericDelaporte · 2017-03-27T00:50:22Z

I will try to cause this serialization test to have a cached proxy.

hazzik · 2017-03-27T00:54:37Z

The ProxyFactory and its dependencies do not get serialized. It's all fine here.

fredericDelaporte · 2017-03-27T01:00:43Z

Ok, great, thanks.

hazzik reviewed Mar 3, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-3954 branch from 4c13e52 to 81abf6c Compare March 3, 2017 04:45

hazzik self-requested a review March 3, 2017 10:29

fredericDelaporte force-pushed the NH-3954 branch from 81abf6c to 08996f4 Compare March 4, 2017 12:11

fredericDelaporte changed the title ~~NH-3954: wrong equality fix and test cases.~~ NH-3954: Dynamic proxy cache may yield a wrong proxy Mar 4, 2017

fredericDelaporte force-pushed the NH-3954 branch from 08996f4 to 86d116b Compare March 16, 2017 16:06

fredericDelaporte changed the title ~~NH-3954: Dynamic proxy cache may yield a wrong proxy~~ NH-3954 - Dynamic proxy cache may yield a wrong proxy Mar 18, 2017

fredericDelaporte force-pushed the NH-3954 branch from 86d116b to 9d81858 Compare March 19, 2017 10:05

fredericDelaporte force-pushed the NH-3954 branch from 9d81858 to 383c1ee Compare March 19, 2017 10:13

hazzik reviewed Mar 19, 2017

View reviewed changes

hazzik previously requested changes Mar 19, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-3954 branch 2 times, most recently from 14896de to 8e3b52a Compare March 20, 2017 00:50

fredericDelaporte force-pushed the NH-3954 branch from 8e3b52a to c9ef58d Compare March 20, 2017 01:06

fredericDelaporte force-pushed the NH-3954 branch from c9ef58d to 82108c8 Compare March 26, 2017 10:39

fredericDelaporte force-pushed the NH-3954 branch 2 times, most recently from 226fef9 to c3ce0f7 Compare March 26, 2017 12:00

hazzik requested changes Mar 26, 2017

View reviewed changes

fredericDelaporte force-pushed the NH-3954 branch from c3ce0f7 to aa6e51f Compare March 26, 2017 21:27

hazzik reviewed Mar 26, 2017

View reviewed changes

NH-3954 - Dynamic proxy cache collision fix and test cases.

c8a7327

fredericDelaporte force-pushed the NH-3954 branch from aa6e51f to c8a7327 Compare March 26, 2017 22:26

hazzik approved these changes Mar 26, 2017

View reviewed changes

hazzik merged commit c8a7327 into nhibernate:master Mar 26, 2017

fredericDelaporte deleted the NH-3954 branch March 26, 2017 23:00

fredericDelaporte added this to the 5.0.0 milestone Apr 1, 2017

hazzik added the r: Fixed label Aug 3, 2017

fredericDelaporte added the t: Bug label Sep 6, 2017

		@@ -43,15 +44,26 @@ public ProxyCacheEntry(System.Type baseType, System.Type[] interfaces)
		public System.Type BaseType { get; private set; }
		public System.Type[] Interfaces { get; private set; }

NH-3954 - Dynamic proxy cache may yield a wrong proxy #561

NH-3954 - Dynamic proxy cache may yield a wrong proxy #561

Conversation

fredericDelaporte commented Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Mar 3, 2017

fredericDelaporte commented Mar 3, 2017 • edited Loading

fredericDelaporte commented Mar 4, 2017

fredericDelaporte commented Mar 16, 2017

fredericDelaporte commented Mar 19, 2017

Choose a reason for hiding this comment

hazzik commented Mar 19, 2017 • edited Loading

hazzik left a comment • edited Loading

Choose a reason for hiding this comment

hazzik Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hazzik Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Mar 19, 2017

fredericDelaporte commented Mar 20, 2017

fredericDelaporte commented Mar 20, 2017

hazzik commented Mar 20, 2017 • edited Loading

fredericDelaporte commented Mar 26, 2017 • edited Loading

hazzik left a comment

Choose a reason for hiding this comment

fredericDelaporte commented Mar 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredericDelaporte commented Mar 27, 2017 • edited Loading

hazzik commented Mar 27, 2017

fredericDelaporte commented Mar 27, 2017

fredericDelaporte commented Mar 27, 2017

hazzik commented Mar 27, 2017

fredericDelaporte commented Mar 27, 2017

fredericDelaporte commented Mar 3, 2017 •

edited

Loading

fredericDelaporte commented Mar 3, 2017 •

edited

Loading

hazzik commented Mar 19, 2017 •

edited

Loading

hazzik left a comment •

edited

Loading

hazzik Mar 19, 2017 •

edited

Loading

hazzik Mar 19, 2017 •

edited

Loading

fredericDelaporte Mar 19, 2017 •

edited

Loading

fredericDelaporte Mar 20, 2017 •

edited

Loading

hazzik commented Mar 20, 2017 •

edited

Loading

fredericDelaporte commented Mar 26, 2017 •

edited

Loading

fredericDelaporte commented Mar 27, 2017 •

edited

Loading