Sunday, January 18, 2009

Identify the Subtle Bug.....

A friend of mine pointed me out to this.

This code has a subtle bug. What is it?
Hint: it has nothing to do with encryption.


using(RijndaelManaged enc=new RijndaelManaged(){Key=key,IV=iv,Mode=CipherMode.CBC })
{
//DO SOME WORK WITH enc
}


So to outline what and why this doesn’t do what is expected let’s review what this code is
shorthand for.

First:The using block is actually shorthand for a particular try … finally pattern.Roughly this code:

using (SomeDisposableType item = new SomeDisposableType()){}


Is equivalent to:


SomeDisposableType item = null;
try
{
item = new SomeDisposableType();
//DO WORK
}
finally
{
If (item != null) item.Dispose();
}


Depending on how IDisposable is implemented, there could be an implicit cast to the interface involved as well so you’d see ((IDisposable)item).Dispose(); in the finally block instead. Meaningless to the current concept however.

The new C# feature of Object Initializers are another form of syntatic sugar for really this:

SomeTypeWithSetters item = new SomeTypeWithSetters();
Item.Prop1 = “SomeValue”;


By writing it this way:

SomeTypeWithSetters item = new SomeTypeWithSetters(){Prop1=“SomeValue”}


So when you put them together (as in the original example) you would expect the code would be equivalent to this:


RijndaelManaged enc = null;
try
{
RijndaelManaged enc = new RijndaelManaged();
enc.Key = key;
enc.IV = iv;
enc.Mode = CipherMode.CBC;
//DO WORK WITH enc
}
finally
{
if (enc != null) enc.Dispose();
}


This is NOT the case however (and hence the bug)!Due to the nested statement rules in the C# spec, instead the compiler evaluates the code as a nested initializer block followed by a completely separate using block and not a unioned language construct!


RijndaelManaged enc = new RijndaelManaged();
enc.Key = key;
enc.IV = iv;
enc.Mode = CipherMode.CBC;
try

{
//DO WORK WITH enc
}
finally
{
if (enc != null) enc.Dispose();
}


Note: technically the compiler will actually emit two variables pointing to the same object. For clarity I’ve skipped that as it’s frankly not important to the example.

So if there’s something that goes hinky in the initializer block, the Dispose() method is NEVER called by your using block as the code has yet to enter it. All sorts of general badness may then follow. It might be as simple as inefficient use of critical or expensive resources to something as bad as a leak. In general, all sorts of badness, in varying degrees of said badness, may happen to your application.

After that: Hilarity Ensues followed by an immediate Epic Fail.


While I agree that this should be handled by a C# language specification change regarding how the using construct works with nested statements, this is currently how it works today. While not a bug per say in the compiler, it should be considered a hole in the spec itself. Maybe we’ll see this as a change in C# 4.0?


Original Credits : Jimmy Zimmerman
Source: http://ayende.com/Blog/archive/2009/01/15/avoid-object-initializers-amp-the-using-statement.aspx


No comments: