PlatformNotSupportedException when attempting to deserialize exceptions not deriving directly from System.Exception

[NightOwl888]

I suspect this is related to #21433, but it is unclear what "partial serialization support" means entirely. It seems like all exception types should be serializable/deserializable given the fact they are the most likely to cross application boundaries, but that doesn't seem to be the case.

We have a test to verify exceptions can both serialize and deserialize. When adding support for .NET Standard 2.0, we have put the FEATURE_SERIALIZABLE compilation symbol back in, meaning these tests should now run (was that a mistake?). Here is what the test looks like:

#if FEATURE_SERIALIZABLE
using Lucene.Net.Attributes;
using NUnit.Framework;
using System;
using System.Collections.Generic;
using System.Linq;

namespace Lucene.Net.Support
{
    /*
     * Licensed to the Apache Software Foundation (ASF) under one or more
     * contributor license agreements.  See the NOTICE file distributed with
     * this work for additional information regarding copyright ownership.
     * The ASF licenses this file to You under the Apache License, Version 2.0
     * (the "License"); you may not use this file except in compliance with
     * the License.  You may obtain a copy of the License at
     *
     *     http://www.apache.org/licenses/LICENSE-2.0
     *
     * Unless required by applicable law or agreed to in writing, software
     * distributed under the License is distributed on an "AS IS" BASIS,
     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
     * See the License for the specific language governing permissions and
     * limitations under the License.
     */

    [TestFixture]
    public class TestExceptionSerialization : ExceptionSerializationTestBase
    {
        public static IEnumerable<object> ExceptionTestData
        {
            get
            {
                var exceptionTypes = typeof(Lucene.Net.QueryParsers.Classic.ICharStream).Assembly.GetTypes().Where(t => typeof(Exception).IsAssignableFrom(t)).Cast<object>();

                // If the assembly has no exceptions, just provide Exception so the test will pass
                if (!exceptionTypes.Any())
                {
                    return new Type[] { typeof(Exception) };
                }

                return exceptionTypes;
            }
        }

        [Test, LuceneNetSpecific]
        public void AllExceptionsInLuceneNamespaceCanSerialize([ValueSource("ExceptionTestData")]Type luceneException)
        {
            var instance = TryInstantiate(luceneException);
            Assert.That(TypeCanSerialize(instance), string.Format("Unable to serialize {0}", luceneException.FullName));
        }
    }
}
#endif

#if FEATURE_SERIALIZABLE
using Lucene.Net.Util;
using NUnit.Framework;
using System;
using System.Globalization;
using System.IO;
using System.Reflection;
using System.Runtime.Serialization;
using System.Runtime.Serialization.Formatters.Binary;

namespace Lucene.Net.Support
{
    /*
     * Licensed to the Apache Software Foundation (ASF) under one or more
     * contributor license agreements.  See the NOTICE file distributed with
     * this work for additional information regarding copyright ownership.
     * The ASF licenses this file to You under the Apache License, Version 2.0
     * (the "License"); you may not use this file except in compliance with
     * the License.  You may obtain a copy of the License at
     *
     *     http://www.apache.org/licenses/LICENSE-2.0
     *
     * Unless required by applicable law or agreed to in writing, software
     * distributed under the License is distributed on an "AS IS" BASIS,
     * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
     * See the License for the specific language governing permissions and
     * limitations under the License.
     */

    public abstract class ExceptionSerializationTestBase : LuceneTestCase
    {
        protected static bool TypeCanSerialize<T>(T exception)
        {
            T clone;

            try
            {
                var binaryFormatter = new BinaryFormatter();
                using (var serializationStream = new MemoryStream())
                {
                    binaryFormatter.Serialize(serializationStream, exception);
                    serializationStream.Seek(0, SeekOrigin.Begin);
                    clone = (T)binaryFormatter.Deserialize(serializationStream);
                }
            }
#pragma warning disable 168
            catch (SerializationException ex)
#pragma warning restore 168
            {
                return false;
            }

            return true;
        }

