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!