Throw when deserializing Exceptions by reference, and avoid reference…

… tracking for all Exception-derived types (#8628) (#8698)

* Throw ReferenceFieldNotSupportedException on references for serialized exceptions

* Move tests to a more appropriate place

* Prevent reference tracking for generated exception codecs and exception surrogates

* Remove accidentally introduced class

---------

Co-authored-by: David Obee <git@davidobee.com>

src/Orleans.CodeGenerator/Model/SerializableTypeDescription.cs

@@ -195,7 +195,7 @@ public bool IsEmptyConstructable
 
         public bool UseActivator => Type.HasAttribute(_libraryTypes.UseActivatorAttribute) || !IsEmptyConstructable || HasActivatorConstructor;
 
-        public bool TrackReferences => !IsValueType && !Type.HasAttribute(_libraryTypes.SuppressReferenceTrackingAttribute);
+        public bool TrackReferences => !IsValueType && !IsExceptionType && !Type.HasAttribute(_libraryTypes.SuppressReferenceTrackingAttribute);
         public bool OmitDefaultMemberValues => Type.HasAttribute(_libraryTypes.OmitDefaultMemberValuesAttribute);
 
         public List<INamedTypeSymbol> SerializationHooks { get; }

src/Orleans.Serialization/Exceptions.cs

@@ -297,6 +297,48 @@ public override void GetObjectData(SerializationInfo info, StreamingContext cont
         }
     }
 
+    /// <summary>
+    /// A reference to a value is not supported here.
+    /// </summary>
+    [Serializable]
+    [GenerateSerializer]
+    public sealed class ReferenceFieldNotSupportedException : SerializerException
+    {
+        /// <summary>
+        /// Gets the type of the target reference.
+        /// </summary>
+        /// <value>The type of the target reference.</value>
+        [Id(0)]
+        public Type TargetReferenceType { get; }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ReferenceFieldNotSupportedException"/> class.
+        /// </summary>
+        /// <param name="targetType">Type of the target.</param>
+        public ReferenceFieldNotSupportedException(Type targetType) : base(
+            $"Reference with type {targetType} not allowed here.")
+        {
+            TargetReferenceType = targetType;
+        }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="ReferenceFieldNotSupportedException"/> class.
+        /// </summary>
+        /// <param name="info">The <see cref="T:System.Runtime.Serialization.SerializationInfo" /> that holds the serialized object data about the exception being thrown.</param>
+        /// <param name="context">The <see cref="T:System.Runtime.Serialization.StreamingContext" /> that contains contextual information about the source or destination.</param>
+        private ReferenceFieldNotSupportedException(SerializationInfo info, StreamingContext context) : base(info, context)
+        {
+            TargetReferenceType = (Type)info.GetValue(nameof(TargetReferenceType), typeof(Type));
+        }
+
+        /// <inheritdoc/>
+        public override void GetObjectData(SerializationInfo info, StreamingContext context)
+        {
+            base.GetObjectData(info, context);
+            info.AddValue(nameof(TargetReferenceType), TargetReferenceType);
+        }
+    }
+
     /// <summary>
     /// A well-known type was not known.
     /// </summary>

src/Orleans.Serialization/ISerializableSerializer/ExceptionCodec.cs

