Whilst browsing the .NET 3.5 framework source code yesterday, I came across an odd peculiarity with the XmlDocument class. The XmlDocument.Load(stream) method does not appear to be validating the input stream against a null value. I couldn’t believe it was a real bug until I had reproduced it with the following one-liner:

(new XmlDocument()).Load((Stream)null);

Which results in the following exception:

System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Xml.XmlReader.CalcBufferSize(Stream input)
   at System.Xml.XmlTextReaderImpl.InitStreamInput(Uri baseUri, String baseUriStr, Stream stream, Byte[] bytes, Int32 byteCount, Encoding encoding)
   at System.Xml.XmlTextReaderImpl.InitStreamInput(Stream stream, Encoding encoding)
   at System.Xml.XmlTextReaderImpl..ctor(String url, Stream input, XmlNameTable nt)
   at System.Xml.XmlTextReader..ctor(Stream input, XmlNameTable nt)
   at System.Xml.XmlDocument.Load(Stream inStream)
   at Scratch.XmlDocumentNullCheckTest.CallLoadWithNullStream()

You can see why I was unsure of myself — the rogue null stream drills in through five levels of functions before reaching the innocent-enough CalcBufferSize() method, which asks if the null stream if it CanSeek.

What XmlDocument.Load(Stream) should really do is to trap my null input right at the front door, and throw the specialised exception which exists for this exact scenario, System.ArgumentNullException, instead of exposing its implementation internals to my rogue input value.

Another Culprit: XmlTextReader

The first thing XmlDocument.Load(Stream) does is to delegate the task to the XmlTextReader class. Since XmlTextReader is a public class, and its (Stream, XmlNameTable) constructor is also public, then XmlTextReader is also in violation. (Perhaps the author of XmlDocument.Load() felt it acceptable to not validate for a null stream because they were relying on the XmlTextReader constructor doing that check for them?)

[ These problems exist in System.Xml 2.0.0.0 (2.0.50727.4918) but have probably been fixed in the version that ships as part of .NET 4.0. ]

Static Analysis: CA1062

The other reason I refused to believe this could be true until I had reproduced it is because I know that there is a static code analysis rule built in for this exact scenario: CA1062: Validate arguments of public methods. This rule should trigger for both XmlDocument.Load(Stream) and XmlTextReader..ctor(Stream, XmlNameTable) to remind the developer to validate any reference arguments passed in to externally visible methods.

Not in VS2008

I tried to validate the behaviour of this rule when I was reminded that this static analysis rule (and a few others) were actually removed in Visual Studio 2008! One of the buggy and ill-performing static analysis engines was cut loose for VS2008 (along with the rules that depended upon it) as part of a longer-term strategic move to write a new data flow analysis engine based on MSR’s “Phoenix”.

Back for VS2010

The rules that were removed for 2008 were reinstated in the Visual Studio 2010 September CTP. When they talk about “8 New Data Flow rules” in the September CTP, I guess they really mean “8 old rules from VS2005, that were removed in VS 2008, are now back” — I confirmed this by correlating the new analysis rules listed in the VS2010 September CTP Walkthroughs document with the list of rules removed in VS 2008 due to removal of the data flow engine.

I was hoping that some new rules would also pop up in the Beta, but there’s been no mention of this on the Code Analysis Team Blog so maybe they only managed to reinstate the old rules that were removed for now. This is understandable, considering they had to re-write the data flow analysis engine.

Update (12 July 2009): Reported on Microsoft Connect.

Update (16 July 2009): This can’t be fixed because it might break backward compatibility for applications that are already trapping the NullReferenceException. Ah, one of the joys of strict framework versioning: Unfixable bugs!

If you define your own .NET Exception type (derived from System.Exception) it is important to remember that this custom exception type will not be serializable by default.

It is often necessary for Exceptions be serializable because without this they will not travel across remoting boundaries, or even between application domains within the same process.

Rule 1: Custom exception types must be marked with the [Serializable] attribute

It is the [Serializable] attribute that indicates if your type is serializable. Although System.Exception is decorated with the [Serializable] attribute, this attribute will not be inherited by your custom exception class. The reason for this is clear when we open up Reflector and look at the declaration of SerializableAttribute:

