From a09690d3325e453dc4a7e185bfaa48c4d3b15156 Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Fri, 21 Mar 2014 12:19:30 +0100 Subject: [PATCH 1/8] [Gendarme] Update for Mono 3.2 Use Mono.Cecil.Mdb 0.9.5 that is included in Mono 3.2 --- gendarme/framework/Gendarme.Framework.Rocks/ModuleRocks.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 } From e7ff13671207d1b4bf4b069e615b23372fdf870f Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Fri, 21 Mar 2014 11:43:23 +0100 Subject: [PATCH 2/8] [Gendarme] Unit tests for #18498 --- .../Test/EnsureLocalDisposalTest.cs | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs index e375d1d0c..6e5852757 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs @@ -439,5 +439,74 @@ 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 } } From 42ae0a92db6cb0ef92b3a49d257de26be6b43508 Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Fri, 21 Mar 2014 11:46:17 +0100 Subject: [PATCH 3/8] [Gendarme] Fix EnsureLocalDispsable rule (#18498) We need to keep track of local variables that reference other variables that hold disposable objects. The rule should not get triggered if we return one of these local variables. The Mono 3.2 compiler occasionally introduces such local variables. --- .../EnsureLocalDisposalRule.cs | 58 ++++++++++++++++++- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs index 6d1c73dbe..61e141d77 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) { @@ -168,11 +170,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) @@ -239,9 +253,46 @@ void CheckReassignment (MethodDefinition method, Instruction ins) Runner.Report (method, ins, Severity.High, Confidence.Normal, msg); } else { 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 ()) + return; + + var prevIns = ins.Previous; + if (prevIns == null || !prevIns.IsLoadLocal ()) + return; + + var vStore = ins.GetVariable (method); + if (vStore == null) + return; + + if (locals.Get ((ulong)vStore.Index)) + return; + + var vLoad = prevIns.GetVariable (method); + if (vLoad == null) + return; + + if (localsReferencingOtherLocals == null) + localsReferencingOtherLocals = new Dictionary (); + localsReferencingOtherLocals [vStore.Index] = GetFirstVariableHoldingObject (vLoad.Index); + } + public RuleResult CheckMethod (MethodDefinition method) { if (!method.HasBody) @@ -255,6 +306,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 +319,10 @@ public RuleResult CheckMethod (MethodDefinition method) CheckForOutParameters (method, ins); continue; default: - if (!callsAndNewobjBitmask.Get (code)) + if (!callsAndNewobjBitmask.Get (code)) { + CheckForAssignmentReferencingOtherVariable (method, ins); continue; + } break; } From 0616c8038399f86c78d74515ba70143b303a17be Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Fri, 21 Mar 2014 13:06:36 +0100 Subject: [PATCH 4/8] [Gendarme] Fix EnsureLocalDisposable rule with properties Properties can return IDisposable objects, so we need to check those as well, otherwise we report false positives. --- .../EnsureLocalDisposalRule.cs | 6 +++--- .../Test/EnsureLocalDisposalTest.cs | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs index 61e141d77..b78e2ea8c 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs @@ -131,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) { @@ -347,8 +346,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 6e5852757..1c86b70e6 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs @@ -508,5 +508,21 @@ 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"); + } + } } From 35e005acdeac5b513be63e0221a889baa1f5d9c4 Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Mon, 24 Mar 2014 16:49:33 +0100 Subject: [PATCH 5/8] [Gendarme] Add unit test for storing obj in variable We shouldn't report an error if we temporarily store an object in a (new) variable and then return that. The Mono 3.2 compiler produces such code in debug mode. --- .../Test/EnsureLocalDisposalTest.cs | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs index 1c86b70e6..aceaeb767 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs @@ -524,5 +524,48 @@ 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"); + } + } } From a550cd64df83c5d6b5ff1ab2fb1feaea375bdfab Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Mon, 24 Mar 2014 16:50:38 +0100 Subject: [PATCH 6/8] [Gendarme] Fix problem with returned obj stored in variable We shouldn't report an error if we temporarily store an object in a (new) local variable and then return that. The Mono 3.2 compiler produces such code in debug mode. --- .../EnsureLocalDisposalRule.cs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs index b78e2ea8c..3351286dc 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs @@ -241,6 +241,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); @@ -251,6 +263,9 @@ 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); @@ -290,6 +305,8 @@ void CheckForAssignmentReferencingOtherVariable (MethodDefinition method, Instru 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) From 3df56fe4f997f8ba20a69a87f77d6e27d59c983f Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Tue, 1 Apr 2014 16:57:39 +0200 Subject: [PATCH 7/8] [Gendarme] Improve EnsureLocalDisposable rule Don't complain that dispose is not guaranteed if we return an object of that type. This catches the case where we dispose an object in a catch block that we otherwise return. --- .../EnsureLocalDisposalRule.cs | 11 ++++-- .../Test/EnsureLocalDisposalTest.cs | 37 +++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs index 3351286dc..4fb375bbd 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs @@ -217,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); } diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs index aceaeb767..25119cca7 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs @@ -567,5 +567,42 @@ 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"); + } } } From 8e84b64d33160f61c110df8a80792bcd65a3eed6 Mon Sep 17 00:00:00 2001 From: Eberhard Beilharz Date: Tue, 1 Apr 2014 17:05:43 +0200 Subject: [PATCH 8/8] [Gendarme] Deal with field initializers When a field gets assigned a disposable object created with initializers we shouldn't report this as failure. --- .../InstructionRocks.cs | 14 +++++++ .../EnsureLocalDisposalRule.cs | 15 ++++--- .../Test/EnsureLocalDisposalTest.cs | 42 +++++++++++++++++++ 3 files changed, 66 insertions(+), 5 deletions(-) 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/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs index 4fb375bbd..f4b484a17 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs @@ -287,13 +287,22 @@ int GetFirstVariableHoldingObject (int index) void CheckForAssignmentReferencingOtherVariable (MethodDefinition method, Instruction ins) { - if (!ins.IsStoreLocal ()) + 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; @@ -301,10 +310,6 @@ void CheckForAssignmentReferencingOtherVariable (MethodDefinition method, Instru if (locals.Get ((ulong)vStore.Index)) return; - var vLoad = prevIns.GetVariable (method); - if (vLoad == null) - return; - if (localsReferencingOtherLocals == null) localsReferencingOtherLocals = new Dictionary (); localsReferencingOtherLocals [vStore.Index] = GetFirstVariableHoldingObject (vLoad.Index); diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs index 25119cca7..aaf5e8099 100644 --- a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs +++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs @@ -604,5 +604,47 @@ 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"); + } } }