        protected static object TryInstantiate(Type type)
        {
            object instance = null;
            BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance;
            try
            {
                instance = Activator.CreateInstance(type, flags, null, new object[] { "A message" }, CultureInfo.InvariantCulture);
            }
            catch (MissingMethodException)
            {
                try
                {
                    instance = Activator.CreateInstance(type, flags, null, null, CultureInfo.InvariantCulture);
                }
                catch (MissingMethodException)
                {
                    Assert.Fail("Can't instantiate type {0}, it's missing the necessary constructors.", type.FullName);
                }
            }
            return instance;
        }
    }
}
#endif

It passes for all tests that inherit System.Exception directly. However, it fails for exceptions that inherit System.IO.IOException and System.NotSupportedException on the line clone = (T)binaryFormatter.Deserialize(serializationStream);.

Example Exception that Fails to Deserialize

#if FEATURE_SERIALIZABLE
    [Serializable]
#endif
    public class TooManyBasicQueries : System.IO.IOException
    {
        public TooManyBasicQueries(int maxBasicQueries)
            : this("Exceeded maximum of " + maxBasicQueries + " basic queries.")
        { }

        // For testing
        public TooManyBasicQueries(string message)
            : base(message)
        { }

#if FEATURE_SERIALIZABLE
        /// <summary>
        /// Initializes a new instance of this class with serialized data.
        /// </summary>
        /// <param name="info">The <see cref="SerializationInfo"/> that holds the serialized object data about the exception being thrown.</param>
        /// <param name="context">The <see cref="StreamingContext"/> that contains contextual information about the source or destination.</param>
        public TooManyBasicQueries(SerializationInfo info, StreamingContext context)
            : base(info, context)
        {
        }
#endif

Test Name:	AllExceptionsInLuceneNamespaceCanSerialize(Lucene.Net.QueryParsers.Surround.Query.TooManyBasicQueries)
Test FullName:	Lucene.Net.Support.TestExceptionSerialization.AllExceptionsInLuceneNamespaceCanSerialize(Lucene.Net.QueryParsers.Surround.Query.TooManyBasicQueries)
Test Source:	F:\Projects\lucenenet\src\Lucene.Net.Tests.QueryParser\Support\TestExceptionSerialization.cs : line 49
Test Outcome:	Failed
Test Duration:	0:00:00.032

Result StackTrace:	
at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeConstructorInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters)
   at System.Runtime.Serialization.ObjectManager.CompleteISerializableObject(Object obj, SerializationInfo info, StreamingContext context)
   at System.Runtime.Serialization.ObjectManager.FixupSpecialObject(ObjectHolder holder)
   at System.Runtime.Serialization.ObjectManager.DoFixups()
   at System.Runtime.Serialization.Formatters.Binary.ObjectReader.Deserialize(BinaryParser serParser, Boolean fCheck)
   at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Deserialize(Stream serializationStream, Boolean check)
   at Lucene.Net.Support.ExceptionSerializationTestBase.TypeCanSerialize[T](T exception) in F:\Projects\lucenenet\src\Lucene.Net.TestFramework\Support\ExceptionSerializationTestBase.cs:line 42
   at Lucene.Net.Support.TestExceptionSerialization.AllExceptionsInLuceneNamespaceCanSerialize(Type luceneException) in F:\Projects\lucenenet\src\Lucene.Net.Tests.QueryParser\Support\TestExceptionSerialization.cs:line 49
--PlatformNotSupportedException
   at System.SystemException..ctor(SerializationInfo info, StreamingContext context)
   at System.IO.IOException..ctor(SerializationInfo info, StreamingContext context)
   at Lucene.Net.QueryParsers.Surround.Query.TooManyBasicQueries..ctor(SerializationInfo info, StreamingContext context) in F:\Projects\lucenenet\src\Lucene.Net.QueryParser\Surround\Query\TooManyBasicQueries.cs:line 55
Result Message:	
System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
  ----> System.PlatformNotSupportedException : Operation is not supported on this platform.
Result StandardOutput:	
Culture: rof
Time Zone: (UTC+04:30) Kabul
Default Codec: Lucene3x (Lucene.Net.Codecs.Lucene3x.PreFlexRWCodec)
Default Similarity: RandomSimilarityProvider(queryNorm=False,coord=no):

Note that if the above exception is changed to inherit System.Exception instead of System.IO.IOException the test will pass. However, we can't exactly work around this by inheriting System.Exception since we are porting from Java and the application relies heavily on catching the right exception types to go down the correct execution path (at least not without putting in a catch block for every single one of our custom exception types that inherits System.IO.IOException, etc.)

