Merry x-mas,
This new rule in Correctness checks that disposable locals are always
disposed before the method returns.
Use 'using' statement (or a try/finally block) to guarantee local disposal
even in the event an uncatched exception occurs.
Bad example (non-guaranteed disposal):
void DecodeFile (string file) {
var stream = new StreamReader (file);
DecodeHeader (stream);
if (!DecodedHeader.HasContent)
return;
DecodeContent (stream);
stream.Dispose ();
}
Good example (non-guaranteed disposal):
void DecodeFile (string file) {
using (var stream = new StreamReader (file)) {
DecodeHeader (stream);
if (!DecodedHeader.HasContent)
return;
DecodeContent (stream);
}
}
Bad example (not disposed / not locally disposed):
void DecodeFile (string file) {
var stream = new StreamReader (file);
Decode (stream);
}
void Decode (Stream stream)
{
/*code to decode the stream*/
stream.Dispose ();
}
Good example (not disposed / not locally disposed):
void DecodeFile (string file) {
using (var stream = new StreamReader (file)) {
Decode (stream);
}
}
void Decode (Stream stream)
{
/*code to decode the stream*/
}
Interestingly the rule finds 115 defects in mscorlib, most of them in
Security* where temporarily-created disposable HashAlgorithm objects are not
disposed.
Cheers,
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Gendarme" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/gendarme?hl=en
-~----------~----~----~----~------~----~------~--~---
From 7ec49d3695056f7234755c3015f6d33cfb0473bb Mon Sep 17 00:00:00 2001
From: Cedric Vivier <[email protected]>
Date: Thu, 25 Dec 2008 20:59:40 +0800
Subject: [PATCH] WIP in 'EnsureLocalDisposal' branch.
2008-12-25 Cedric Vivier <[email protected]>
* EnsureLocalDisposalRule.cs: New. Rule that checks if locals
are guaranteed to be disposed before the method returns.
---
.../rules/Gendarme.Rules.Correctness/ChangeLog | 5 +
.../EnsureLocalDisposalRule.cs | 232 +++++++++++++++++++
.../rules/Gendarme.Rules.Correctness/Makefile.am | 2 +
.../Test/EnsureLocalDisposalTest.cs | 235 ++++++++++++++++++++
4 files changed, 474 insertions(+), 0 deletions(-)
create mode 100644 gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs
create mode 100644 gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs
diff --git a/gendarme/rules/Gendarme.Rules.Correctness/ChangeLog b/gendarme/rules/Gendarme.Rules.Correctness/ChangeLog
index f620970..7b6784f 100644
--- a/gendarme/rules/Gendarme.Rules.Correctness/ChangeLog
+++ b/gendarme/rules/Gendarme.Rules.Correctness/ChangeLog
@@ -1,3 +1,8 @@
+2008-12-25 Cedric Vivier <[email protected]>
+
+ * EnsureLocalDisposalRule.cs: New. Rule that checks if locals
+ are guaranteed to be disposed before the method returns.
+
2008-12-23 Cedric Vivier <[email protected]>
* ProvideCorrectRegexPatternRule.cs: New. Rule that checks if
diff --git a/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs
new file mode 100644
index 0000000..99b5806
--- /dev/null
+++ b/gendarme/rules/Gendarme.Rules.Correctness/EnsureLocalDisposalRule.cs
@@ -0,0 +1,232 @@
+//
+// Gendarme.Rules.Correctness.EnsureLocalDisposalRule class
+//
+// Authors:
+// Cedric Vivier <[email protected]>
+//
+// Copyright (C) 2008 Cedric Vivier
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+using System;
+using System.Collections.Generic;
+
+using Mono.Cecil;
+using Mono.Cecil.Cil;
+
+using Gendarme.Framework;
+using Gendarme.Framework.Rocks;
+using Gendarme.Framework.Engines;
+using Gendarme.Framework.Helpers;
+
+
+namespace Gendarme.Rules.Correctness {
+
+ /// <summary>
+ /// This rule checks that disposable locals are always disposed before the method returns.
+ /// Use 'using' statement (or a try/finally block) to guarantee local disposal even
+ /// in the event an uncatched exception occurs.
+ /// </summary>
+ /// <example>
+ /// Bad example (non-guaranteed disposal):
+ /// <code>
+ /// void DecodeFile (string file) {
+ /// var stream = new StreamReader (file);
+ /// DecodeHeader (stream);
+ /// if (!DecodedHeader.HasContent)
+ /// return;
+ /// DecodeContent (stream);
+ /// stream.Dispose ();
+ /// }
+ /// </code>
+ /// </example>
+ /// <example>
+ /// Good example (non-guaranteed disposal):
+ /// <code>
+ /// void DecodeFile (string file) {
+ /// using (var stream = new StreamReader (file)) {
+ /// DecodeHeader (stream);
+ /// if (!DecodedHeader.HasContent)
+ /// return;
+ /// DecodeContent (stream);
+ /// }
+ /// }
+ /// </code>
+ /// </example>
+ /// <example>
+ /// Bad example (not disposed / not locally disposed):
+ /// <code>
+ /// void DecodeFile (string file) {
+ /// var stream = new StreamReader (file);
+ /// Decode (stream);
+ /// }
+ ///
+ /// void Decode (Stream stream)
+ /// {
+ /// /*code to decode the stream*/
+ /// stream.Dispose ();
+ /// }
+ /// </code>
+ /// </example>
+ /// <example>
+ /// Good example (not disposed / not locally disposed):
+ /// <code>
+ /// void DecodeFile (string file) {
+ /// using (var stream = new StreamReader (file)) {
+ /// Decode (stream);
+ /// }
+ /// }
+ ///
+ /// void Decode (Stream stream)
+ /// {
+ /// /*code to decode the stream*/
+ /// }
+ /// </code>
+ /// </example>
+
+ [Problem ("This disposable local is not guaranteed to be disposed before the method returns.")]
+ [Solution ("Use 'using' statement or surround the local's usage with a try/finally block.")]
+ [EngineDependency (typeof (OpCodeEngine))]
+ public class EnsureLocalDisposalRule : Rule, IMethodRule {
+
+ static OpCodeBitmask callsAndNewobjBitmask = BuildCallsAndNewobjOpCodeBitmask ();
+ static HashSet<Instruction> suspectLocals = new HashSet<Instruction> ();
+
+ static bool IsDispose (MethodReference call)
+ {
+ if (!call.HasThis)
+ return false;
+ return MethodSignatures.Dispose.Matches (call) || MethodSignatures.DisposeExplicit.Matches (call);
+ }
+
+ static bool DoesReturnDisposable (MethodReference call)
+ {
+ //ignore properties (likely not the place where the IDisposable is *created*)
+ if (call.IsProperty ())
+ return false;
+ if (call.Name == MethodDefinition.Ctor || call.Name == MethodDefinition.Cctor)
+ return call.DeclaringType.Implements ("System.IDisposable");
+
+ return call.ReturnType.ReturnType.Implements ("System.IDisposable");
+ }
+
+ static bool AreBothInstructionsInSameTryFinallyBlock (MethodBody body, Instruction a, Instruction b)
+ {
+ foreach (ExceptionHandler eh in body.ExceptionHandlers) {
+ if (eh.Type != ExceptionHandlerType.Finally)
+ continue;
+ if (eh.TryStart.Offset <= a.Next.Offset && eh.TryEnd.Offset >= a.Offset
+ && eh.HandlerStart.Offset <= b.Offset && eh.HandlerEnd.Offset >= b.Offset)
+ return true;
+ }
+ return false;
+ }
+
+ static Instruction FindRelatedSuspectLocal (MethodDefinition method, Instruction ins)
+ {
+ ins = ins.TraceBack (method);
+ if (null == ins || !ins.IsLoadLocal ())
+ return null;
+
+ int index = ins.GetVariable (method).Index;
+ foreach (var local in suspectLocals) {
+ if (local.GetVariable (method).Index == index)
+ return local;
+ }
+ return null;
+ }
+
+ public RuleResult CheckMethod (MethodDefinition method)
+ {
+ if (!method.HasBody || method.IsGeneratedCode ())
+ return RuleResult.DoesNotApply;
+
+ //we ignore methods/constructors that returns IDisposable themselves
+ //where local(s) are most likely used for disposable object construction
+ if (DoesReturnDisposable (method))
+ return RuleResult.DoesNotApply;
+
+ //is there any potential IDisposable-getting opcode in the method?
+ OpCodeBitmask methodBitmask = OpCodeEngine.GetBitmask (method);
+ if (!callsAndNewobjBitmask.Intersect (methodBitmask))
+ return RuleResult.DoesNotApply;
+
+ //do we potentially store that IDisposable in a local?
+ if (!OpCodeBitmask.StoreLocal.Intersect (methodBitmask))
+ return RuleResult.DoesNotApply;
+
+ suspectLocals.Clear ();
+
+ foreach (Instruction ins in method.Body.Instructions) {
+ if (!callsAndNewobjBitmask.Get (ins.OpCode.Code))
+ continue;
+
+ MethodReference call = (MethodReference) ins.Operand;
+
+ if (IsDispose (call)) {
+ Instruction local = FindRelatedSuspectLocal (method, ins);
+ if (local != null) {
+ if (AreBothInstructionsInSameTryFinallyBlock (method.Body, local, ins)) {
+ suspectLocals.Remove (local);
+ } else {
+ string msg = string.Format ("Local {0}is not guaranteed to be disposed.", GetFriendlyNameOrEmpty (local.GetVariable (method)));
+ Runner.Report (method, local, Severity.Medium, Confidence.Normal, msg);
+ suspectLocals.Remove (local);
+ }
+ }
+ continue;
+ }
+
+ if (ins.Next == null || !ins.Next.IsStoreLocal ())
+ continue; //even if an IDisposable, it isn't stored in a local
+
+ if (!DoesReturnDisposable (call))
+ continue;
+
+ suspectLocals.Add (ins.Next);
+ }
+
+ foreach (var local in suspectLocals) {
+ string msg = string.Format ("Local {0}is not disposed (at least not locally).", GetFriendlyNameOrEmpty (local.GetVariable (method)));
+ Runner.Report (method, local, Severity.High, Confidence.Normal, msg);
+ }
+
+ return Runner.CurrentRuleResult;
+ }
+
+ static string GetFriendlyNameOrEmpty (VariableReference variable)
+ {
+ if (null == variable.Name || variable.Name.StartsWith ("V_"))
+ return string.Empty;
+ return string.Format ("'{0}' ", variable.Name);
+ }
+
+ static OpCodeBitmask BuildCallsAndNewobjOpCodeBitmask ()
+ {
+ #if true
+ return new OpCodeBitmask (0x8000000000, 0x4400000000000, 0x0, 0x0);
+ #else
+ OpCodeBitmask mask = new OpCodeBitmask ();
+ mask.UnionWith (OpCodeBitmask.Calls);
+ mask.Set (Code.Newobj);
+ return mask;
+ #endif
+ }
+ }
+}
diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Makefile.am b/gendarme/rules/Gendarme.Rules.Correctness/Makefile.am
index fae489d..e593edb 100644
--- a/gendarme/rules/Gendarme.Rules.Correctness/Makefile.am
+++ b/gendarme/rules/Gendarme.Rules.Correctness/Makefile.am
@@ -12,6 +12,7 @@ rules_sources = \
DisposableFieldsShouldBeDisposedRule.cs \
DoNotRoundIntegersRule.cs \
DontCompareWithNaNRule.cs \
+ EnsureLocalDisposalRule.cs \
FinalizersShouldCallBaseClassFinalizerRule.cs \
FloatComparisonRule.cs \
ReviewInconsistentIdentityRule.cs \
@@ -37,6 +38,7 @@ tests_sources = \
DisposableFieldsShouldBeDisposedTest.cs \
DontCompareWithNaNTest.cs \
DoNotRoundIntegersTest.cs \
+ EnsureLocalDisposalTest.cs \
FinalizersShouldCallBaseClassFinalizerTest.cs \
ReviewInconsistentIdentityTest.cs \
MethodCanBeMadeStaticTest.cs \
diff --git a/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs
new file mode 100644
index 0000000..660853d
--- /dev/null
+++ b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs
@@ -0,0 +1,235 @@
+//
+// Unit tests for EnsureLocalDisposalRule
+//
+// Authors:
+// Cedric Vivier <[email protected]>
+//
+// Copyright (C) 2008 Cedric Vivier
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+using System;
+using System.IO;
+using System.Xml;
+using NUnit.Framework;
+
+using Gendarme.Rules.Correctness;
+
+using Test.Rules.Fixtures;
+using Test.Rules.Helpers;
+using Test.Rules.Definitions;
+
+namespace Test.Rules.Correctness {
+
+ class DisposalCases {
+ string foo;
+ StreamReader sr;
+
+ string DoesNotApply1 () { //no call/newobj/stloc
+ return foo;
+ }
+
+ XmlReader DoesNotApply2 () { //returns IDisposable
+ return null;
+ }
+
+ void DoesNotApply3 () {
+ sr = new StreamReader ("bar.xml"); //field
+ }
+
+ string Success0 () {
+ var o = new object ();
+ return o.ToString ();
+ }
+
+ void Success1 () {
+ using (var reader = XmlReader.Create ("foo.xml")) {
+ Console.WriteLine (reader.ToString ());
+ }
+ }
+
+ void Success2 (string pattern) {
+ using (var reader = XmlReader.Create ("foo.xml")) {
+ Console.WriteLine (reader.ToString ());
+ StreamReader reader2 = null;
+ try {
+ reader2 = new StreamReader ("bar.xml"); //newobj
+ } catch (InvalidOperationException e) {
+ Console.WriteLine (e.Message);
+ } finally {
+ if (reader2 != null)
+ reader2.Dispose ();
+ }
+ }
+ }
+
+ void Success3 (IDisposable disposable) {
+ int x = 0;
+ disposable.Dispose ();
+ }
+
+ void Success4 () {
+ int x = 0;
+ sr = new StreamReader ("bar.xml"); //field
+ }
+
+ void Failure0 () {
+ var reader = XmlReader.Create ("foo.xml");
+ Console.WriteLine (reader.ToString ());
+ }
+
+ void Failure1 () {
+ var reader = XmlReader.Create ("foo.xml");
+ Console.WriteLine (reader.ToString ());
+ ((IDisposable) reader).Dispose (); //not guaranteed
+ }
+
+ void Failure2 () {
+ XmlReader reader = null;
+ try {
+ reader = XmlReader.Create ("foo.xml");
+ Console.WriteLine (reader.ToString ());
+ } catch (InvalidOperationException e) {
+ Console.WriteLine (e.Message);
+ }
+ ((IDisposable) reader).Dispose ();
+ }
+
+ void Failure3 () {
+ using (var reader = XmlReader.Create ("foo.xml")) {
+ Console.WriteLine (reader.ToString ());
+ try {
+ var reader2 = new StreamReader ("bar.xml"); //newobj
+ } finally {
+ ((IDisposable) reader).Dispose (); //reader != reader2 !!!
+ }
+ }
+ }
+
+ void Failure4 () {
+ var reader = XmlReader.Create ("foo.xml");
+ reader.ToString ();
+ Success3 (reader);
+ }
+
+ bool Failure5 () {
+ using (var reader = XmlReader.Create ("foo.xml")) {
+ var reader2 = new StreamReader ("bar.xml"); //newobj
+ var reader3 = new StreamReader ("baz.xml"); //newobj
+ if (reader2.ReadLine () == reader3.ReadLine ())
+ return true;
+ }
+ return false;
+ }
+ }
+
+ [TestFixture]
+ public class EnsureLocalDisposalTest : MethodRuleTestFixture<EnsureLocalDisposalRule> {
+
+ [Test]
+ public void DoesNotApply0 ()
+ {
+ AssertRuleDoesNotApply (SimpleMethods.EmptyMethod);
+ }
+
+ [Test]
+ public void DoesNotApply1 ()
+ {
+ AssertRuleDoesNotApply<DisposalCases> ("DoesNotApply1");
+ }
+
+ [Test]
+ public void DoesNotApply2 ()
+ {
+ AssertRuleDoesNotApply<DisposalCases> ("DoesNotApply2");
+ }
+
+ [Test]
+ public void DoesNotApply3 ()
+ {
+ AssertRuleDoesNotApply<DisposalCases> ("DoesNotApply3");
+ }
+
+ [Test]
+ public void Success0 ()
+ {
+ AssertRuleSuccess<DisposalCases> ("Success0");
+ }
+
+ [Test]
+ public void Success1 ()
+ {
+ AssertRuleSuccess<DisposalCases> ("Success1");
+ }
+
+ [Test]
+ public void Success2 ()
+ {
+ AssertRuleSuccess<DisposalCases> ("Success2");
+ }
+
+ [Test]
+ public void Success3 ()
+ {
+ AssertRuleSuccess<DisposalCases> ("Success3");
+ }
+
+ [Test]
+ public void Success4 ()
+ {
+ AssertRuleSuccess<DisposalCases> ("Success4");
+ }
+
+ [Test]
+ public void Failure0 ()
+ {
+ AssertRuleFailure<DisposalCases> ("Failure0", 1);
+ }
+
+ [Test]
+ public void Failure1 ()
+ {
+ AssertRuleFailure<DisposalCases> ("Failure1", 1);
+ }
+
+ [Test]
+ public void Failure2 ()
+ {
+ AssertRuleFailure<DisposalCases> ("Failure2", 1);
+ }
+
+ [Test]
+ public void Failure3 ()
+ {
+ AssertRuleFailure<DisposalCases> ("Failure3", 2);
+ }
+
+ [Test]
+ public void Failure4 ()
+ {
+ AssertRuleFailure<DisposalCases> ("Failure4", 1);
+ }
+
+ [Test]
+ public void Failure5 ()
+ {
+ AssertRuleFailure<DisposalCases> ("Failure5", 2);
+ }
+ }
+}
--
1.6.0.3