[ComVisible(true)]
[AttributeUsage(
    AttributeTargets.Delegate | AttributeTargets.Enum | 
    AttributeTargets.Struct | AttributeTargets.Class, 
    Inherited=false)] // Not inherited!
public sealed class SerializableAttribute : Attribute
{ ... } 

This means that any custom exception types you create must be explicitly decorated with the [Serializable] attribute.

Rule 2: Custom exception types must implement ISerializable

The default serialization behaviour (which you get for free when you just decorate your class with [Serializable]) is to serialize all public and private fields in the type. This is good enough for many cases, but System.Exception needs to do some special things when being serialized and de-serialized. (For example, populate the lazily instantiated stack trace, which may not have been generated if the default serializer tries to just reach in and grab the field values through reflection.)

Because System.Exception implements the ISerializable interface, by inheritance, your derived exception class also necessarily implements this interface. This means you cannot rely on any default serialization behaviour.

You have to play along with the full ISerializable pattern. This involves two things:

1. Override the GetObjectData() method, which gives your object the opportunity to save its state into a SerializationInfo object (and then call through to the base class to let it do the same!)
2. Override the de-serialization constructor, which gives your object the opportunity to initialize itself by retrieving its state from a SerializationInfo object (and then call through to the base class to let it do the same!)

Other caveats and notes

• Your de-serialization constructor should be protected for unsealed classes, or private for sealed classes. (The Serializer uses reflection to invoke your de-serialization constructor, so it can manage even if the constructor is not publically accessible.)
• Remember to specify the appropriate security demands for your de-serialization constructor, and GetObjectData() method. This will ensure that code running with lower privileges will not be able to, for example, invoke GetObjectData() to covertly extract internal state from your class.

Example

Here is an example custom exception which defines additional properties and serializes them correctly.

[Serializable] 
// Attribute is NOT inherited from Exception and MUST be specified. 
public class SerializableException : Exception 
{ 
    private readonly string resourceName; 
    private readonly IList<string> validationErrors; 
    
    public SerializableException() 
    { 
    } 
    
    public SerializableException(string message) 
    : base(message) 
    { 
    } 
    
    public SerializableException(string message, Exception inner) 
    : base(message, inner) 
    { 
    } 
    
    public SerializableException(string message, string resourceName, IList<string> validationErrors) 
    : base(message) 
    { 
        this.resourceName = resourceName; 
        this.validationErrors = validationErrors; 
    } 
    
    public SerializableException(string message, string resourceName, IList<string> validationErrors, Exception inner) 
    : base(message, inner) 
    { 
        this.resourceName = resourceName; 
        this.validationErrors = validationErrors; 
    } 
    
    [SecurityPermissionAttribute(SecurityAction.Demand, SerializationFormatter = true)] 
    // Protected for unsealed classes, private for sealed. 
    protected SerializableException(SerializationInfo info, StreamingContext context) 
    : base(info, context) 
    { 
        this.resourceName = info.GetString("ResourceName"); 
        this.validationErrors = (IList<string>)info.GetValue("ValidationErrors", typeof(IList<string>)); 
    } 
    
    public string ResourceName 
    { 
        get { return this.resourceName; } 
    } 
    
    public IList<string> ValidationErrors 
    { 
        get { return this.validationErrors; } 
    } 
    
    [SecurityPermissionAttribute(SecurityAction.Demand, SerializationFormatter = true)] 
    public override void GetObjectData(SerializationInfo info, StreamingContext context) 
    { 
        if (info == null) 
        { 
            throw new ArgumentNullException("info"); 
        } 
        
        info.AddValue("ResourceName", this.ResourceName); 
        
        // Note: if "List<T>" isn't serializable you may need to work out another 
        // method of adding your list, this is just for show... 
        info.AddValue("ValidationErrors", this.ValidationErrors, typeof(IList<string>)); 
        
        // MUST call through to the base class to let it save its own state 
        base.GetObjectData(info, context); 
    } 
} 

You can also download a VS2008 project with a more complete set of examples, including unit tests.

(This post was based on my Stack Overflow question, “What is the correct way to make a custom .NET Exception serializable?”)