What I am wondering is whether or not this is a bug, whether or not we shouldn't be relying on serialization yet in .NET Standard 2.0, and if this was intentional why such an odd choice to allow serialization on some exceptions but not others was made?

Platform

Microsoft Windows [Version 10.0.15063]
(c) 2017 Microsoft Corporation. All rights reserved.

.NET Command Line Tools (2.0.0)

Product Information:
Version: 2.0.0
Commit SHA-1 hash: cdcd1928c9

Runtime Environment:
OS Name: Windows
OS Version: 10.0.15063
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\2.0.0\

Microsoft .NET Core Shared Framework Host

Version : 2.0.0
Build : e8b8861

[danmoseley]

@NightOwl888 thank you for the detailed report. To be able to serialize/deserialize types with BinaryFormatter they need to be marked as [Serializable] and also any deserialization constructors and ISerializable implementatoins in the hierarchy must work or at least not throw. As you have discovered, in .NET Core 2.0 we support binary serialization on a limited set of types. The complete list is here. The list is limited in this release to make sure we could support everything on the list, and we did not support binary serialization where we did not have to -- it has a large cost in supportability, reliability, maintenance and potentially security. You will see the only exception types on the list are Exception and AggregateException. All other exceptions, we did not mark as [Serializable] so the binary formatter would not de/serialize them. Since they were not expected to serialize, where they had deserialization constructors or ISerializable implementations, we left them present but made them throw PlatformNotSupportedException. In your case, you are deriving from IOException, and your derived type has the [Serializable] attribute. this allows the binary formatter to begin de/serializing them, only to hit the PlatformNotSupportedException in the deseralization constructor of IOException.

If we want to help make this work, our options are

• Make more exception types serializable, which is useful, but also adds the burden mentioned, or
• Leave them unserializable, but stop throwing in them. Serializing derived user types may then work, but will leave the base types in an "undetermined" state -- with their members not initialized. That may be OK in many cases, for example IOException has only one field, which is probably OK to be null. We could even mark IOException and so forth as [Serializable] so they could be serialized directly, albeit only preserving the members on the bas Exception type. @ViktorHofer , @morganbr thoughts on that option?

Meantime if you are running on .NET Core 2.0 and want to serialize types not on the list at the link above, you will unfortunately not be able to use binary serialization for them.

[danmoseley]

also cc @stephentoub for my suggestion above.

[danmoseley]

I don't think we would go this way (support types with uninitialized state) because it leads us directly towards one of the worst issues with binary serialization: bad state.

What I think we would want to do is increase the number of exception types we support serializing. In your case @NightOwl888 do you have a distinct list of such types?

[NightOwl888]

In your case @NightOwl888 do you have a distinct list of such types?

@danmosemsft

It looks like the complete list we would/will need to subclass are:

System.IO.FileNotFoundException
System.IO.IOException
System.ArgumentException
System.Exception
System.FormatException
System.InvalidOperationException
System.NotSupportedException
System.ObjectDisposedException

[ViktorHofer]

I don't think we would go this way (support types with uninitialized state) because it leads us directly towards one of the worst issues with binary serialization: bad state.

I agree, we should definitely avoid putting object in an unitialized state as this could also introduce security issues.

Make more exception types serializable, which is useful, but also adds the burden mentioned

I'm not convinced this is the right approach. If we make more and more exceptions serializable again, we end up at the same spot we were before. I think we should first clarify what @NightOwl888 intends to do with the exceptions "across application boundaries" and maybe give suggestions how to not rely on binary serialization for that.

It's crucial that we state as often as possible that binary serialization is not intended to be used for new applications but to make porting from Desktop to Core easier. There are tons of security concerns that are introduced by binary serialization, one of it is reading from a stream that could be compromised (network transferred date, e.g. in remoting).

@NightOwl888 what's the scenario you are trying to solve with the list of exceptions you posted? Do you need to access the data that is stored in the different exception types or are they just expressing which path of execution needs to taken?

[danmoseley]

Well, those all either have no fields or string fields (leaving aside Exception which we already support) so compared to some of the other types, they are easy. I dug up our thinking where we originally decided on just Exception and AggregateException and the thinking was mainly that we didn't have a clear place to draw the line. Maybe that is still true.