@@ -222,7 +222,7 @@ public void SetBaseProperties(Exception value, string message, string stackTrace
 
             if (value.GetType() == typeof(Exception))
             {
-                // Exceptions are never written as references. This ensures that reference cycles in exceptions are not possible and is a security precaution. 
+                // Exceptions are never written as references. This ensures that reference cycles in exceptions are not possible and is a security precaution.
                 ReferenceCodec.MarkValueField(writer.Session);
                 writer.WriteStartObject(fieldIdDelta, expectedType, typeof(ExceptionCodec));
                 SerializeException(ref writer, value);
@@ -287,10 +287,15 @@ public Exception ReadValue<TInput>(ref Reader<TInput> reader, Field field)
             // In order to handle null values.
             if (field.WireType == WireType.Reference)
             {
-                // Discard the result of consuming the reference and always return null.
+                var referencedException = ReferenceCodec.ReadReference<Exception, TInput>(ref reader, field);
+
                 // We do not allow exceptions to participate in reference cycles because cycles involving InnerException are not allowed by .NET
                 // Exceptions must never form cyclic graphs via their well-known properties/fields (eg, InnerException).
-                var _ = ReferenceCodec.ReadReference<Exception, TInput>(ref reader, field);
+                if (referencedException is not null)
+                {
+                    throw new ReferenceFieldNotSupportedException(field.FieldType);
+                }
+
                 return null;
             }
 
@@ -313,7 +318,7 @@ object IFieldCodec.ReadValue<TInput>(ref Reader<TInput> reader, Field field)
 
             return DeserializeException(ref reader, field);
         }
-                                
+
         public Exception DeserializeException<TInput>(ref Reader<TInput> reader, Field field)
         {
             field.EnsureWireTypeTagDelimited();

src/Orleans.Serialization/Serializers/SurrogateCodec.cs

@@ -71,11 +71,22 @@ public TField ReadValue<TInput>(ref Reader<TInput> reader, Field field)
         if (field.FieldType is null || field.FieldType == _fieldType)
         {
             field.EnsureWireTypeTagDelimited();
-            var placeholderReferenceId = ReferenceCodec.CreateRecordPlaceholder(reader.Session);
+
+            uint placeholderReferenceId = default;
+            if (IsReferenceTrackingSupported)
+            {
+                placeholderReferenceId = ReferenceCodec.CreateRecordPlaceholder(reader.Session);
+            }
+
             TSurrogate surrogate = default;
             _surrogateSerializer.Deserialize(ref reader, ref surrogate);
             var result = _converter.ConvertFromSurrogate(in surrogate);
-            ReferenceCodec.RecordObject(reader.Session, result, placeholderReferenceId);
+
+            if (IsReferenceTrackingSupported)
+            {
+                ReferenceCodec.RecordObject(reader.Session, result, placeholderReferenceId);
+            }
+
             return result;
         }
 
@@ -85,7 +96,7 @@ public TField ReadValue<TInput>(ref Reader<TInput> reader, Field field)
     /// <inheritdoc/>
     public void WriteField<TBufferWriter>(ref Writer<TBufferWriter> writer, uint fieldIdDelta, Type expectedType, TField value) where TBufferWriter : IBufferWriter<byte>
     {
-        if (ReferenceCodec.TryWriteReferenceField(ref writer, fieldIdDelta, expectedType, value))
+        if (IsReferenceTrackingSupported && ReferenceCodec.TryWriteReferenceField(ref writer, fieldIdDelta, expectedType, value))
         {
             return;
         }
@@ -103,6 +114,8 @@ public TField ReadValue<TInput>(ref Reader<TInput> reader, Field field)
         }
     }
 
+    private bool IsReferenceTrackingSupported => typeof(TField) != typeof(Exception) && !typeof(TField).IsSubclassOf(typeof(Exception));
+
     /// <inheritdoc/>
     public void Serialize<TBufferWriter>(ref Writer<TBufferWriter> writer, TField value) where TBufferWriter : IBufferWriter<byte>
     {

test/Orleans.Serialization.UnitTests/GeneratedSerializerTests.cs

@@ -6,6 +6,7 @@
 using Microsoft.Extensions.DependencyInjection;
 using Newtonsoft.Json;
 using System;
+using System.Buffers;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.IO.Pipelines;
@@ -445,6 +446,52 @@ public void CopyImmutableAndSealedTypes()
         }
     }
 
