diff --git a/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs b/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs index aef546a2d..770d55508 100644 --- a/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs +++ b/gendarme/framework/Gendarme.Framework.Rocks/InstructionRocks.cs @@ -511,6 +511,20 @@ public static bool IsStoreLocal (this Instruction self) return OpCodeBitmask.StoreLocal.Get (self.OpCode.Code); } + /// + /// Return true if the Instruction is a store of a field (stfld). + /// + /// The Instruction on which the extension method can be called. + /// True if the instruction is a store field, False otherwise + public static bool IsStoreField (this Instruction self) + { + if (self == null) + return false; + + return self.OpCode.Code == Code.Stfld; + } + + /// /// Return the instruction that match the current instruction. This is computed by /// substracting push and adding pop counts until the total becomes zero. diff --git a/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs b/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs index 3aad11713..2527dc7a0 100644 --- a/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs +++ b/gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs @@ -85,7 +85,7 @@ public static void LoadDebuggingSymbols (this ModuleDefinition self) // so we start by looking for it's debugging symbol file if (File.Exists (symbol_name)) { // "always" if we can find Mono.Cecil.Mdb - reader_type = Type.GetType ("Mono.Cecil.Mdb.MdbReaderProvider, Mono.Cecil.Mdb, Version=0.9.4.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756"); + reader_type = Type.GetType ("Mono.Cecil.Mdb.MdbReaderProvider, Mono.Cecil.Mdb, Version=0.9.5.0, Culture=neutral, PublicKeyToken=0738eb9f132ed756"); // load the assembly from the current folder if // it is here, or fallback to the gac } diff --git a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs index 6d1c73dbe..f4b484a17 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs @@ -37,6 +37,7 @@ using Gendarme.Framework.Rocks; using Gendarme.Framework.Engines; using Gendarme.Framework.Helpers; +using System.Diagnostics; namespace Gendarme.Rules.Correctness { @@ -119,6 +120,7 @@ public sealed class EnsureLocalDisposalRule : Rule, IMethodRule { OpCodeBitmask callsAndNewobjBitmask = BuildCallsAndNewobjOpCodeBitmask (); Bitmask locals = new Bitmask (); + Dictionary localsReferencingOtherLocals = null; static bool IsDispose (MethodReference call) { @@ -129,9 +131,8 @@ static bool IsDispose (MethodReference call) static bool DoesReturnDisposable (MethodReference call) { - //ignore properties (likely not the place where the IDisposable is *created*) MethodDefinition method = call.Resolve (); - if ((method == null) || call.IsProperty ()) + if ((method == null) || (call.IsProperty () && !method.IsGetter)) return false; if (method.IsConstructor) { @@ -168,11 +169,23 @@ static bool IsInsideFinallyBlock (MethodDefinition method, Instruction ins) return false; } + void ClearVariable (int index) + { + if (locals.Get ((ulong)index)) { + locals.Clear ((ulong)index); + Debug.Assert (localsReferencingOtherLocals == null || !localsReferencingOtherLocals.ContainsKey (index)); + } else { + int referencedIndex; + if (localsReferencingOtherLocals != null && localsReferencingOtherLocals.TryGetValue (index, out referencedIndex)) + ClearVariable (referencedIndex); + } + } + void Clear (MethodDefinition method, Instruction ins) { VariableDefinition v = ins.GetVariable (method); if (v != null) - locals.Clear ((ulong) v.Index); + ClearVariable (v.Index); } void CheckForReturn (MethodDefinition method, Instruction ins) @@ -204,10 +217,13 @@ void CheckDisposeCalls (MethodDefinition method, Instruction ins) ulong index = v == null ? UInt64.MaxValue : (ulong) v.Index; if (v != null && locals.Get (index)) { if (!IsInsideFinallyBlock (method, ins)) { - string msg = String.Format (CultureInfo.InvariantCulture, - "Local {0}is not guaranteed to be disposed of.", - GetFriendlyNameOrEmpty (v)); - Runner.Report (method, Severity.Medium, Confidence.Normal, msg); + if (!v.VariableType.Equals (method.ReturnType) && + !v.VariableType.Inherits (method.ReturnType.Namespace, method.ReturnType.Name)) { + string msg = String.Format (CultureInfo.InvariantCulture, + "Local {0}is not guaranteed to be disposed of.", + GetFriendlyNameOrEmpty (v)); + Runner.Report (method, Severity.Medium, Confidence.Normal, msg); + } } locals.Clear (index); } @@ -228,6 +244,18 @@ bool CheckCallsToOtherInstances (MethodDefinition method, Instruction ins, Metho return true; } + bool AssignedToLocalVarAndReturn (MethodDefinition method, Instruction ins) + { + var nextIns = ins.Next; + if (nextIns.OpCode.Code != Code.Br && nextIns.OpCode.Code != Code.Br_S) + return false; + + var targetIns = nextIns.Operand as Instruction; + + return (targetIns != null && targetIns.IsLoadLocal () && + targetIns.Next != null && targetIns.Next.OpCode.Code == Code.Ret); + } + void CheckReassignment (MethodDefinition method, Instruction ins) { VariableDefinition v = ins.GetVariable (method); @@ -238,8 +266,55 @@ void CheckReassignment (MethodDefinition method, Instruction ins) GetFriendlyNameOrEmpty (v)); Runner.Report (method, ins, Severity.High, Confidence.Normal, msg); } else { + if (AssignedToLocalVarAndReturn (method, ins)) + return; + locals.Set (index); + if (localsReferencingOtherLocals != null && localsReferencingOtherLocals.ContainsKey ((int)index)) + localsReferencingOtherLocals.Remove ((int)index); + } + } + + int GetFirstVariableHoldingObject (int index) + { + int referencedVar; + if (localsReferencingOtherLocals.TryGetValue (index, out referencedVar)) + return GetFirstVariableHoldingObject (referencedVar); + + Debug.Assert (locals.Get ((ulong)index)); + return index; + } + + void CheckForAssignmentReferencingOtherVariable (MethodDefinition method, Instruction ins) + { + if (!ins.IsStoreLocal () && !ins.IsStoreField ()) + return; + + var prevIns = ins.Previous; + if (prevIns == null || !prevIns.IsLoadLocal ()) + return; + + var vLoad = prevIns.GetVariable (method); + if (vLoad == null) + return; + + if (ins.IsStoreField ()) { + ClearVariable (vLoad.Index); + return; } + + var vStore = ins.GetVariable (method); + if (vStore == null) + return; + + if (locals.Get ((ulong)vStore.Index)) + return; + + if (localsReferencingOtherLocals == null) + localsReferencingOtherLocals = new Dictionary (); + localsReferencingOtherLocals [vStore.Index] = GetFirstVariableHoldingObject (vLoad.Index); + if (localsReferencingOtherLocals [vStore.Index] == vStore.Index) + localsReferencingOtherLocals.Remove (vStore.Index); } public RuleResult CheckMethod (MethodDefinition method) @@ -255,6 +330,7 @@ public RuleResult CheckMethod (MethodDefinition method) bool return_idisposable = DoesReturnDisposable (method); locals.ClearAll (); + localsReferencingOtherLocals = null; foreach (Instruction ins in method.Body.Instructions) { Code code = ins.OpCode.Code; @@ -267,8 +343,10 @@ public RuleResult CheckMethod (MethodDefinition method) CheckForOutParameters (method, ins); continue; default: - if (!callsAndNewobjBitmask.Get (code)) + if (!callsAndNewobjBitmask.Get (code)) { + CheckForAssignmentReferencingOtherVariable (method, ins); continue; + } break; } @@ -293,8 +371,9 @@ public RuleResult CheckMethod (MethodDefinition method) Code nextCode = nextInstruction.OpCode.Code; if (nextCode == Code.Pop || OpCodeBitmask.Calls.Get (nextCode)) { + // We ignore properties (likely not the place where the IDisposable is *created*) // We ignore setter because it is an obvious share of the IDisposable - if (!IsSetter (nextInstruction.Operand as MethodReference)) + if (!call.IsProperty () && !IsSetter (nextInstruction.Operand as MethodReference)) ReportCall (method, ins, call); } else if (nextInstruction.IsStoreLocal ()) { // make sure we're not re-assigning over a non-disposed IDisposable diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs index e375d1d0c..aaf5e8099 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs @@ -439,5 +439,212 @@ public void ReturnValue () #endif AssertRuleSuccess ("ReturnIDisposable2"); } + + #region Bug #18498 + abstract class Image : IDisposable + { + public void Dispose () + { + } + } + + class Bitmap : Image + { + } + + Image ReturnsReference () + { + Bitmap bmp = new Bitmap (); + Image img = (Image)bmp; + return img; + } + + Image AlsoReturnsReference () + { + var bmp = new Bitmap (); + var img = (Image)bmp; + var img2 = img; + return img2; + } + + Image MissesDispose () + { + var bmp = new Bitmap (); + var img = (Image)bmp; + img = new Bitmap (); + return img; + } + + Image AlsoMissesDispose () + { + var bmp = new Bitmap (); + var img = new Bitmap (); + bmp = img; + return bmp; + } + + + [Test] + public void ReturnsReference_Bug18498 () + { + AssertRuleSuccess ("ReturnsReference"); + } + + [Test] + public void ReturnReference2_Bug18498 () + { + AssertRuleSuccess ("AlsoReturnsReference"); + } + + [Test] + public void MissingDispose_Bug18498 () + { + AssertRuleFailure ("MissesDispose"); + } + + [Test] + public void MissingDispose2_Bug18498 () + { + AssertRuleFailure ("AlsoMissesDispose"); + } + #endregion + + Image PropertyReturningObj { + get { + using (var bmp = new Bitmap ()) { + var img = new Bitmap (); + return img; + } + } + } + + [Test] + public void PropertyReturningReference () + { + AssertRuleSuccess ("get_PropertyReturningObj"); + } + + Image ReturnObjectWithIfs () + { + Bitmap obj = null; + int a = new Random ().Next (10); + if (a == 1) + obj = new Bitmap (); + else if (a == 2) + obj = new Bitmap (); + else if (a == 3) + obj = new Bitmap (); + return obj; + } + + [Test] + public void MethodThatConditionallyReturnsObjects () + { + AssertRuleSuccess ("ReturnObjectWithIfs"); + } + + Bitmap ReturnObjectWithSwitch () + { + Bitmap obj = null; + switch (new Random ().Next (10)) + { + case 1: + obj = new Bitmap(); + break; + case 2: + obj = new Bitmap(); + break; + default: + obj = new Bitmap(); + break; + } + return obj; + } + + [Test] + public void MethodThatConditionallyReturnsObjectsThroughSwitch () + { + AssertRuleSuccess ("ReturnObjectWithSwitch"); + } + + Bitmap ReturnObjectAndCallsDisposeInTryCatchBlock () + { + Bitmap obj = null; + try { + obj = new Bitmap (); + } catch (Exception) { + if (obj != null) + obj.Dispose (); + throw; + } + + return obj; + } + + void CallsDisposeInTryCatchBlock () + { + Bitmap obj = null; + try { + obj = new Bitmap (); + } catch (Exception) { + if (obj != null) + obj.Dispose (); + throw; + } + } + + [Test] + public void MethodReturnsObjectAndCallsDisposeInTryCatchBlock () + { + AssertRuleSuccess ("ReturnObjectAndCallsDisposeInTryCatchBlock"); + } + + [Test] + public void MethodCallsDisposeInTryCatchBlock () + { + AssertRuleFailure ("CallsDisposeInTryCatchBlock"); + } + + class TestableClass: IDisposable + { + Stream stream; + + public void AssignFieldFromLocalVariableWithInitializer () + { + var s = new MemoryStream { + ReadTimeout = 10 + }; + + stream = s; + } + + public void AssignFieldWithInitializer () + { + stream = new MemoryStream { + ReadTimeout = 10 + }; + } + + #region IDisposable implementation + + public void Dispose () + { + stream.Dispose (); + } + + #endregion + } + + [Test] + public void FieldAssignedFromLocalVariableWithInitializer () + { + AssertRuleSuccess ("AssignFieldFromLocalVariableWithInitializer"); + } + + [Test] + public void FieldAssignedWithInitializer () + { + AssertRuleSuccess ("AssignFieldWithInitializer"); + } } }