On Thu, 2008-12-25 at 22:50 +0800, Cedric Vivier wrote:
> 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.
HashAlgorithm-devired types are, for MS, unmanaged wrappers.
> Cheers,
>
>
> >
>
>
>
>
>
>
> text/x-diff attachment (0001-WIP-in-EnsureLocalDisposal-branch.patch)
>
> 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;
the xml2wiki converter always needs { } in order pretty-print the source
code
> + /// 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;
same
> + /// 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> ();
We don't have guidelines* for this but I think read/write fields should
not be static. That way we'll, eventually, be able to create multiple
instances of a rule to be executed simultaneously (but there are a lot
of other changes required before doing so).
* JB once told me he would write some guidelines about this ;-)
However comments from everyone is welcome on the subject - it's
not a short term goal but it would be nice to have some
guidelines
> +
> + 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;
IsGeneratedCode has badly evolved - not just in Gendarme but with its
usage in the framework and compilers (e.g. it's not impossible to know
if a property name was generated by a tool, or is a C#3 automatic
property). I think its use should be documented in the rules, i.e. some
comments why it's better used than not (that will help for the review we
need to do on its current uses).
> +
> + //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)));
Is it really helpful not to display V_* ? maybe adding the type of the
variable would be useful (always or when the name is not available).
E.g. "Local of type 'Disposable' is no disposed ..."
> + 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
> + }
> + }
> +}
...
> +++
> b/gendarme/rules/Gendarme.Rules.Correctness/Test/EnsureLocalDisposalTest.cs
Tests looks great, but can you add one where the same local is
not-disposed twice ?
var x = new Disposable ();
Disposable.Do ();
x = new Disposable ();
Thanks
Sebastien
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