+    [Fact]
+    public void DuplicateReferencesSerializeTargetJustOnce()
+    {
+        var sharedObject = new MyValue(1);
+        var original = new object[] { sharedObject, sharedObject };
+
+        var result = RoundTripThroughCodec(original);
+
+        Assert.Equal(original, result);
+        Assert.Same(result[0], result[1]);
+    }
+
+    [Fact]
+    public void DuplicateReferencesSerializeTargetMultipleTimesWhenSuppressReferenceTrackingEnabled()
+    {
+        var sharedObject = new MySuppressReferenceTrackingValue(1);
+        var original = new object[] { sharedObject, sharedObject };
+
+        var result = RoundTripThroughCodec(original);
+
+        Assert.Equal(original, result);
+        Assert.NotSame(result[0], result[1]);
+    }
+
+    [Fact]
+    public void DuplicateReferencesToAnyExceptionTypesSerializeTargetMultipleTimes()
+    {
+        var sharedException = new MyCustomException("Something bad");
+        var original = new Exception[] { sharedException, sharedException };
+
+        var result = RoundTripThroughCodec(original);
+
+        Assert.NotSame(result[0], result[1]);
+    }
+
+    [Fact]
+    public void DuplicateReferencesToExceptionTypeWithSurrogateSerializeTargetMultipleTimes()
+    {
+        var sharedException = new MyCustomForeignException(1);
+        var original = new Exception[] { sharedException, sharedException };
+
+        var result = RoundTripThroughCodec(original);
+
+        Assert.NotSame(result[0], result[1]);
+    }
+
     [Fact]
     public void TypeReferencesAreEncodedOnce()
     {

test/Orleans.Serialization.UnitTests/ISerializableTests.cs

@@ -78,7 +78,7 @@ private object SerializationLoop(object original)
                 var output = BitStreamFormatter.Format(ref reader);
                 _log.WriteLine(output);
             }
- 
+
             {
                 using var readerSession = _sessionPool.GetSession();
                 var reader = Reader.Create(readResult.Buffer, readerSession);
@@ -176,7 +176,7 @@ public void ISerializableStructWithCallbacks()
             Assert.Equal(input2.History, input.History);
             Assert.Equal(result2.History, result.History);
         }
-        
+
         private class BaseException : Exception
         {
             public BaseException() { }

test/Orleans.Serialization.UnitTests/Models.cs

@@ -2,6 +2,7 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Linq.Expressions;
+using System.Runtime.Serialization;
 using System.Text.Json;
 using Newtonsoft.Json;
 using Orleans;
@@ -95,7 +96,7 @@ public override bool Equals(object obj)
     }
 
     public override int GetHashCode() => Value;
-} 
+}
 
 [GenerateSerializer]
 [Immutable]
@@ -150,6 +151,62 @@ public class MyUnsealedImmutableSub : MyMutableBase, IMySub
     public MyValue SubValue { get; set; }
 }
 
+[GenerateSerializer]
+[SuppressReferenceTracking]
+public class MySuppressReferenceTrackingValue : MyValue
+{
+    public MySuppressReferenceTrackingValue(int value) : base(value)
+    {
+    }
+}
+
+[GenerateSerializer]
+public class MyCustomException : Exception
+{
+    public MyCustomException() { }
+    public MyCustomException(string message) : base(message) { }
+    public MyCustomException(string message, Exception inner) : base(message, inner) { }
+    public MyCustomException(SerializationInfo info, StreamingContext context) : base(info, context) { }
+
+    [Id(0)]
+    public int CustomInt;
+}
+
+public class MyCustomForeignException : Exception
+{
+    public MyCustomForeignException(int customInt)
+    {
+        CustomInt = customInt;
+    }
+
+    public MyCustomForeignException(SerializationInfo info, StreamingContext context) : base(info, context) { }
+
+    [Id(0)]
+    public int CustomInt;
+}
+
+[GenerateSerializer]
+public struct MyCustomForeignExceptionSurrogate
+{
+    [Id(0)]
+    public int CustomInt { get; set; }
+
+    public MyCustomForeignExceptionSurrogate(int customInt)
+    {
+        CustomInt = customInt;
+    }
+}
+
+[RegisterConverter]
+public sealed class MyCustomForeignExceptionSurrogateConverter : IConverter<MyCustomForeignException, MyCustomForeignExceptionSurrogate>
+{
+    public MyCustomForeignException ConvertFromSurrogate(in MyCustomForeignExceptionSurrogate surrogate) =>
+        new MyCustomForeignException(surrogate.CustomInt);
+
+    public MyCustomForeignExceptionSurrogate ConvertToSurrogate(in MyCustomForeignException value) =>
+        new MyCustomForeignExceptionSurrogate(value.CustomInt);
+}
+
 [GenerateSerializer]
 public class SomeClassWithSerializers
 {