[ViktorHofer]

Marking simple exceptions that don't store additional or complex data as serializable would definitely work BUT for me the serialization story for Exceptions was the clearest of all:

We don't support serialization of any "framework" exception besides the ones that are needed to build custom serializable ones (and store them in an aggregated fashion...).

[NightOwl888]

@NightOwl888 what's the scenario you are trying to solve with the list of exceptions you posted? Do you need to access the data that is stored in the different exception types or are they just expressing which path of execution needs to taken?

We are following your lead on this. Basically, we are doing this to be a "good citizen" to allow exceptions to be serialized for our framework. But if this is no longer considered a best practice, we can leave this feature out in .NET Standard 2.0 as we did in .NET Standard 1.5.

On the other hand, if the plan is to bring this back at some point, we want to ensure that we have only 1 .NET Standard 2.0 build configuration instead of 2 (speaking of high costs). So, we will either leave out the serialization support now, or wait for you to complete it before adding official support for .NET Standard 2.0.

[ViktorHofer]

Binary serialization itself isn't considered as a good practice anymore as the security concerns overweight...

So, we will either leave out the serialization support now, or wait for you to complete it before adding official support for .NET Standard 2.0.

Consider the serialization support in .NET Core 2.0 "finished". We might add a few types if customer demand is strong but more or less we should be settled.

[NightOwl888]

@ViktorHofer, @danmosemsft

Is there a up to date document describing the best practices around binary serialization when it comes to building an API for others to consume? I did a Google search for ".NET binary serialization best practices 2017", but the only thing I can find is this document that is 15 years old.

Again, we have no need for this functionality within our framework. We are only trying to provide a good balance of support for it so the hundreds of thousands of applications that depend on our framework can upgrade without too much trouble. Some classes seem like good candidates for binary serialization support (for example custom collection types and types that are meant for managing a single value or array), but although your above comments make it clear that the line in the sand needs to change, it is still no more clear exactly where we should draw that line.

What I am looking for is a document that describes the best practices to follow, similar to the set of rules you have for properties vs methods. Also, should the rules differ between .NET Framework and .NET Standard? For example, should we just forget about binary serialization in .NET Standard 2.0 and provide it on .NET Framework for legacy purposes? Or could you at least explain what guidelines you used for .NET Core 2.0?

Basically, this goes back to a legacy issue that we had where someone was trying to serialize one of our core types for a distributed cache. To do this now, we will need to make about 15 types serializable. What I am wondering is whether this still considered an acceptable solution, or should we leave out binary serialization and recommend some other approach?

[ViktorHofer]

Is there a up to date document describing the best practices around binary serialization when it comes to building an API for others to consume? I did a Google search for ".NET binary serialization best practices 2017", but the only thing I can find is this document that is 15 years old.

Nothing I'm aware of. I guess there aren't many articles about API design with binary serialization as most people are either using json/xml or a custom binary protocol (e.g. with BinaryReader, BinaryWriter).
cc @mairaw

For example, should we just forget about binary serialization in .NET Standard 2.0 and provide it on .NET Framework for legacy purposes?

As already mentioned, we recommend to step away from binary serialization and use other serialization frameworks like Json.NET which operate on the public API surface. This applies to the entire .NET Stack (Framework, Core, Mono, UWP).

What I am wondering is whether this still considered an acceptable solution, or should we leave out binary serialization and recommend some other approach?

It's definitely still and acceptable and working solution but has several security concerns. We only support the defined set of types because binary serialization limits us in "modernizing" the BCL. Example: A type in .NET Framework has x fields, the same type in .NET Core has x-1 fields because the one field isn't needed anymore. To support serialization of that type we need to bring that one field as a dummy back. It doesn't do anything besides consuming memory but it is needed so that the serialization engine won't complain and throw an exception.
Therefore we limited the set of supported types to "clean up" most of the existing types.

[joperezr]

@ViktorHofer @danmosemsft can this one be closed now?

[ViktorHofer]

We will bring back exception serialization support in 2.1 and maybe also in a servicing release in 2.0. For more details see https://github.com/dotnet/corefx/issues/24424.

@joperezr thanks for spotting