Review request for new rule: DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule
Rolf Bjarne Kvinge
I'm attaching a patch for a new rule:
DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule in
Gendarme.Rules.Interoperability.
Since I didn't read the faq properly, I already committed the rule to
svn, I'm really sorry about this.
The documentation in mono-project.com/Gendarme.Rules.Interoperability
has also been written.
Feedback welcome :)
Rolf
Jesse Jones
I like the idea behind this rule a lot. I wonder though, if it should
be a more general rule. For example, unhandled exceptions in thread
callbacks are nearly as evil as unhandled exceptions in native
delegates (see <http://msdn.microsoft.com/en-us/library/ms228965.aspx>).
More comments inline...
> Index: DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule.cs
> ===================================================================
> --- DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule.cs
> (revision 0)
> +++ DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule.cs
> (revision 125705)
> @@ -0,0 +1,458 @@
> +//
> +//
> Gendarme
> .Rules
> .Interoperability
> .DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule
> +//
> +// Authors:
> +// Rolf Bjarne Kvinge <RKv...@novell.com>
> +//
> +// Copyright (C) 2009 Novell, Inc (http://www.novell.com)
> +//
> +// 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.Engines;
> +using Gendarme.Framework.Helpers;
> +using Gendarme.Framework.Rocks;
> +
> +namespace Gendarme.Rules.Interoperability {
> +
> + /// <summary>
> + /// <code>Every delegate which is passed to native code must
> include an exception block which spans the entire method and has a
> catch clause with no condition.</code>
This language seems a trifle clumsy. Something like "and has a catch
all block." would be clearer I think.
>
> + /// </summary>
> + /// <example>
> + /// Bad example:
> + /// <code>
> + /// public void NativeCallback ()
> + /// {
> + /// Console.WriteLine ("{0}", 1);
> + /// }
> + /// </code>
> + /// </example>
> + /// <example>
> + /// Good example:
> + /// <code>
> + /// public void NativeCallback ()
> + /// {
> + /// try {
> + /// Console.WriteLine ("{0}", 1);
> + /// } catch {
> + /// }
> + /// }
>
> + /// </code>
> + /// </example>
> +
> + [Problem ("Every delegate passed to native code must include an
> exception block which spans the entire method and has a catch clause
> with no condition.")]
> + [Solution ("Surround the entire method body with a try/catch
> block.")]
> + [EngineDependency (typeof (OpCodeEngine))]
> + public class
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule : Rule,
> IMethodRule {
Should probably seal the class.
>
> + /// <summary>
> + /// A list of methods which have been verified to be safe to call
> from native code.
> + /// </summary>
> + Dictionary<MethodDefinition, bool> verified_methods;
> +
> + /// <summary>
> + /// A list of methods which have been reported to be unsafe (to
> not report the same method twice).
> + /// </summary>
> + List<MethodDefinition> reported_methods;
> +
> + /// <summary>
> + /// A list of all the fields which have been passed to pinvokes
> (as a delegate parameter).
> + /// We report an error if a ld[s]fld stores an unsafe function
> pointer into any of these fields.
> + /// </summary>
> + List<FieldDefinition> fields_loads;
> +
> + /// <summary>
> + /// A list of all the fields which have been assigned function
> pointers.
> + /// We report an error if a pinvoke loads any of these fields.
> + /// </summary>
> + Dictionary<FieldDefinition, List<MethodDefinition>> field_stores;
> +
> + public RuleResult CheckMethod (MethodDefinition method)
> + {
> + MethodDefinition called_method;
> + List<MethodDefinition> pointers;
> + List<List<MethodDefinition>> locals = new
> List<List<MethodDefinition>> ();
> + List<ILRange> stack = new List<ILRange> (); // A rude way of
> assigning code sequences to stack positions.
> +
> + // Rule does not apply if the method has no IL
> + if (!method.HasBody)
> + return RuleResult.DoesNotApply;
Do this first thing so allocations can be skipped. It should also be
possible to use OpCodeBitmask to prune more methods.
>
>
> +
> + // Console.WriteLine ("Checking method: {0} on type: {1}",
> method.Name, method.DeclaringType.FullName);
It's better to use the logger, especially for complex methods like
this. The logger will allow you to dump the disassembled methods which
is very handy and allows people to easily debug your rule without
going in and manually un-commenting the CWLs.
>
> +
> + //
> + // We check the following code patterns:
> + // - All delegate expressions created and passed directly to the
> pinvoke
> + // - All delegate expressions created and stored in fields or
> local variables and then passed to the pinvoke.
> + //
> + // To do the second item, we keep track of all reads from fields
> to pinvoke and stores to fields (with delegates)
> + // and report an error if the delegate is unsafe in either case
> (i.e. we read from a field which has been
> + // assigned an unsafe delegate, or we store an unsafe delegate
> in a field which have been used in a pinvoke).
> + // This way only one pass over the code is required.
> + //
> + // No other static analysis is done, which means that for things
> like this we'll report false positives:
> + // delegate v = SafeMethod;
> + // PInvoke (v);
> + // v = UnsafeMethod;
> + //
> + //
> + // There are several ways to get around this:
> + // - Create a delegate, store it in a array/list and then pass
> it to a pinvoke.
> + // - Create a delegate, return it from a function and use the
> return value directly in a call to a pinvoke.
> + // - ...
> + //
> +
> + int stack_count = 0;
> + foreach (Instruction ins in method.Body.Instructions) {
> + int push = ins.GetPushCount ();
> + int pop = ins.GetPopCount (method);
> +
> + // Console.WriteLine ("before {0} + push {1} - pop {2} = after
> {3} {4}", stack_count, push, pop, stack_count + push - pop, ToString
> (ins));
> + // Console.WriteLine ("{4}", stack_count, push, pop,
> stack_count + push - pop, ToString (ins));
> +
> + if (ins.OpCode.Code == Code.Call) {
> + called_method = (ins.Operand as MethodReference).Resolve ();
> +
> + if (called_method != null && called_method.IsPInvokeImpl) {
> + // Console.WriteLine ("Reached a call instruction to a
> pinvoke method: {0}", called_method.Name);
> +
> + for (int i = 0; i < called_method.Parameters.Count; i++) {
> + if (!called_method.Parameters [i].ParameterType.IsDelegate ())
> + continue;
> +
> + // Console.WriteLine (" Stack slot #{0}:", i);
> + // stack [i].DumpAll (" ");
> +
> + // if we load a field, store the field so that any
> subsequent unsafe writes to that field can be reported.
> + Instruction last = stack [i].Last;
> + if (last.OpCode.Code == Code.Ldfld || last.OpCode.Code ==
> Code.Ldsfld) {
> + FieldDefinition field = (last.Operand as
> FieldReference).Resolve ();
> + if (fields_loads == null)
> + fields_loads = new List<FieldDefinition> ();
I'm not so sure it's an optimization to constantly perform these null
checks. Probably better to just allocate them up front and clear them
on rule entry.
>
> + if (!fields_loads.Contains (field))
> + fields_loads.Add (field);
> + }
> +
> + // Get and check the pointers
> + VerifyMethods (GetDelegatePointers (method, locals, stack
> [i]));
> + }
> + }
> + } else if (ins.OpCode.Code == Code.Stfld || ins.OpCode.Code ==
> Code.Stsfld) {
> + FieldDefinition field = (ins.Operand as
> FieldReference).Resolve ();
> +
> + pointers = GetDelegatePointers (method, locals, stack
> [stack_count - 1]);
> +
> + // Console.WriteLine (" Reached a field variable store to the
> field {0}, there are {1} unsafe pointers here.", field.Name,
> pointers == null ? 0 : pointers.Count);
> +
> + if (pointers != null && pointers.Count > 0) {
> + List<MethodDefinition> tmp;
> + if (field_stores == null) {
> + field_stores = new Dictionary<FieldDefinition,
> List<MethodDefinition>> ();
> + tmp = new List<MethodDefinition> ();
> + field_stores.Add (field, tmp);
> + } else if (!field_stores.TryGetValue (field, out tmp)) {
> + tmp = new List<MethodDefinition> ();
> + field_stores.Add (field, tmp);
> + }
> + tmp.AddRange (pointers);
> + }
> +
> + // If this field has been loaded into a pinvoke,
> + // check the list for unsafe pointers.
> + if (fields_loads != null && fields_loads.Contains (field))
> + VerifyMethods (pointers);
> + } else if (ins.IsStoreLocal ()) {
> + int index = ins.GetStoreIndex ();
> +
> + pointers = GetDelegatePointers (method, locals, stack
> [stack_count - 1]);
> +
> + // Console.WriteLine (" Reached a local variable store at
> index {0}, there are {1} pointers here.", index, pointers == null ?
> 0 : pointers.Count);
> +
> + if (pointers != null && pointers.Count > 0) {
> + while (locals.Count <= index)
> + locals.Add (null);
> + if (locals [index] == null)
> + locals [index] = new List<MethodDefinition> ();
> + locals [index].AddRange (pointers);
> + }
> + }
> +
> + stack_count += push - pop;
> +
> + while (stack_count > stack.Count)
> + stack.Add (new ILRange (ins));
> + while (stack_count < stack.Count)
> + stack.RemoveAt (stack.Count - 1);
> +
> + for (int i = 0; i < stack_count; i++)
> + stack [i].Last = ins;
> + }
> +
> + // Console.WriteLine ("Checking method: {0} [Done]", method.Name);
> +
> + return Runner.CurrentRuleResult;
> + }
> +
> + /// <summary>
> + /// Verifies that the method is safe to call as a callback from
> native code.
> + /// </summary>
> + private bool VerifyCallbackSafety (MethodDefinition callback)
> + {
> + bool result;
> + bool valid_ex_handler = false;
> +
> + if (callback == null)
> + return true;
> +
> + if (!callback.HasBody)
> + return true;
> +
> + if (verified_methods != null && verified_methods.TryGetValue
> (callback, out result))
> + return result;
> +
> + if (callback.Body.HasExceptionHandlers) {
> + foreach (ExceptionHandler eh in
> callback.Body.ExceptionHandlers) {
> + /*
> + Console.WriteLine ("HandlerStart: {0}, HandlerEnd: {1},
> FilterStart: {2}, FilterEnd: {3}, TryStart: {4}, TryEnd: {5},
> CatchType: {6}, Type: {7}",
> + GetOffset (eh.HandlerStart), GetOffset
> (eh.HandlerEnd), GetOffset (eh.FilterStart), GetOffset (eh.FilterEnd),
> + GetOffset (eh.TryStart), GetOffset
> (eh.TryEnd), eh.CatchType, eh.Type);
> + */
> +
> + /*
> + * Here we could get a lot stricter, we accept the following
> code:
> + *
> + * void Method ()
> + * {
> + * // no code here
> + * try {
> + * // ... code ..
> + * [ } catch (Exception ex } ]
> + * [ // ... code ... ]
> + * } catch {
> + * // ... code ...
> + * [ } finally { ]
> + * [ // ... code ... ]
> + * }
> + * // no code here
> + * }
> + *
> + * There are a few ways to break this
> + * - any of the catch clauses may throw an exception.
> + * - finally clause may throw an exception.
> + *
> + * The stricter rules would ensure that:
> + * - all code in catch and finally clauses are also embedded
> in a simple try { <code> } catch { <no code> } block
> + *
> + */
> +
> + // We only care about catch clauses.
> + if (eh.Type != ExceptionHandlerType.Catch)
> + continue;
> +
> + // no 'Catch ... When <condition>' clause. C# doesn't support
> it, VB does
> + if (eh.FilterStart != null || eh.FilterEnd != null)
> + continue;
> +
> + // exception clause has to start at the beginning of the method
> + if (eh.TryStart == null || eh.TryStart.Offset != 0)
> + continue;
> +
> + // and span the entire method
> + if (eh.HandlerEnd == null || eh.HandlerEnd.Offset !=
> callback.Body.CodeSize - 1)
> + continue;
> +
> + // check for empty catch clause catching every single exception
> + // we don't allow 'catch (Exception)' because in IL it's valid
> to
> + // throw any object.
> + if (eh.CatchType != null && eh.CatchType.FullName !=
> "System.Object")
> + continue;
> +
> + valid_ex_handler = true;
It would be really nice to handle throws in the catch blocks. People
do that a lot so it's a fairly serious hole in the rule.
>
> + break;
> + }
> + }
> +
> + if (verified_methods == null)
> + verified_methods = new Dictionary<MethodDefinition, bool> ();
> +
> + verified_methods.Add (callback, valid_ex_handler);
> +
> + return valid_ex_handler;
> + }
> +
> + private class ILRange {
> + public Instruction First;
> + public Instruction Last;
> + public ILRange (Instruction first)
> + {
> + First = first;
> + }
> + public void DumpAll (string prefix)
> + {
> + Instruction instr = First;
> + do {
> + Console.WriteLine ("{0}{1}", prefix, instr.ToPrettyString ());
> + if (instr == Last)
> + break;
> + instr = instr.Next;
> + } while (true);
> + }
> + }
> +
> + /// <summary>
> + /// Parses the ILRange and return all delegate pointers that
> could end up on the stack as a result of executing that code.
> + /// </summary>
> + private List<MethodDefinition> GetDelegatePointers
> (MethodDefinition method, List<List<MethodDefinition>> locals,
> ILRange range)
> + {
> + List<MethodDefinition> result = null;
> + Instruction current;
> +
> + // Check if the code does any ldftn.
> + current = range.First;
> + do {
> + if (current.OpCode.Code == Code.Ldftn) {
> + if (result == null)
> + result = new List<MethodDefinition> ();
>
> + result.Add ((current.Operand as MethodReference).Resolve ());
I think the code works, but it seems fragile to populate the list with
nulls, especially when the potential breakage does not normally happen.
>
> + }
> + if (current == range.Last)
> + break;
> + current = current.Next;
> + } while (true);
> +
> + // Check if the code loads any local variables which can be
> delegates.
> + if (locals != null && range.Last.IsLoadLocal ()) {
> + int index = range.Last.GetLoadIndex ();
> + if (locals.Count > index) {
> + List<MethodDefinition> pointers = locals [index];
> + if (pointers != null && pointers.Count != 0) {
> + if (result == null)
> + result = new List<MethodDefinition> ();
> + result.AddRange (pointers);
> + }
> + }
> + }
> +
> + // If the last opcode is a field load, check if any pointers
> have been stored in that field.
> + if (field_stores != null && (range.Last.OpCode.Code ==
> Code.Ldfld || range.Last.OpCode.Code == Code.Ldsfld)) {
> + FieldDefinition field = (range.Last.Operand as
> FieldReference).Resolve ();
> + List<MethodDefinition> pointers;
> + if (field_stores.TryGetValue (field, out pointers)) {
> + if (result == null)
> + result = new List<MethodDefinition> ();
> + result.AddRange (pointers);
> + }
> +
> + }
> +
> + return result;
> + }
> +
> +
> + /// <summary>
> + /// Verifies that all methods in the list are safe to call from
> native code,
> + /// otherwise reports the correspoding result.
> + /// </summary>
> + private void VerifyMethods (List<MethodDefinition> pointers)
> + {
> + if (pointers == null)
> + return;
> +
> + foreach (MethodDefinition pointer in pointers)
> + ReportVerifiedMethod (pointer, VerifyCallbackSafety (pointer));
> + }
> +
> + /// <summary>
> + /// Reports the result from verifying the method.
> + /// </summary>
> + private void ReportVerifiedMethod (MethodDefinition pointer, bool
> safe)
> + {
> + if (!safe) {
> + if (reported_methods == null)
> + reported_methods = new List<MethodDefinition> ();
> + reported_methods.Add (pointer);
> + Runner.Report (pointer, Severity.High, Confidence.High);
> + }
> + }
> + }
> +
> + internal static class
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRuleHelper
> + {
> +
> + public static string ToPrettyString (this Instruction instr)
> + {
> + if (instr == null)
> + return "<nil>";
> +
> + return string.Format ("IL_{0} {1} {2}", instr.Offset,
> instr.OpCode.Name, instr.Operand);
> + }
> +
> + public static int GetOffset (this Instruction instr)
> + {
> + if (instr != null)
> + return instr.Offset;
> + return -1;
> + }
> +
> + /// <summary>
> + /// Return the index of the load opcode.
> + /// This could probably go into InstructionRocks.
> + /// </summary>
> + public static int GetLoadIndex (this Instruction ins)
> + {
> + switch (ins.OpCode.Code) {
> + case Code.Ldloc_0: return 0;
> + case Code.Ldloc_1: return 1;
> + case Code.Ldloc_2: return 2;
> + case Code.Ldloc_3: return 3;
> + case Code.Ldloc: // Untested for ldloc
> + case Code.Ldloc_S: return ((VariableDefinition)
> ins.Operand).Index;
> + default:
> + throw new Exception (string.Format ("Invalid opcode: {0}",
> ins.OpCode.Name));
> + }
> + }
> +
> + /// <summary>
> + /// Return the index of the store opcode.
> + /// This could probably go into InstructionRocks.
> + /// </summary>
> + public static int GetStoreIndex (this Instruction ins)
> + {
> + switch (ins.OpCode.Code) {
> + case Code.Stloc_0: return 0;
> + case Code.Stloc_1: return 1;
> + case Code.Stloc_2: return 2;
> + case Code.Stloc_3: return 3;
> + case Code.Stloc: // Untested for stloc
> + case Code.Stloc_S: return ((VariableDefinition)
> ins.Operand).Index;
> + default:
> + throw new Exception (string.Format ("Invalid opcode: {0}",
> ins.OpCode.Name));
> + }
> + }
> + }
> +
> +}
> +
> Index: Test/
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs
> ===================================================================
> --- Test/
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs
> (revision 0)
> +++ Test/
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs
> (revision 125705)
> @@ -0,0 +1,655 @@
> +//
> +// Unit tests for
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest
> +//
> +// Authors:
> +// Rolf Bjarne Kvinge <RKv...@novell.com>
> +//
> +// Copyright (C) 2009 Novell, Inc (http://www.novell.com)
> +//
> +// 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.Reflection;
> +using System.Runtime.InteropServices;
> +using System.Text;
> +
> +using Gendarme.Framework;
> +using Gendarme.Rules.Interoperability;
> +using Mono.Cecil;
> +
> +using NUnit.Framework;
> +using Test.Rules.Helpers;
> +using Test.Rules.Fixtures;
> +
> +namespace Test.Rules.Interoperability {
> +
> + [TestFixture]
> + public class
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest :
> MethodRuleTestFixture
> <DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule>
> + {
> +
> + private
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule rule;
> + private AssemblyDefinition assembly;
> + private TypeDefinition type;
> +
> + private TestRunner runner {
> + get {
> + return new TestRunner (new
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule ());
> + }
> + }
> +
> + public delegate void CallbackDelegate ();
> +
> + [DllImport ("library.dll")]
> + static extern void PInvokeNormal ();
> +
> + [DllImport ("library.dll")]
> + static extern void PInvokeDelegate1 (CallbackDelegate d);
> +
> + [DllImport ("library.dll")]
> + static extern void PInvokeDelegate2 (object obj, CallbackDelegate
> d);
> +
> + [DllImport ("library.dll")]
> + static extern void PInvokeDelegate3 (CallbackDelegate d1, object
> obj, CallbackDelegate d3);
> +
> +#region Callback methods
> + private CallbackDelegate CallbackOK1_Field;
> + private CallbackDelegate CallbackOK1_StaticField;
> + private void CallbackOK1 ()
> + {
> + int a, b;
> + try {
> + a = 1;
> + } catch {
> + b = 2;
> + }
> + }
> +
> + private CallbackDelegate CallbackOKStatic_Field;
> + private CallbackDelegate CallbackOKStatic_StaticField;
> + private static void CallbackOKStatic ()
> + {
> + int a, b;
> + try {
> + a = 1;
> + } catch {
> + b = 2;
> + }
> + }
> +
> + private CallbackDelegate CallbackFailEmpty_Field;
> + private CallbackDelegate CallbackFailEmpty_StaticField;
> + private void CallbackFailEmpty ()
> + {
> + }
> +
> + private CallbackDelegate CallbackFailEmptyStatic_Field;
> + private CallbackDelegate CallbackFailEmptyStatic_StaticField;
> + private static void CallbackFailEmptyStatic ()
> + {
> + }
> +
> + private void CallbackFailNoCatch ()
> + {
> + try {
> + } finally {
> + }
> + }
> +
> + private void CallbackFailNoEmptyCatch ()
> + {
> + try {
> + } catch (Exception ex) {
> + }
> + }
> +
> + private void CallbackFailNotEntireMethod1 ()
> + {
> + int i = 0;
> + try {
> + } catch {
> + }
> + }
> +
> + private void CallbackFailNotEntireMethod2 ()
> + {
> + int i;
> + try {
> + } catch {
> + }
> + i = 0;
> + }
> +
> + private void CallbackFailNoEmptyCatchEntireMethod ()
> + {
> + try {
> + int a = 1;
> + try {
> + int b = 2;
> + } catch {
> + int c = 3;
> + }
> + } catch (Exception ex) {
> + int d = 4;
> + }
> + }
> + #endregion
> +
> + [TestFixtureSetUp]
> + public void FixtureSetUp ()
> + {
> + string unit = Assembly.GetExecutingAssembly ().Location;
> + assembly = AssemblyFactory.GetAssembly (unit);
> + type = assembly.MainModule.Types
> ["Test
> .Rules
> .Interoperability
> .DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest"];
> + }
> +
> + private MethodDefinition GetTest (string name)
> + {
> + foreach (MethodDefinition method in type.Methods) {
> + if (method.Name == name)
> + return method;
> + }
> + return null;
> + }
> +
> + private TypeDefinition GetTypeTest (string name)
> + {
> + Console.WriteLine ("GetTypeTest ('{1}') NestedType count: {0}",
> this.type.NestedTypes.Count, name);
> + foreach (TypeDefinition type in this.type.NestedTypes) {
> + Console.WriteLine (" Name: '{0}'", type.Name);
> + if (type.Name == name)
> + return type;
> + }
> + return null;
> + }
> +
> + private void TestMethod_OK1 ()
> + {
> + CallbackDelegate c = CallbackFailEmpty;
> + // Create a bad delegate but not pass it to a pinvoke.
> + PInvokeNormal ();
> + }
> +
> + private void TestMethod_AnonymousOK1 ()
> + {
> + CallbackDelegate c = delegate () {};
> + // Create a bad delegate but not pass it to a pinvoke.
> + PInvokeNormal ();
> + }
> +
> + private void TestMethod_OK2 ()
> + {
> + CallbackDelegate a = CallbackFailEmpty;
> + CallbackDelegate b = CallbackOK1;
> + // Create a bad delegate and a good delegate, pass the good
> delegate.
> + PInvokeDelegate1 (b);
> + }
> +
> + private void TestMethod_AnonymousOK2 ()
> + {
> + CallbackDelegate a = delegate () { };
> + CallbackDelegate b = delegate () { try {} catch {} };
> + // Create a bad delegate and a good delegate, pass the good
> delegate.
> + PInvokeDelegate1 (b);
> + }
> +
> + private void TestMethod_CallbackOK1 ()
> + {
> + CallbackDelegate c = CallbackOK1;
> +
> + CallbackOK1_Field = CallbackOK1;
> + CallbackOK1_StaticField = CallbackOK1;
> +
> + PInvokeDelegate1 (CallbackOK1);
> + PInvokeDelegate1 (new CallbackDelegate (CallbackOK1));
> + PInvokeDelegate1 (c);
> + PInvokeDelegate1 (CallbackOK1_Field);
> + PInvokeDelegate1 (CallbackOK1_StaticField);
> + PInvokeDelegate2 (null, CallbackOK1);
> + PInvokeDelegate2 (new CallbackDelegate (CallbackOK1), null);
> + PInvokeDelegate3 (CallbackOK1_Field, null,
> CallbackOK1_StaticField);
> + }
> +
> + private void TestMethod_CallbackOKStatic ()
> + {
> + CallbackDelegate c = CallbackOKStatic;
> +
> + CallbackOKStatic_Field = CallbackOKStatic;
> + CallbackOKStatic_StaticField = CallbackOKStatic;
> + PInvokeDelegate1 (CallbackOKStatic);
> + PInvokeDelegate1 (new CallbackDelegate (CallbackOKStatic));
> + PInvokeDelegate1 (c);
> + PInvokeDelegate1 (CallbackOKStatic_Field);
> + PInvokeDelegate1 (CallbackOKStatic_StaticField);
> + PInvokeDelegate2 (null, CallbackOKStatic);
> + PInvokeDelegate2 (new CallbackDelegate (CallbackOKStatic), null);
> + PInvokeDelegate3 (CallbackOKStatic_Field, null,
> CallbackOKStatic_StaticField);
> + }
> +
> + private void TestMethod_AnonymousCallbackOK1 ()
> + {
> + // These anonymous delegates will turn out non-static since they
> access a method variable in outer scope.
> + int o = 7;
> + CallbackDelegate c = delegate () { int a, b; try { a = o; }
> catch { b = o; } };
> +
> + CallbackOKStatic_Field = delegate () { int a, b; try { a = o; }
> catch { b = o; } };
> + CallbackOKStatic_StaticField = delegate () { int a, b; try { a =
> o; } catch { b = o; } };
> + PInvokeDelegate1 (delegate () { int a, b; try { a = o; } catch
> { b = o; } });
> + PInvokeDelegate1 (new CallbackDelegate (delegate () { int a, b;
> try { a = o; } catch { b = o; } }));
> + PInvokeDelegate1 (c);
> + PInvokeDelegate1 (CallbackOKStatic_Field);
> + PInvokeDelegate1 (CallbackOKStatic_StaticField);
> + PInvokeDelegate2 (null, delegate () { int a, b; try { a = o; }
> catch { b = o; } });
> + PInvokeDelegate2 (new CallbackDelegate (delegate () { int a, b;
> try { a = o; } catch { b = o; } }), null);
> + PInvokeDelegate3 (CallbackOKStatic_Field, null,
> CallbackOKStatic_StaticField);
> + }
There should be a test where the native method argument instructions
are more than trivial loads to verify that the trace back code is
working.
>
> +
> + private void TestMethod_AnonymousCallbackOKStatic ()
> + {
> + // These anonymous delegates will turn out static given that
> they don't access any class/method variables in outer scope.
> + CallbackDelegate c = delegate () { int a, b; try { a = 1; }
> catch { b = 2; } };
> +
> + CallbackOK1_Field = delegate () { int a, b; try { a = 1; } catch
> { b = 2; } };
> + CallbackOK1_StaticField = delegate () { int a, b; try { a = 1; }
> catch { b = 2; } };
> +
> + PInvokeDelegate1 (delegate () { int a, b; try { a = 1; } catch
> { b = 2; } });
> + PInvokeDelegate1 (new CallbackDelegate (delegate () { int a, b;
> try { a = 1; } catch { b = 2; } }));
> + PInvokeDelegate1 (c);
> + PInvokeDelegate1 (CallbackOK1_Field);
> + PInvokeDelegate1 (CallbackOK1_StaticField);
> + PInvokeDelegate2 (null, delegate () { int a, b; try { a = 1; }
> catch { b = 2; } });
> + PInvokeDelegate2 (new CallbackDelegate (delegate () { int a, b;
> try { a = 1; } catch { b = 2; } }), null);
> + PInvokeDelegate3 (CallbackOK1_Field, null,
> CallbackOK1_StaticField);
> + }
> +
> + private void TestMethod_CallbackFailEmpty_a ()
> + {
> + PInvokeDelegate1 (CallbackFailEmpty);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmpty_a ()
> + {
> + PInvokeDelegate1 (delegate () {});
> + }
> +
> + private void TestMethod_CallbackFailEmpty_b ()
> + {
> + PInvokeDelegate1 (new CallbackDelegate (CallbackFailEmpty));
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmpty_b ()
> + {
> + PInvokeDelegate1 (new CallbackDelegate (delegate () {}));
> + }
> +
> + private void TestMethod_CallbackFailEmpty_c ()
> + {
> + CallbackDelegate c = CallbackFailEmpty;
> +
> + PInvokeDelegate1 (c);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmpty_c ()
> + {
> + CallbackDelegate c = delegate () {};
> +
> + PInvokeDelegate1 (c);
> + }
> +
> + private void TestMethod_CallbackFailEmpty_d ()
> + {
> + CallbackFailEmptyStatic_Field = CallbackFailEmpty;
> +
> + PInvokeDelegate1 (CallbackFailEmptyStatic_Field);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmpty_d ()
> + {
> + CallbackFailEmptyStatic_Field = delegate () {};
> +
> + PInvokeDelegate1 (CallbackFailEmptyStatic_Field);
> + }
> +
> + private void TestMethod_CallbackFailEmpty_e ()
> + {
> + CallbackFailEmptyStatic_StaticField = CallbackFailEmpty;
> +
> + PInvokeDelegate1 (CallbackFailEmptyStatic_StaticField);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmpty_e ()
> + {
> + CallbackFailEmptyStatic_StaticField = delegate () {};
> +
> + PInvokeDelegate1 (CallbackFailEmptyStatic_StaticField);
> + }
> +
> + private void TestMethod_CallbackFailEmpty_f ()
> + {
> + PInvokeDelegate2 (null, CallbackFailEmpty);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmpty_f ()
> + {
> + PInvokeDelegate2 (null, delegate () {});
> + }
> +
> + private void TestMethod_CallbackOKEmpty_g ()
> + {
> + PInvokeDelegate2 (new CallbackDelegate (CallbackFailEmpty), null);
> + }
> +
> + private void TestMethod_AnonymousCallbackOKEmpty_g ()
> + {
> + PInvokeDelegate2 (new CallbackDelegate (delegate () {}), null);
> + }
> +
> + private void TestMethod_CallbackFailEmpty_h ()
> + {
> + CallbackFailEmptyStatic_Field = CallbackFailEmpty;
> + CallbackFailEmptyStatic_StaticField = CallbackFailEmpty;
> +
> + PInvokeDelegate3 (CallbackFailEmptyStatic_Field, null,
> CallbackFailEmptyStatic_StaticField);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmpty_h ()
> + {
> + CallbackFailEmptyStatic_Field = delegate () {};
> + CallbackFailEmptyStatic_StaticField = delegate () {};
> +
> + PInvokeDelegate3 (CallbackFailEmptyStatic_Field, null,
> CallbackFailEmptyStatic_StaticField);
> + }
> +
> + class TestClass_InstanceFieldFail {
> + private CallbackDelegate field;
> + public void DoBadA ()
> + {
> +
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest
> .PInvokeDelegate1 (field);
> + }
> + public void Set ()
> + { // Note that we set the field (textually) after using the
> pinvoke above.
> + field =
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest
> .CallbackFailEmptyStatic;
> + }
> + }
> +
> + class TestClass_StaticFieldFail {
> + private static CallbackDelegate field;
> + public void Set ()
> + {
> + // Reverse textual order from instance field test above.
> + field =
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest
> .CallbackFailEmptyStatic;
> + }
> + public void DoBadA ()
> + {
> +
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest
> .PInvokeDelegate1 (field);
> + }
> + }
> +
> + class TestClass_AnonymousInstanceFieldFail {
> + private CallbackDelegate field;
> + public void DoBadA ()
> + {
> +
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest
> .PInvokeDelegate1 (field);
> + }
> + public void Set ()
> + { // Note that we set the field (textually) after using the
> pinvoke above.
> + field = delegate () {};
> + }
> + }
> +
> + class TestClass_AnonymousStaticFieldFail {
> + private static CallbackDelegate field;
> + public void Set ()
> + {
> + // Reverse textual order from instance field test above.
> + field = delegate () {};
> + }
> + public void DoBadA ()
> + {
> +
> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest
> .PInvokeDelegate1 (field);
> + }
> + }
> +
> + private void TestMethod_CallbackFailEmptyStatic ()
> + {
> + PInvokeDelegate1 (CallbackFailEmptyStatic);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailEmptyStatic ()
> + {
> + PInvokeDelegate1 (delegate () { });
> + }
> +
> + private void TestMethod_CallbackFailNoCatch ()
> + {
> + PInvokeDelegate1 (CallbackFailNoCatch);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailNoCatch ()
> + {
> + PInvokeDelegate1 (delegate () { try { } finally {} });
> + }
> +
> + private void TestMethod_CallbackFailNoEmptyCatch ()
> + {
> + PInvokeDelegate1 (CallbackFailNoEmptyCatch);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailNoEmptyCatch ()
> + {
> + PInvokeDelegate1 (delegate () { try { } catch (Exception ex)
> { } });
> + }
> +
> + private void TestMethod_CallbackFailNotEntireMethod1 ()
> + {
> + PInvokeDelegate1 (CallbackFailNotEntireMethod1);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailNotEntireMethod1 ()
> + {
> + PInvokeDelegate1 (delegate () { int i = 0; try { } catch { } });
> + }
> +
> + private void TestMethod_CallbackFailNotEntireMethod2 ()
> + {
> + PInvokeDelegate1 (CallbackFailNotEntireMethod2);
> + }
> +
> + private void TestMethod_AnonymousCallbackFailNotEntireMethod2 ()
> + {
> + PInvokeDelegate1 (delegate () { int i; try { } catch { } i =
> 0; } );
> + }
> +
> + private void TestMethod_CallbackFailNoEmptyCatchEntireMethod ()
> + {
> + PInvokeDelegate1 (CallbackFailNoEmptyCatchEntireMethod);
> + }
> +
> + private void
> TestMethod_AnonymousCallbackFailNoEmptyCatchEntireMethod ()
> + {
> + PInvokeDelegate1 (delegate () { try { int a = 1; try { int b =
> 2; } catch { int c = 3; } } catch (Exception ex) { int d = 4; } });
> + }
> +
> + private void AssertTest (string name)
> + {
> + bool success = name.Contains ("OK");
> + if (success) {
> +
> AssertRuleSuccess
> <DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest> (name);
> + } else {
> +
> AssertRuleFailure
> <DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest> (name);
> + }
> + }
> +
> + private void AssertClass<T> ()
> + {
> + bool failed = false;
> + RuleResult result;
> +
> + foreach (MethodDefinition method in
> DefinitionLoader.GetTypeDefinition <T> ().Methods) {
> + result = ((TestRunner) Runner).CheckMethod (method);
> + if (result == RuleResult.Failure)
> + failed = true;
> + }
> +
> + Assert.IsTrue (failed);
> + }
> +
> + [Test]
> + public void Test_OK1 ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_OK2 ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackOK1 ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackOKStatic ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmpty_a ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmpty_b ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmpty_c ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmpty_d ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmpty_e ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmpty_f ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackOKEmpty_g ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmpty_h ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_StaticFieldFail ()
> + {
> + AssertClass<TestClass_StaticFieldFail> ();
> + AssertClass<TestClass_AnonymousStaticFieldFail> ();
> + }
> +
> + [Test]
> + public void Test_InstanceFieldFail ()
> + {
> + AssertClass<TestClass_InstanceFieldFail> ();
> + AssertClass<TestClass_AnonymousInstanceFieldFail> ();
> + }
> +
> + [Test]
> + public void Test_CallbackFailEmptyStatic ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailNoCatch ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailNoEmptyCatch ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailNotEntireMethod1 ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailNotEntireMethod2 ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + [Test]
> + public void Test_CallbackFailNoEmptyCatchEntireMethod ()
> + {
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_"));
> + AssertTest (MethodInfo.GetCurrentMethod ().Name.Replace
> ("Test_", "TestMethod_Anonymous"));
> + }
> +
> + }
> +}
-- Jesse
Rolf Bjarne Kvinge
Attached is a the rule with a few changes:
* Using Log.WriteLine instead of Console.WriteLine.
* The verification for delegates has changed, the rule now verifies
that all instructions in the method are safe, i.e. the instruction
itself is either one that will never throw an exception, or covered by
a catch all exception block. This means that the following two cases
are now reported:
try {
// code
} catch {
throw new Exception (); // throwing exceptions is of course unsafe
}
try {
// code
} catch (Exception ex) {
Console.WriteLine (ex); // any method calls are unsafe
}
A lot of instructions are marked as safe (list is possibly incomplete
though), so this can still be done:
try {
return 1 / 0;
} catch {
return 123;
}
More comments inline...
On Thu, Feb 5, 2009 at 4:32 AM, Jesse Jones <jesj...@mindspring.com> wrote:
>
> I like the idea behind this rule a lot. I wonder though, if it should
> be a more general rule. For example, unhandled exceptions in thread
> callbacks are nearly as evil as unhandled exceptions in native
> delegates (see <http://msdn.microsoft.com/en-us/library/ms228965.aspx>).
>
You mean checking delegates passed to System.Thread's constructor?
This could be done, the verification logic would be exactly the same.
However I'm not sure how to name such a rule, nor where it should live
(Gendarme.Rules.Interoperability would be wrong for this).
(...)
>> + /// <code>Every delegate which is passed to native code must
>> include an exception block which spans the entire method and has a
>> catch clause with no condition.</code>
>
> This language seems a trifle clumsy. Something like "and has a catch
> all block." would be clearer I think.
Changed.
>> + public class
>> DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule : Rule,
>> IMethodRule {
>
> Should probably seal the class.
Changed.
>> + // Rule does not apply if the method has no IL
>> + if (!method.HasBody)
>> + return RuleResult.DoesNotApply;
>
> Do this first thing so allocations can be skipped. It should also be
> possible to use OpCodeBitmask to prune more methods.
Changed.
>> +
>> + // Console.WriteLine ("Checking method: {0} on type: {1}",
>> method.Name, method.DeclaringType.FullName);
>
> It's better to use the logger, especially for complex methods like
> this. The logger will allow you to dump the disassembled methods which
> is very handy and allows people to easily debug your rule without
> going in and manually un-commenting the CWLs.
>
Changed, though I think a Log.IsEnabled<T> (T obj) would be good to
add, as you can see I call Log.IsEnabled (GetType ().Name) a few times
to skip several lines of code.
>> + if (fields_loads == null)
>> + fields_loads = new List<FieldDefinition> ();
>
> I'm not so sure it's an optimization to constantly perform these null
> checks. Probably better to just allocate them up front and clear them
> on rule entry.
Changed.
>> +
>> + valid_ex_handler = true;
>
> It would be really nice to handle throws in the catch blocks. People
> do that a lot so it's a fairly serious hole in the rule.
Logic has been changed as noted in the beginning of the mail.
>>
>> + result.Add ((current.Operand as MethodReference).Resolve ());
>
> I think the code works, but it seems fragile to populate the list with
> nulls, especially when the potential breakage does not normally happen.
Changed.
Rolf
Jesse Jones
On Feb 5, 2009, at 10:24 AM, Rolf Bjarne Kvinge wrote:
> On Thu, Feb 5, 2009 at 4:32 AM, Jesse Jones
> <jesj...@mindspring.com> wrote:
>>
>> I like the idea behind this rule a lot. I wonder though, if it should
>> be a more general rule. For example, unhandled exceptions in thread
>> callbacks are nearly as evil as unhandled exceptions in native
>> delegates (see <http://msdn.microsoft.com/en-us/library/
>> ms228965.aspx>).
>>
>
> You mean checking delegates passed to System.Thread's constructor?
> This could be done, the verification logic would be exactly the same.
Yeah, and thread pool and thread timers as well if they have the same
issue (async methods don't). I think the logic would be slightly
different though: an explicit throw from a catch block should either
be OK or be at a low confidence if the developer wants to simply throw
up his hands and allow the app to abort.
>
> However I'm not sure how to name such a rule, nor where it should live
> (Gendarme.Rules.Interoperability would be wrong for this).
I dunno, but they are pretty similar scenarios...
>> It's better to use the logger, especially for complex methods like
>> this. The logger will allow you to dump the disassembled methods
>> which
>> is very handy and allows people to easily debug your rule without
>> going in and manually un-commenting the CWLs.
>>
>
> Changed, though I think a Log.IsEnabled<T> (T obj) would be good to
> add, as you can see I call Log.IsEnabled (GetType ().Name) a few times
> to skip several lines of code.
The best way to do this is to wrap the code with `#if DEBUG`.
>>
>>> +
>>> + valid_ex_handler = true;
>>
>> It would be really nice to handle throws in the catch blocks. People
>> do that a lot so it's a fairly serious hole in the rule.
>
> Logic has been changed as noted in the beginning of the mail.
Sweet. How do the system assemblies look? Are there any false positives?
>
>
>>>
>>> + result.Add ((current.Operand
>>> as MethodReference).Resolve ());
>>
>> I think the code works, but it seems fragile to populate the list
>> with
>> nulls, especially when the potential breakage does not normally
>> happen.
>
> Changed.
I think there are similar issues with the field lists. In fact I think
there is a bug. Consider a method which uses protected fields from a
base class where the assembly for that class could not be loaded and
the following:
PInvoke(baseField1); // field doesn't resolve
baseField2 = this.AMethodWhichThrows; // field doesn't resolve
Then as far as I can tell the rule will have a false positive. The fix
is to simply not resolve the fields: the code seems like it will work
fine with FieldReferences instead.
Here are a few more notes:
1) This is just something I have found useful so feel free to ignore
it, but when figuring out what a method rule is doing it's very useful
to print the disassembled method and the offsets of instructions of
note, e.g.
Log.WriteLine (this, "\n\nChecking method: {0} on type: {1}",
method.Name, method.DeclaringType.FullName);
Log.WriteLine (this, method);
Log.WriteLine (this, " Reached a field variable store at {0:X4}, there
are {1} unsafe pointers here.", ins.Offset, pointers == null ? 0 :
pointers.Count);
This can be a bit noisy, but it does show you exactly what is going on.
2) The code no longer needs to check for null field_stores (and
probably some of the other collections).
3) The initialization of the fields_loads collection is rather
unconventional. I haven't examined it closely enough to try to see if
there are problems, but the unit tests seem to poorly test it which
scares me.
-- Jesse
Rolf Bjarne Kvinge
On Fri, Feb 6, 2009 at 2:38 AM, Jesse Jones <jesj...@mindspring.com> wrote:
>
>> You mean checking delegates passed to System.Thread's constructor?
>> This could be done, the verification logic would be exactly the same.
>
> Yeah, and thread pool and thread timers as well if they have the same
> issue (async methods don't). I think the logic would be slightly
> different though: an explicit throw from a catch block should either
> be OK or be at a low confidence if the developer wants to simply throw
> up his hands and allow the app to abort.
>>
>> However I'm not sure how to name such a rule, nor where it should live
>> (Gendarme.Rules.Interoperability would be wrong for this).
>
> I dunno, but they are pretty similar scenarios...
I couldn't find anything on MSDN about thread timers, however the
thread pools do have this issue too.
I'll add checks for these methods at a later stage, to summarize the
rule would check delegates passed to the following methods (in
addition to pinvoke methods):
System.Threading.Thread.ctor
System.Threading.ThreadPool.QueueUserWorkItem
System.Threading.ThreadPool.RegisterWaitForSingleObject
System.Threading.ThreadPool.UnsafeQueueUserWorkItem
System.Threading.ThreadPool.UnsafeRegisterWaitForSingleObject
and I'll have to test the thread timers to see what happens there.
And I agree explicit throws in these delegates should be low confidence.
>>>
>>> It would be really nice to handle throws in the catch blocks. People
>>> do that a lot so it's a fairly serious hole in the rule.
>>
>> Logic has been changed as noted in the beginning of the mail.
>
> Sweet. How do the system assemblies look? Are there any false positives?
For all 2.0 system assemblies there are 2 failures in System.Net.dll
and 4 in System.Windows.Forms.dll, all valid.
> I think there are similar issues with the field lists. In fact I think
> there is a bug. Consider a method which uses protected fields from a
> base class where the assembly for that class could not be loaded and
> the following:
>
> PInvoke(baseField1); // field doesn't resolve
>
> baseField2 = this.AMethodWhichThrows; // field doesn't resolve
>
> Then as far as I can tell the rule will have a false positive. The fix
> is to simply not resolve the fields: the code seems like it will work
> fine with FieldReferences instead.
You're right about the issue with unresolvable field references, I've
changed it to not consider unresolvable fields at all.
Using field references instead of definitions will break if an unsafe
delegate is stored into a field reference, and the field definition is
then used when calling the pinvoke.
>
> Here are a few more notes:
>
> 1) This is just something I have found useful so feel free to ignore
> it, but when figuring out what a method rule is doing it's very useful
> to print the disassembled method and the offsets of instructions of
> note, e.g.
>
> Log.WriteLine (this, "\n\nChecking method: {0} on type: {1}",
> method.Name, method.DeclaringType.FullName);
> Log.WriteLine (this, method);
>
> Log.WriteLine (this, " Reached a field variable store at {0:X4}, there
> are {1} unsafe pointers here.", ins.Offset, pointers == null ? 0 :
> pointers.Count);
>
> This can be a bit noisy, but it does show you exactly what is going on.
Thanks, I'll have it in mind!
>
> 2) The code no longer needs to check for null field_stores (and
> probably some of the other collections).
Removed the redundant null checks I found.
> 3) The initialization of the fields_loads collection is rather
> unconventional. I haven't examined it closely enough to try to see if
> there are problems, but the unit tests seem to poorly test it which
> scares me.
I'm not quite sure what you mean here, it is initialized at class
level. (Note that I renamed it to field_loads, a more correct name).
Thanks!
Rolf
Jesse Jones
On Feb 6, 2009, at 5:59 AM, Rolf Bjarne Kvinge wrote:
>>>> I think there are similar issues with the field lists. In fact I
>>>> think
>> there is a bug. Consider a method which uses protected fields from a
>> base class where the assembly for that class could not be loaded and
>> the following:
>>
>> PInvoke(baseField1); // field doesn't resolve
>>
>> baseField2 = this.AMethodWhichThrows; // field doesn't resolve
>>
>> Then as far as I can tell the rule will have a false positive. The
>> fix
>> is to simply not resolve the fields: the code seems like it will work
>> fine with FieldReferences instead.
>
> You're right about the issue with unresolvable field references, I've
> changed it to not consider unresolvable fields at all.
>
> Using field references instead of definitions will break if an unsafe
> delegate is stored into a field reference, and the field definition is
> then used when calling the pinvoke.
Bah, I hate that identity versus equality distinction...
>
>
>> 3) The initialization of the fields_loads collection is rather
>> unconventional. I haven't examined it closely enough to try to see if
>> there are problems, but the unit tests seem to poorly test it which
>> scares me.
>
> I'm not quite sure what you mean here, it is initialized at class
> level. (Note that I renamed it to field_loads, a more correct name).
What I mean is that you cannot simply index backwards n times to find
the instruction that loaded the nth method argument. Most rules handle
this with the Traceback instruction rock. This rule instead seems to
manually track pushes and pops, but as far as I can tell there is no
unit test to check that it does so correctly for any but the most
trivial case.
-- Jesse
Rolf Bjarne Kvinge
> What I mean is that you cannot simply index backwards n times to find
> the instruction that loaded the nth method argument. Most rules handle
> this with the Traceback instruction rock. This rule instead seems to
> manually track pushes and pops, but as far as I can tell there is no
> unit test to check that it does so correctly for any but the most
> trivial case.
Ah, you mean the list variable for the stack.
The problem with the TraceBack instruction rock is that this rule
needs to know where an expression starts and ends, and TraceBack only
says where an expression starts. Of course more tests wouldn't hurt,
not quite sure how to make it comprehensive though. One issue is that
this is sort of language/compiler dependent, gmcs doesn't generate the
same il sequences as csc, and vb/boo/etc differ even more.
Rolf
Rolf Bjarne Kvinge
I've also added #if DEBUG instead of using Log.IsEnabled in a few
places (these are the only changes).
Rolf
Jesse Jones
I don't think you need a comprehensive test, but there should be least
one that isn't simply stepping back N instructions for the Nth argument.
-- Jesse
Sebastien Pouliot
Sorry for the delay in replying.
A few generalities (I'll update the doc asap to make sure it's stated)
* The wiki documentation is generated from the rule class XML comments
(it's still not perfectly automated but it's totally worth it ;-). This
also means that it should be as complete as possible (so you need to get
back your wiki stuff before I push the 2.4 docs in).
* New rules should be Gendarme-friendly, i.e. they should obey the
existing rules. We have a make target for this (make self-test). This
catch a lot of smaller problems and it's a great dog-fooding.
* Before committing I run Gendarme (and all rules) against many projects
- the binaries selected differs by contributors (e.g. boo ;-) but
everyone should run new rules against Mono 2.0 class libraries. This
catch a lot of potential exceptions.
A few comments inline. I need to think a bit more about the rule as I
think it's a bit heavy and I'm not yet sure if it could be done
differently (less heavy) or if the Gendarme framework/design lacks
something to make it easier (or both). That may take a while (of forever
if the rule does not show on any profiling results ;-) so don't wait for
them ;-)
>
>
> //
...
> namespace Gendarme.Rules.Interoperability {
>
> /// <summary>
> /// <code>Every delegate which is passed to native code must include an exception block which spans the entire method and has a catch all block.</code>
No <code> in summary (unless it around real source code or keywords).
> /// </summary>
> /// <example>
> /// Bad example:
> /// <code>
> /// </code>
> /// </example>
> /// <example>
> /// Good example:
> /// <code>
> /// try {
> /// }
> /// }
> /// </code>
> /// </example>
>
> [Problem ("Every delegate passed to native code must include an exception block which spans the entire method and has a catch all block.")]
This reminds me we have a rule against "catch all" so we need to update
its documentation to deal with native callbacks.
> [Solution ("Surround the entire method body with a try/catch block.")]
> public sealed class DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule : Rule, IMethodRule {
> /// <summary>
The current "xml2wiki" does not deal well with XML inside types. Also at
some (not-too-future) point we'll use XML doc to document configurable
properties. Please use normal // comments everywhere inside the source.
> Dictionary<MethodDefinition, bool> verified_methods = new Dictionary<MethodDefinition, bool> ();
>
> /// <summary>
> List<MethodDefinition> reported_methods = new List<MethodDefinition> ();
>
> /// <summary>
> List<FieldDefinition> field_loads = new List<FieldDefinition> ();
>
> /// <summary>
> Dictionary<FieldDefinition, List<MethodDefinition>> field_stores = new Dictionary<FieldDefinition, List<MethodDefinition>> ();
>
> /// <summary>
> /// A list of all the locals in a method.
> /// Have one class-level instance which is emptied before checking a method.
> /// Avoids creating a new list instance for each method to check.
> /// </summary>
>
> /// A list of ilranges a stack position corresponds to.
> /// Avoids creating a new list instance for each method to check.
> /// </summary>
> /// <summary>
> /// A bitmask of safe instructions.
> /// </summary>
> static OpCodeBitmask safe_instructions;
>
> /// <summary>
> /// A bitmask of opcodes a method must have to be checked in CheckMethod.
> /// </summary>
> static OpCodeBitmask store_field_bitmask;
>
> /* IGNORE THIS
> * static class Log
> * {
> * public static bool IsEnabled (object o) { return true; }
> * public static void WriteLine (object o, string msg, params object [] args)
> * {
> * Console.WriteLine (msg, args);
> * }
> * public static void WriteLine (object o, MethodDefinition m)
> * {
> * //Gendarme.Framework.Helpers.Log.WriteLine (o, m);
> * Console.WriteLine (new MethodPrinter (m).ToString ());
> * }
> * }
> */
> public RuleResult CheckMethod (MethodDefinition method)
> MethodDefinition called_method;
> List<MethodDefinition> pointers;
> // Rule does not apply if the method has no IL
> return RuleResult.DoesNotApply;
>
> //
> // We need to check all methods which has any of the following opcodes: call, stfld or stsfld.
> //
> if (store_field_bitmask == null) {
> store_field_bitmask = new OpCodeBitmask ();
> store_field_bitmask.Set (Code.Stfld);
> store_field_bitmask.Set (Code.Stsfld);
Do not initialize static fields inside instance methods. The pattern we
use is to use the "int"s ctor of bitmask and to have a #ifdef out method
to (re-)compute them.
> }
> OpCodeBitmask bitmask = OpCodeEngine.GetBitmask (method);
> if (!OpCodeBitmask.Calls.Intersect (bitmask) && !store_field_bitmask.Intersect (bitmask))
> return RuleResult.DoesNotApply;
>
>
> locals.Clear ();
> stack.Clear ();
> Log.WriteLine (this, "\n\nChecking method: {0} on type: {1}", method.Name, method.DeclaringType.FullName);
> Log.WriteLine (this, method);
>
> foreach (ExceptionHandler e in method.Body.ExceptionHandlers)
> Log.WriteLine (this, " Type: {7}, TryStart: {4:X}, TryEnd: {5:X}, HandlerStart: {0:X}, HandlerEnd: {1:X}, FilterStart: {2:X}, FilterEnd: {3:X}, CatchType: {6}",
> e.HandlerStart.GetOffset (), e.HandlerEnd.GetOffset (), e.FilterStart.GetOffset (), e.FilterEnd.GetOffset (),
> e.TryStart.GetOffset (), e.TryEnd.GetOffset (), e.CatchType, e.Type);
> #endif
>
> //
> // v = UnsafeMethod;
> //
> //
> //
>
> int stack_count = 0;
>
> // leave or leave.s, they leave the stack empty.
> stack_count = 0;
> stack.Clear ();
> continue; // No need to do anything else here.
> }
>
> if (stack_count == 0) {
> foreach (ExceptionHandler eh in method.Body.ExceptionHandlers) {
> if (eh.HandlerStart != null && eh.HandlerStart.Offset == ins.Offset) {
> // upon entry to a catch handler there is an implicit object on the stack already (the thrown object)
> stack_count = 1;
> stack.Add (null);
> break;
> }
> }
> }
>
> Log.WriteLine (this, " {0:X} {5} prev stack: {1}, pop: {2}, push: {3}, post stack: {4}", ins.Offset, stack_count, pop, push, stack_count + push - pop, ins.OpCode.Name);
>
> if (stack_count == 1 && stack [stack_count - 1] == null) {
> // Don't do anything, this is the implicit exception object passed to a catch handler.
> } else if (ins.OpCode.Code == Code.Call) {
>
> for (int i = 0; i < called_method.Parameters.Count; i++) {
> continue;
> if (!called_method.Parameters [i].ParameterType.IsDelegate ())
>
> Log.WriteLine (this, " Parameter #{0} takes a delegate, stack expression:", i);
>
> field_loads.Add (field);
>
> // Get and check the pointers
> }
> }
>
>
> stack [stack_count - 1].DumpAll (" ");
> if (pointers != null && pointers.Count > 0) {
> }
> tmp.AddRange (pointers);
>
> // If this field has been loaded into a pinvoke,
> VerifyMethods (pointers);
> } else if (ins.IsStoreLocal ()) {
>
>
> if (pointers != null && pointers.Count > 0) {
> locals.Add (null);
> }
>
> stack_count += push - pop;
>
> stack.Add (new ILRange (ins));
> while (stack_count < stack.Count)
> stack.RemoveAt (stack.Count - 1);
>
> if (stack_count > 0 && stack [stack_count - 1] != null)
> stack [stack_count - 1].Last = ins;
> }
>
> Log.WriteLine (this, "Checking method: {0} [Done]", method.Name);
>
> return Runner.CurrentRuleResult;
> }
>
> /// <summary>
> bool result;
> bool valid_ex_handler;
> bool [] safe;
> MethodBody body;
> InstructionCollection instructions;
>
> if (callback == null)
> return true;
>
> if (!callback.HasBody)
> return true;
>
> if (verified_methods.TryGetValue (callback, out result))
> return result;
>
> Log.WriteLine (this, " Verifying: {0} with code size: {1}", callback.Name, callback.Body.CodeSize);
>
> body = callback.Body;
> instructions = body.Instructions;
> safe = new bool [body.Instructions.Count]; // all instructions in the method start out as unsafe
That's a potentially huge allocation being done multiple times. It could
be reused (and cleared or reallocated/extended when needed).
(yes I'm an allocation freak since 50% of the time gains we made in the
last year comes from reducing allocations, the second half is by being
wiser about when to execute, or not, a rule)
> //
> // We assume that the method is verifiable.
> //
>
> if (safe_instructions == null) {
> // a list of safe instructions, which under no circumstances
> // (for verifiable code) can cause an exception to be raised.
> safe_instructions = new OpCodeBitmask ();
> safe_instructions.Set (Code.Nop);
> safe_instructions.Set (Code.Ret);
> safe_instructions.Set (Code.Ldloc);
> safe_instructions.Set (Code.Ldloc_0);
> safe_instructions.Set (Code.Ldloc_1);
> safe_instructions.Set (Code.Ldloc_2);
> safe_instructions.Set (Code.Ldloca);
> safe_instructions.Set (Code.Ldloca_S);
> // safe_instructions.Set (Code.Ldsfld); // Not quite sure about these two, leaving them out for now.
> // safe_instructions.Set (Code.Ldsflda); //
> safe_instructions.Set (Code.Leave);
> safe_instructions.Set (Code.Endfilter);
> safe_instructions.Set (Code.Endfinally);
> safe_instructions.Set (Code.Ldc_I4);
> safe_instructions.Set (Code.Ldc_I4_0);
> safe_instructions.Set (Code.Ldc_I4_1);
> safe_instructions.Set (Code.Ldc_I4_2);
> safe_instructions.Set (Code.Ldc_I4_3);
> safe_instructions.Set (Code.Ldc_I4_4);
> safe_instructions.Set (Code.Ldc_I4_5);
> safe_instructions.Set (Code.Ldc_I4_6);
> safe_instructions.Set (Code.Ldc_I4_7);
> safe_instructions.Set (Code.Ldc_I4_8);
> safe_instructions.Set (Code.Ldc_I4_M1);
> safe_instructions.Set (Code.Ldc_I8);
> safe_instructions.Set (Code.Ldc_R4);
> safe_instructions.Set (Code.Ldc_R8);
> safe_instructions.Set (Code.Ldarg);
> safe_instructions.Set (Code.Ldarg_0);
> safe_instructions.Set (Code.Ldarg_1);
> safe_instructions.Set (Code.Ldarg_2);
> safe_instructions.Set (Code.Ldarg_3);
> safe_instructions.Set (Code.Ldarg_S);
> safe_instructions.Set (Code.Stloc);
> safe_instructions.Set (Code.Stloc_0);
> safe_instructions.Set (Code.Stloc_1);
> safe_instructions.Set (Code.Stloc_2);
> safe_instructions.Set (Code.Stloc_3);
> safe_instructions.Set (Code.Ldnull);
> safe_instructions.Set (Code.Initobj);
> safe_instructions.Set (Code.Pop);
> // The stind* instructions can raise an exception:
> // "NullReferenceException is thrown if addr is not naturally aligned for the argument type implied by the instruction suffix."
> // Not sure how we can verify that the alignment is correct, and this instruction is emitted by the compiler for byref/out parameters.
> // Not marking this instructions as safe would mean that it would be impossible to fix a delegate that takes
> // a byref/out parameter.
> safe_instructions.Set (Code.Stind_I);
> safe_instructions.Set (Code.Stind_I1);
> safe_instructions.Set (Code.Stind_I2);
> safe_instructions.Set (Code.Stind_I4);
> safe_instructions.Set (Code.Stind_I8);
> safe_instructions.Set (Code.Stind_R4);
> safe_instructions.Set (Code.Stind_R8);
> safe_instructions.Set (Code.Stind_Ref);
same comment about initialization
> }
>
> // Mark all instructions corresponding to a safe opcode as safe.
> for (int i = 0; i < instructions.Count; i++) {
> if (safe_instructions.Get (instructions [i].OpCode.Code))
> safe [i] = true;
> }
>
>
> //
> // Mark code handled by a catch all block as safe.
> //
> // Catch all block is any of the following:
> // a) A try block which does not specify an exception type
> // b) A try block which specifies System.Exception as the exception type.
> //
> // The second case will break if both of the following happens:
> // 1) An exception is thrown which does not inherit from System.Exception (not valid in C# nor VB, but it is valid in C++/CIL and IL)
> // 2) The assembly where the exception handler resides has the System.Runtime.CompilerServices.RuntimeCompatibility attribute set with WrapNonExceptionThrows = true.
> //
> // If 2) is not true, the runtime will wrap the exception in a RuntimeWrappedException object, which is handled by case b) above.
> // Given that this is the normal case (otherwise you'd have to put the attribute in the assembly), we accept 2) as safe too.
> //
>
> if (callback.Body.HasExceptionHandlers) {
> continue;
> // no 'Catch ... When <condition>' clause. C# doesn't support it, VB does
>
> // check for catch all clauses
> if (!(eh.CatchType == null || eh.CatchType.FullName == "System.Object" || eh.CatchType.FullName == "System.Exception"))
> continue;
>
> // Mark the code this exception handler handles as safe.
> int start_index = instructions.IndexOf (eh.TryStart);
> int end_index = instructions.IndexOf (eh.TryEnd);
> Log.WriteLine (this, " Catch all block found, marking instruction at index {0} to index {1} (included) as safe.", start_index, end_index - 1);
> for (int j = start_index; j < end_index; j++)
> safe [j] = true;
> }
> }
>
> // Check that all instructions have been marked as safe, otherwise mark the method as unsafe.
> valid_ex_handler = true;
> for (int i = 0; i < safe.Length; i++) {
> if (!safe [i]) {
> valid_ex_handler = false;
> break;
> }
> }
>
> #if DEBUG
> Log.WriteLine (this, " Method {0} verified: {1}.", callback.Name, valid_ex_handler);
> for (int i = 0; i < safe.Length; i++) {
> // Console.ForegroundColor = safe [i] ? ConsoleColor.DarkGreen : ConsoleColor.Red;
> Log.WriteLine (this, " {1} {0}", instructions [i].ToPrettyString (), safe [i] ? "Y" : "N");
> // Console.ResetColor ();
> }
> foreach (ExceptionHandler e in body.ExceptionHandlers)
> Log.WriteLine (this, " Type: {7}, TryStart: {4}, TryEnd: {5}, HandlerStart: {0}, HandlerEnd: {1}, FilterStart: {2}, FilterEnd: {3}, CatchType: {6}",
> e.HandlerStart.GetOffset (), e.HandlerEnd.GetOffset (), e.FilterStart.GetOffset (), e.FilterEnd.GetOffset (),
> e.TryStart.GetOffset (), e.TryEnd.GetOffset (), e.CatchType, e.Type);
> #endif
>
> verified_methods.Add (callback, valid_ex_handler);
>
> return valid_ex_handler;
> }
>
> private class ILRange {
should be sealed (if only to please 'self-test')
> public Instruction First;
> public Instruction Last;
> public ILRange (Instruction first)
> {
> First = first;
> }
> [Conditional ("DEBUG")]
> if (!Log.IsEnabled (this.GetType ().Name))
> return;
>
> Instruction instr = First;
> do {
> Log.WriteLine (this, "{0}{1}", prefix, instr.ToPrettyString ());
> if (instr == Last)
> break;
> instr = instr.Next;
> } while (true);
> }
> }
>
> /// <summary>
> {
> MethodDefinition ldftn_definition;
> // Check if the code does any ldftn.
> ldftn = range.Last.Previous.Operand as MethodReference;
> if (ldftn != null) {
> ldftn_definition = ldftn.Resolve ();
> if (ldftn_definition != null) {
> if (result == null)
> }
> }
>
> // Check if the code loads any local variables which can be delegates.
> }
> }
>
> // If the last opcode is a field load, check if any pointers have been stored in that field.
> FieldReference field = range.Last.Operand as FieldReference;
> FieldDefinition field_definition;
> List<MethodDefinition> pointers;
> if (field != null) {
> field_definition = (range.Last.Operand as FieldReference).Resolve ();
> if (field_definition != null && field_stores.TryGetValue (field_definition, out pointers)) {
> if (result == null)
> }
> }
> }
>
> return result;
> }
>
> /// <summary>
> /// </summary>
> Log.WriteLine (this, " Verifying {0} method pointers.", pointers == null ? 0 : pointers.Count);
>
> if (pointers == null)
> return;
> foreach (MethodDefinition pointer in pointers)
> }
>
> /// <summary>
> if (!safe) {
> if (reported_methods.Contains (pointer))
> return;
>
> reported_methods.Add (pointer);
>
> Log.WriteLine (this, " Reporting: {0}", pointer.Name);
> Runner.Report (pointer, Severity.High, Confidence.High);
> } else {
> Log.WriteLine (this, " Safe: {0}", pointer.Name);
> }
> }
> }
>
> internal static class DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRuleHelper
> {
> if (instr == null)
> return "<nil>";
> if (instr != null)
> return instr.Offset;
> return -1;
> }
This method is only used inside #if DEBUG code (and should be inside
that condition too)
>
> /// <summary>
> switch (ins.OpCode.Code) {
> case Code.Ldloc_0: return 0;
> case Code.Ldloc_1: return 1;
> case Code.Ldloc_2: return 2;
> case Code.Ldloc_3: return 3;
> case Code.Ldloc: // Untested
> case Code.Ldloca: // Untested
> case Code.Ldloca_S:
Rules should not throw exceptions - but this code should probably be
elsewhere. In any case Exception should no be thrown (too basic) and
this is clearly an ArgumentException (even if this lose a bit of it's
meaning in extension methods).
> }
> }
>
> /// <summary>
> switch (ins.OpCode.Code) {
> case Code.Stloc_0: return 0;
> case Code.Stloc_1: return 1;
> case Code.Stloc_2: return 2;
> case Code.Stloc_3: return 3;
same
> }
> }
> }
> }
About the unit tests, can you explain how/why you did them that way ?
Our addition to nunit have proven to be big time (and space) saver but
it can surely be improved - and this would be more helpful than having
different customized extension on several rules.
Thanks!
Sebastien
Jesse Jones
(like the one that comes with mono 2.2) because it thinks methods
named Test* are test methods. This causes all kinds of badness
including trying to load the library.dll. I've attached a patch which
fixes this.
-- Jesse
Jesse Jones
using System;
using System.Reflection;
using System.Runtime.InteropServices;
[assembly: AssemblyVersion("0.4.84.0")]
[assembly: CLSCompliant(false)]
[assembly: ComVisible(false)]
namespace FTest
{
public static class NativeDelegateTest
{
public static void Work(Action<int> action)
{
Exception exception = null;
Callback callback = (int x) =>
{
try
{
action(x);
}
catch (Exception e)
{
exception = e;
}
};
PInvoke(callback);
if (exception != null)
Console.Error.WriteLine("{0}", exception);
}
private delegate void Callback(int x);
[DllImport("lib")]
private static extern void PInvoke(Callback callback);
}
}
-- Jesse
Jesse Jones
Index: rules/Gendarme.Rules.Interoperability/
DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule.cs
===================================================================
--- rules/Gendarme.Rules.Interoperability/
DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule.cs
(revision 126984)
+++ rules/Gendarme.Rules.Interoperability/
DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule.cs
(working copy)
@@ -449,6 +449,9 @@
case Code.Ldloc_3: return 3;
case Code.Ldloc: // Untested for ldloc
case Code.Ldloc_S: return ((VariableDefinition) ins.Operand).Index;
+ case Code.Ldloca: // we don't care about ldloca
+ case Code.Ldloca_S: return int.MaxValue;
+
default:
throw new Exception (string.Format ("Invalid opcode: {0}",
ins.OpCode.Name));
}
Index: rules/Gendarme.Rules.Interoperability/ChangeLog
===================================================================
--- rules/Gendarme.Rules.Interoperability/ChangeLog (revision 126984)
+++ rules/Gendarme.Rules.Interoperability/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2009-01-15 Jesse Jones <jesj...@mindspring.com>
+
+ * DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule.cs:
+ Fix for ldloca instructions.
+
2008-11-30 Sebastien Pouliot <seba...@ximian.com>
* MarshalBooleansInPInvokeDeclarationsRule.cs: Use HasParameters
-- Jesse
Rolf Bjarne Kvinge
I guess the way to detect it as safe would be to assume that a stfld
to 'this' is safe and handle it that way.
Rolf
Jesse Jones
It
1) Fixes the unit tests so that they run correctly with old versions
of nunit (like the one that ships with mono 2.2).
2) Includes a minor fix that prevents the rule from throwing an
exception when processing mscorlib.
-- Jesse
Sebastien Pouliot
Please commit them.
Thanks
Sebastien
Jesse Jones
r127954
-- Jesse
Rolf Bjarne Kvinge
> On Fri, Feb 13, 2009 at 4:54 AM, Sebastien Pouliot <sebastie...@gmail.com> wrote:
>
> Hello Rolf,
>
> Sorry for the delay in replying.
>
> A few generalities (I'll update the doc asap to make sure it's stated)
>
> * The wiki documentation is generated from the rule class XML comments
> (it's still not perfectly automated but it's totally worth it ;-). This
> also means that it should be as complete as possible (so you need to get
> back your wiki stuff before I push the 2.4 docs in).
>
> * New rules should be Gendarme-friendly, i.e. they should obey the
> existing rules. We have a make target for this (make self-test). This
> catch a lot of smaller problems and it's a great dog-fooding.
console, now that I've found the html report I have fixed most of the
issues which are reported, except for:
AvoidLackOfCohesionOfMethodsRule : Type cohesiveness : 31%. I don't
really have much idea how to fix this (split the class down into tiny
subclasses?)
AvoidCodeDuplicatedInSameClassRule : Duplicated code in GetLoadIndex
and GetStoreIndex (I can't see how this can be fixed in a way that
results in better code - I can put everything into a GetIndex method,
but imho that's not a good solution).
How can I fix these?
>* Before committing I run Gendarme (and all rules) against many projects
>- the binaries selected differs by contributors (e.g. boo ;-) but
>everyone should run new rules against Mono 2.0 class libraries. This
>catch a lot of potential exceptions.
obviously I did something wrong since corlib was throwing an
exception. Anyways, I re-ran the rule against all dlls and exes I
could find (874), and everything it reported seemed valid (basically a
few error in almost every *-sharp.dll assembly, these are wrappers
around native libs and use callbacks heavily + 4 in
System.Windows.Forms.dll + a few in moonlight's System.Windows.dll
which I'll fix asap ;-) ).
> A few comments inline. I need to think a bit more about the rule as I
> think it's a bit heavy and I'm not yet sure if it could be done
> differently (less heavy) or if the Gendarme framework/design lacks
> something to make it easier (or both). That may take a while (of forever
> if the rule does not show on any profiling results ;-) so don't wait for
> them ;-)
>
> No <code> in summary (unless it around real source code or keywords).
into the wiki.
>> [Solution ("Surround the entire method body with a try/catch block.")]
>> [EngineDependency (typeof (OpCodeEngine))]
>> public sealed class DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule : Rule, IMethodRule {
>> /// <summary>
>> /// A list of methods which have been verified to be safe to call from native code.
>> /// </summary>
>
>The current "xml2wiki" does not deal well with XML inside types. Also at
>some (not-too-future) point we'll use XML doc to document configurable
>properties. Please use normal // comments everywhere inside the source.
>> List<ILRange> stack = new List<ILRange> ();
>
>That's *a lot* of data!
>> if (store_field_bitmask == null) {
>> store_field_bitmask = new OpCodeBitmask ();
>> store_field_bitmask.Set (Code.Stfld);
>> store_field_bitmask.Set (Code.Stsfld);
>
>Do not initialize static fields inside instance methods. The pattern we
>use is to use the "int"s ctor of bitmask and to have a #ifdef out method
>to (re-)compute them.
>> safe = new bool [body.Instructions.Count]; // all instructions in the method start out as unsafe
>
>That's a potentially huge allocation being done multiple times. It could
>be reused (and cleared or reallocated/extended when needed).
>
>(yes I'm an allocation freak since 50% of the time gains we made in the
>last year comes from reducing allocations, the second half is by being
>wiser about when to execute, or not, a rule)
>> public static int GetLoadIndex (this Instruction ins)
>> {
>> switch (ins.OpCode.Code) {
>> case Code.Ldloc_0: return 0;
>> case Code.Ldloc_1: return 1;
>> case Code.Ldloc_2: return 2;
>> case Code.Ldloc_3: return 3;
>> case Code.Ldloc: // Untested
>> case Code.Ldloca: // Untested
>> case Code.Ldloca_S:
>> case Code.Ldloc_S: return ((VariableDefinition) ins.Operand).Index;
>> default:
>> throw new Exception (string.Format ("Invalid opcode: {0}", ins.OpCode.Name));
>
>Rules should not throw exceptions - but this code should probably be
>elsewhere. In any case Exception should no be thrown (too basic) and
>this is clearly an ArgumentException (even if this lose a bit of it's
>meaning in extension methods).
if you think that's even better.
> About the unit tests, can you explain how/why you did them that way ?
>
>Our addition to nunit have proven to be big time (and space) saver but
>it can surely be improved - and this would be more helpful than having
>different customized extension on several rules.
For AssertTest the issue is that each test needs a new rule instance -
unsafe methods are only reported once, and since the unsafe methods in
the test are reused between tests, reusing the rule instance would
cause tests that should fail to pass. AssertClass has a logic I didn't
find anywhere: for all methods in a class exactly one defect should be
found (in total).
(I also removed a few methods which I weren't used anymore which may
have confused you).
I'm attaching the updated files again.
Thanks!
Rolf
Rolf Bjarne Kvinge
<rolfb...@gmail.com> wrote:
> This code isn't as simple as it looks in C#.
>
> I guess the way to detect it as safe would be to assume that a stfld
> to 'this' is safe and handle it that way.
the method was called - callvirt does a nullcheck when calling, while
call doesn't (VB and C# compilers will always insert a callvirt when
an instance method is called, so this isn't necessarily true. This
means that the following code snipped in pseudo C# would not be safe,
a NRE would be thrown on the line in M and not when M is called.
class C {
object exception;
static void S () {
C value = null;
value.M (); // using call instead of callvirt
}
void M ()
{
exception = "some object here";
}
}
This is really a corner case though, there are other (easier) ways to
fool this rule if you want to do it on purpose. I'll see if I can make
your sample pass.
>> You received this message because you are subscribed to the Google Groups "Gendarme" group.
>> To post to this group, send email to gend...@googlegroups.com
>> To unsubscribe from this group, send email to gendarme+u...@googlegroups.com
>> For more options, visit this group at http://groups.google.com/group/gendarme?hl=en
>> -~----------~----~----~----~------~----~------~--~---
>>
>>
>
Sebastien Pouliot
> Hi Sebastien
>
> > On Fri, Feb 13, 2009 at 4:54 AM, Sebastien Pouliot <sebastie...@gmail.com> wrote:
> >
> > Hello Rolf,
> >
> > Sorry for the delay in replying.
>
> My time to say sorry for the delay now ;-)
...
> > * New rules should be Gendarme-friendly, i.e. they should obey the
> > existing rules. We have a make target for this (make self-test). This
> > catch a lot of smaller problems and it's a great dog-fooding.
>
> At the time I was fooled by the fact that nothing was written to the
> console, now that I've found the html report I have fixed most of the
> issues which are reported, except for:
> AvoidLackOfCohesionOfMethodsRule : Type cohesiveness : 31%. I don't
> really have much idea how to fix this (split the class down into tiny
> subclasses?)
> AvoidCodeDuplicatedInSameClassRule : Duplicated code in GetLoadIndex
> and GetStoreIndex (I can't see how this can be fixed in a way that
> results in better code - I can put everything into a GetIndex method,
> but imho that's not a good solution).
>
> How can I fix these?
than "I must fix it!".
Most of the time they return valid results. However valid the results
are it does not mean it can be fixed (or that the fix would not be worse
than the original code).
> >* Before committing I run Gendarme (and all rules) against many projects
> >- the binaries selected differs by contributors (e.g. boo ;-) but
> >everyone should run new rules against Mono 2.0 class libraries. This
> >catch a lot of potential exceptions.
>
> I'm pretty sure I did run against my mono's class libraries, but
> obviously I did something wrong since corlib was throwing an
> exception. Anyways, I re-ran the rule against all dlls and exes I
> could find (874), and everything it reported seemed valid (basically a
> few error in almost every *-sharp.dll assembly, these are wrappers
> around native libs and use callbacks heavily + 4 in
> System.Windows.Forms.dll + a few in moonlight's System.Windows.dll
> which I'll fix asap ;-) ).
...
> >> List<ILRange> stack = new List<ILRange> ();
> >
> >That's *a lot* of data!
>
> Not sure if this is something to fix?
Some rules, not sure about this one*, can be split (fairly new so it's
not much used) to (a) gather then (b) report (at TearDown). This can
avoid retaining partial data for a long time.
* this rule does not have the common pattern we're trying to
avoid (i.e. another loop on assemblies/modules/types/methods or
a subset of them)
...
> >> public static int GetLoadIndex (this Instruction ins)
> >> {
> >> switch (ins.OpCode.Code) {
> >> case Code.Ldloc_0: return 0;
> >> case Code.Ldloc_1: return 1;
> >> case Code.Ldloc_2: return 2;
> >> case Code.Ldloc_3: return 3;
> >> case Code.Ldloc: // Untested
> >> case Code.Ldloca: // Untested
> >> case Code.Ldloca_S:
> >> case Code.Ldloc_S: return ((VariableDefinition) ins.Operand).Index;
> >> default:
> >> throw new Exception (string.Format ("Invalid opcode: {0}", ins.OpCode.Name));
> >
> >Rules should not throw exceptions - but this code should probably be
> >elsewhere. In any case Exception should no be thrown (too basic) and
> >this is clearly an ArgumentException (even if this lose a bit of it's
> >meaning in extension methods).
>
> Fixed (to throw ArgumentException). I can move it to InstructionRocks
> if you think that's even better.
> > About the unit tests, can you explain how/why you did them that way ?
> >
> >Our addition to nunit have proven to be big time (and space) saver but
> >it can surely be improved - and this would be more helpful than having
> >different customized extension on several rules.
>
> I assume you refer to the custom AssertTest and AssertClass methods.
> For AssertTest the issue is that each test needs a new rule instance -
> unsafe methods are only reported once, and since the unsafe methods in
> the test are reused between tests, reusing the rule instance would
> cause tests that should fail to pass.
tests do that. Not a big deal.
> AssertClass has a logic I didn't
> find anywhere: for all methods in a class exactly one defect should be
> found (in total).
Anyway I asked more because I'm curious how to improve the test code
while minimizing diversity to achieve the same results.
> (I also removed a few methods which I weren't used anymore which may
> have confused you).
>
> I'm attaching the updated files again.
you have it checked-out otherwise I can do it later). I don't expect
I'll have time to do some performance work for 2.6 but I might ping you,
if I have ideas/questions, for 2.8 :-)
Thanks
Sebastien
p.s. please add yourself to the AUTHORS list
> //
> // Gendarme.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule
> //
> // Authors:
> // Rolf Bjarne Kvinge <RKv...@novell.com>
> //
> //
>
> using System;
> using System.Collections.Generic;
>
> using Mono.Cecil;
> using Mono.Cecil.Cil;
>
> using Gendarme.Framework;
> using Gendarme.Framework.Engines;
> using Gendarme.Framework.Helpers;
> using Gendarme.Framework.Rocks;
>
> namespace Gendarme.Rules.Interoperability {
>
> /// <summary>
> /// This rule checks for delegates which are created for methods which don't have exception handling and then passed to native code.
> /// Every delegate which is passed to native code must include an exception block which spans the entire method and has a catch all block
> /// <example>
> /// Bad example:
> /// <code>
> /// [DllImport ("mylibrary.dll")]
> /// static extern void RegisterCallback (Callback callback);
> /// public void RegisterManagedCallback ()
> /// {
> /// RegisterCallback (ManagedCallback);
> /// }
> /// public void ManagedCallback ()
> /// {
> /// throw new NotImplementedException (); // This will cause the process to crash if native code calls this method.
I might have to re-edit some of the long lines since they (rarely) look
good on the wiki.
> /// }
> /// </code>
> /// </example>
> /// <example>
> /// Good example:
> /// <code>
> /// [DllImport ("mylibrary.dll")]
> /// static extern void RegisterCallback (Callback callback);
> /// public void RegisterManagedCallback ()
> /// {
> /// RegisterCallback (ManagedCallback);
> /// }
> /// public void ManagedCallback ()
> /// {
> /// try {
> /// throw new NotImplementedException ();
> /// } catch {
> /// // This way the exception won't "leak" to native code
> /// }
> /// try {
> /// throw new NotImplementedException ();
> /// } catch (System.Exception ex) {
> /// // This is also safe - the runtime will process this catch clause, even if it is technically possible
> /// // (by writing assemblies in managed C++ or IL) to throw an exception that doesn't inherit from
> /// // System.Exception.
> /// }
> /// </code>
> /// </example>
>
> [EngineDependency (typeof (OpCodeEngine))]
> public sealed class DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule : Rule, IMethodRule {
comment for the other List below) but I generally prefer using an
HashSet (honestly I use List<T> every time and only change them to
HashSet after reviewing my code). In any case it might be worth timing
the difference (if only to entertain the list ;-).
>
> // A list of all the fields which have been passed to pinvokes (as a delegate parameter).
> // We report an error if a ld[s]fld stores an unsafe function pointer into any of these fields.
> // A list of all the fields which have been assigned function pointers.
> // We report an error if a pinvoke loads any of these fields.
> // A list of all the locals in a method.
> // Have one class-level instance which is emptied before checking a method.
> // Avoids creating a new list instance for each method to check.
>
>
That reminds me (no change needed) since it's not the only place were it
would be useful, that we should provide a LargeBitmask to complement
Bitmask<T> and OpCodeBitmask. Likely not a very large memory win but...
> // A bitmask of safe instructions.
> // A bitmask of opcodes a method must have to be checked in CheckMethod.
>
> /* IGNORE THIS
> static class Log
> {
> Console.WriteLine (msg, args);
> public static void WriteLine (object o, MethodDefinition m)
> }
> }
>
> #if false
> static DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRule ()
> {
> GenerateBitmasks ();
> }
>
> public static void GenerateBitmasks ()
> {
> OpCodeBitmask bitmask;
>
> bitmask = new OpCodeBitmask ();
> bitmask.Set (Code.Stfld);
> bitmask.Set (Code.Stsfld);
> bitmask.Set (Code.Call);
> bitmask.Set (Code.Callvirt);
> applicable_method_bitmask = bitmask;
> Console.WriteLine ("applicable_method_bitmask: {0}", bitmask);
>
> bitmask = new OpCodeBitmask ();
> // (for verifiable code) can cause an exception to be raised.
> bitmask.Set (Code.Nop);
> bitmask.Set (Code.Ret);
> bitmask.Set (Code.Ldloc);
> bitmask.Set (Code.Ldloc_0);
> bitmask.Set (Code.Ldloc_1);
> bitmask.Set (Code.Ldloc_2);
> bitmask.Set (Code.Ldloca);
> bitmask.Set (Code.Ldloca_S);
> // bitmask.Set (Code.Ldsfld); // Not quite sure about these two, leaving them out for now.
> // bitmask.Set (Code.Ldsflda); //
> bitmask.Set (Code.Leave);
> bitmask.Set (Code.Endfilter);
> bitmask.Set (Code.Endfinally);
> bitmask.Set (Code.Ldc_I4);
> bitmask.Set (Code.Ldc_I4_0);
> bitmask.Set (Code.Ldc_I4_1);
> bitmask.Set (Code.Ldc_I4_2);
> bitmask.Set (Code.Ldc_I4_3);
> bitmask.Set (Code.Ldc_I4_4);
> bitmask.Set (Code.Ldc_I4_5);
> bitmask.Set (Code.Ldc_I4_6);
> bitmask.Set (Code.Ldc_I4_7);
> bitmask.Set (Code.Ldc_I4_8);
> bitmask.Set (Code.Ldc_I4_M1);
> bitmask.Set (Code.Ldc_I8);
> bitmask.Set (Code.Ldc_R4);
> bitmask.Set (Code.Ldc_R8);
> bitmask.Set (Code.Ldarg);
> bitmask.Set (Code.Ldarg_0);
> bitmask.Set (Code.Ldarg_1);
> bitmask.Set (Code.Ldarg_2);
> bitmask.Set (Code.Ldarg_3);
> bitmask.Set (Code.Ldarg_S);
> bitmask.Set (Code.Stloc);
> bitmask.Set (Code.Stloc_0);
> bitmask.Set (Code.Stloc_1);
> bitmask.Set (Code.Stloc_2);
> bitmask.Set (Code.Stloc_3);
> bitmask.Set (Code.Ldnull);
> bitmask.Set (Code.Initobj);
> bitmask.Set (Code.Pop);
> // "NullReferenceException is thrown if addr is not naturally aligned for the argument type implied by the instruction suffix."
> // Not sure how we can verify that the alignment is correct, and this instruction is emitted by the compiler for byref/out parameters.
> // Not marking this instructions as safe would mean that it would be impossible to fix a delegate that takes
> // a byref/out parameter.
> bitmask.Set (Code.Stind_I1);
> bitmask.Set (Code.Stind_I2);
> bitmask.Set (Code.Stind_I4);
> bitmask.Set (Code.Stind_I8);
> bitmask.Set (Code.Stind_R4);
> bitmask.Set (Code.Stind_R8);
> bitmask.Set (Code.Stind_Ref);
> safe_instructions = bitmask;
> Console.WriteLine ("safe_instructions: {0}", bitmask);
> }
> #endif
> {
> return RuleResult.DoesNotApply;
By contract, 'method' cannot be null (no need to check). Sadly that not
something Gendarme CheckParametersNullityInVisibleMethodsRule can (yet,
I have my eyes on 4.0 code contracts) deal with. Until then this is
something for the 'ignore-list'.
>
> //
> // We need to check all methods which has any of the following opcodes: call, stfld or stsfld.
> //
> } else if (ins.IsStoreLocal ()) {
> VerifyStoreLocalInstruction (ins, stack_count);
>
> stack_count += push - pop;
>
> while (stack_count > stack.Count)
> stack.Add (new ILRange (ins));
> while (stack_count < stack.Count)
> stack.RemoveAt (stack.Count - 1);
>
> if (stack_count > 0 && stack [stack_count - 1] != null)
> stack [stack_count - 1].Last = ins;
> }
>
>
> return Runner.CurrentRuleResult;
> }
>
> private void VerifyStoreLocalInstruction (Instruction ins, int stack_count)
> {
> List<MethodDefinition> pointers;
>
> Log.WriteLine (this, " Reached a local variable store at offset {2:X}. index {0}, there are {1} pointers here.", index, pointers == null ? 0 : pointers.Count, ins.Offset);
>
> if (pointers != null && pointers.Count > 0) {
> while (locals.Count <= index)
> locals.Add (null);
> if (locals [index] == null)
> locals [index] = new List<MethodDefinition> ();
> locals [index].AddRange (pointers);
> }
> }
>
> {
> List<MethodDefinition> pointers;
>
> Log.WriteLine (this, " Reached a field variable store to the field {0}, there are {1} unsafe pointers here.", field.Name, pointers == null ? 0 : pointers.Count);
> if (pointers != null && pointers.Count > 0) {
> List<MethodDefinition> tmp;
> tmp.AddRange (pointers);
> }
>
> // If this field has been loaded into a pinvoke,
> // check the list for unsafe pointers.
> VerifyMethods (pointers);
> }
>
> private void VerifyCallInstruction (Instruction ins)
> {
> MethodDefinition called_method;
>
> if (called_method != null && called_method.IsPInvokeImpl) {
> Log.WriteLine (this, " Reached a call instruction to a pinvoke method: {0}", called_method.Name);
>
> for (int i = 0; i < called_method.Parameters.Count; i++) {
> if (stack [i] == null)
> continue;
>
> if (!called_method.Parameters [i].ParameterType.IsDelegate ())
> continue;
>
> Log.WriteLine (this, " Parameter #{0} takes a delegate, stack expression:", i);
> // if we load a field, store the field so that any subsequent unsafe writes to that field can be reported.
> Instruction last = stack [i].Last;
> if (last.OpCode.Code == Code.Ldfld || last.OpCode.Code == Code.Ldsfld) {
> FieldDefinition field = (last.Operand as FieldReference).Resolve ();
> loaded_fields.Add (field);
>
> // Get and check the pointers
> }
> }
>
> // Verifies that the method is safe to call as a callback from native code.
> {
> bool result;
> bool valid_ex_handler;
> InstructionCollection instructions;
>
> if (callback == null)
> return true;
>
> if (!callback.HasBody)
> return true;
>
> if (verified_methods.TryGetValue (callback, out result))
> return result;
>
> instructions = body.Instructions;
> is_safe.Capacity = body.Instructions.Count;
> //
> // We assume that the method is verifiable.
> //
>
> Log.WriteLine (this, " {0} {1}: {2}", i, instructions [i].OpCode.Code, safe);
> is_safe.Add (safe);
> }
>
> // Check that all instructions have been marked as safe, otherwise mark the method as unsafe.
> #if DEBUG
> Log.WriteLine (this, " Method {0} verified: {1}.", callback.Name, valid_ex_handler);
> }
> foreach (ExceptionHandler e in body.ExceptionHandlers)
> Log.WriteLine (this, " Type: {7}, TryStart: {4}, TryEnd: {5}, HandlerStart: {0}, HandlerEnd: {1}, FilterStart: {2}, FilterEnd: {3}, CatchType: {6}",
> e.HandlerStart.GetOffset (), e.HandlerEnd.GetOffset (), e.FilterStart.GetOffset (), e.FilterEnd.GetOffset (),
> e.TryStart.GetOffset (), e.TryEnd.GetOffset (), e.CatchType, e.Type);
> #endif
>
> verified_methods.Add (callback, valid_ex_handler);
>
> return valid_ex_handler;
> }
>
> public Instruction Last;
> public ILRange (Instruction first)
> {
> First = first;
> }
> {
> if (!Log.IsEnabled (this.GetType ().Name))
> return;
>
> Instruction instr = First;
> do {
> Log.WriteLine (this, "{0}{1}", prefix, instr.ToPrettyString ());
> if (instr == Last)
> break;
> instr = instr.Next;
> } while (true);
> }
>
> // Parses the ILRange and return all delegate pointers that could end up on the stack as a result of executing that code.
> result = new List<MethodDefinition> ();
> result.AddRange (pointers);
> }
> }
> }
>
> return result;
> }
>
> {
> Log.WriteLine (this, " Verifying {0} method pointers.", pointers == null ? 0 : pointers.Count);
>
> if (pointers == null)
> return;
>
> ReportVerifiedMethod (pointers [i], VerifyCallbackSafety (pointers [i]));
>
> // Reports the result from verifying the method.
> {
> if (!safe) {
> if (reported_methods.Contains (pointer))
> return;
>
> reported_methods.Add (pointer);
>
> Log.WriteLine (this, " Reporting: {0}", pointer.Name);
> Runner.Report (pointer, Severity.High, Confidence.High);
> } else {
> Log.WriteLine (this, " Safe: {0}", pointer.Name);
> }
> }
> }
>
> internal static class DelegatesPassedToNativeCodeMustIncludeExceptionHandlingRuleHelper
> {
> {
> if (instr == null)
> return "<nil>";
>
> return string.Format ("IL_{0} {1} {2}", instr.Offset, instr.OpCode.Name, instr.Operand);
> }
>
> public static int GetOffset (this Instruction instr)
> {
> if (instr != null)
> return instr.Offset;
> return -1;
> }
> // This could probably go into InstructionRocks.
> {
> switch (ins.OpCode.Code) {
> case Code.Ldloc_0: return 0;
> case Code.Ldloc_1: return 1;
> case Code.Ldloc_2: return 2;
> case Code.Ldloc_3: return 3;
> case Code.Ldloc: // Untested
> case Code.Ldloca: // Untested
> case Code.Ldloca_S:
> case Code.Ldloc_S: return ((VariableDefinition) ins.Operand).Index;
> default:
> }
>
> // Return the index of the store opcode.
> {
> switch (ins.OpCode.Code) {
> case Code.Stloc_1: return 1;
> case Code.Stloc_2: return 2;
> case Code.Stloc_3: return 3;
> case Code.Stloc: // Untested for stloc
> default:
> throw new ArgumentException (string.Format ("Invalid opcode: {0}", ins.OpCode.Name));
> }
> }
> }
> }
Thanks
Sebastien
Rolf Bjarne Kvinge
So over a year after I initially started working again, I once more
lost over a day tracking down one of these exception leaking issues,
so I decided to complete it.
Attached is final, fixed patch against trunk as of today.
I've addressed the issues raised in the latest review, and there is
one extra change: the rule catches all exceptions created during its
execution, and reports a low confidence/severity failure in this case,
to address issues reported in another thread where the rule could end
up throwing exceptions with uncommon code sequences (for the record:
http://groups.google.com/group/gendarme/browse_frm/thread/c37d157ae0c9682/57f89f3abf14f2fd?tvc=1&q=Gendarme+2.6+Preview+1+is+ready+for+download#57f89f3abf14f2fd).
Rolf
On Mon, Nov 30, 2009 at 4:46 AM, Sebastien Pouliot
> --
>
> You received this message because you are subscribed to the Google Groups "Gendarme" group.
> To post to this group, send email to gend...@googlegroups.com.
> To unsubscribe from this group, send email to gendarme+u...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/gendarme?hl=en.
>
>
>
Sebastien Pouliot
Quite a huge update :-)
From a quick (time versus size ;-) it seems great except for two
things.
1- you removed a bunch of C.WL from the unit tests. I added (at least
some of) them a while ago because otherwise CSC optimize the code and
remove the try/catch - i.e. some unit tests fails on Windows, well
once compiled with CSC, using your patch (4 with CSC10*).
2- the "catch all" in the rule. I don't like them because they
(generally) hide problems (that go unreported) and lead to bad
results. Do you have, beside (unsupported) obfuscated code, any test
case that requires it ?
Thanks!
Sebastien
------ Test started: Assembly: Tests.Rules.Interoperability.dll ------
TestCase
'Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_CallbackOK1'
failed:
Expected: Success
But was: Failure
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(722,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.AssertTest(String
name, Int32 expectedCount)
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(701,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.AssertTest(String
name)
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(795,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_CallbackOK1()
TestCase
'Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_CallbackOKStatic'
failed:
Expected: Success
But was: Failure
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(722,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.AssertTest(String
name, Int32 expectedCount)
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(701,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.AssertTest(String
name)
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(802,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_CallbackOKStatic()
TestCase
'Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_NonVoid'
failed:
Expected: Success
But was: Failure
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(722,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.AssertTest(String
name, Int32 expectedCount)
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(774,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_NonVoid()
TestCase
'Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_OK2'
failed:
Expected: Success
But was: Failure
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(722,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.AssertTest(String
name, Int32 expectedCount)
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(701,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.AssertTest(String
name)
Test
\DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.cs(788,0):
at
Test.Rules.Interoperability.DelegatesPassedToNativeCodeMustIncludeExceptionHandlingTest.Test_OK2()
71 passed, 4 failed, 0 skipped, took 16.83 seconds (NUnit 2.4.7).
On Jul 8, 8:52 pm, Rolf Bjarne Kvinge <rolfbja...@gmail.com> wrote:
> Hi Sebastien,
>
> So over a year after I initially started working again, I once more
> lost over a day tracking down one of these exception leaking issues,
> so I decided to complete it.
>
> Attached is final, fixed patch against trunk as of today.
>
> I've addressed the issues raised in the latest review, and there is
> one extra change: the rule catches all exceptions created during its
> execution, and reports a low confidence/severity failure in this case,
> to address issues reported in another thread where the rule could end
> Rolf
>
> On Mon, Nov 30, 2009 at 4:46 AM, Sebastien Pouliot
>
> > On Fri, 2009-11-20 at 23:51 +0100, Rolf Bjarne Kvinge wrote:
> >> Hi Sebastien
>
>
> read more »
>
> delegates-check.patch
> 70KViewDownload
Sebastien Pouliot
Sorry for the delay - I was sure I replied before vacations... but since
I can't find it in my "Sent" folder I'll assume I really needed the
vacations ;-)
On Thu, 2010-07-15 at 10:57 +0200, Rolf Bjarne Kvinge wrote:
> Hi,
>
> On Wed, Jul 14, 2010 at 4:05 AM, Sebastien Pouliot
> <sebastie...@gmail.com> wrote:
> > Hello Rolf,
> >
> > Quite a huge update :-)
> >
> > From a quick (time versus size ;-) it seems great except for two
> > things.
> >
> > 1- you removed a bunch of C.WL from the unit tests. I added (at least
> > some of) them a while ago because otherwise CSC optimize the code and
> > remove the try/catch - i.e. some unit tests fails on Windows, well
> > once compiled with CSC, using your patch (4 with CSC10*).
>
> I removed them because they're incorrect :)
>
> in a method that should pass this rule, this is bad:
>
> try {
> ...
> } catch {
> Console.WriteLine ();
> }
>
> The Console.WriteLine () statement is marked as unsafe. I removed cwls
> from catch and finally clauses because of this. I do find it strange
> that csc can optimize a method to make it unsafe - but I'll have a
> look to see what's happening.
Ok it starting to make sense (with your next emails about leave.s). I
remember some "exception-related removal optimizations" wrt csc - but
they could be related to another rule...
> >
> > 2- the "catch all" in the rule. I don't like them because they
> > (generally) hide problems (that go unreported) and lead to bad
> > results. Do you have, beside (unsupported) obfuscated code, any test
> > case that requires it ?
>
> No, I don't have any test cases that require it.
That's ok - otherwise I would have asked for a copy of the assembly ;-)
> I found it bad to
> have the rule throw exceptions, even once very rarely, because it
> would make it impossible to run gendarme on the assembly in question,
Not really, you need to ignore the rule on the target (or turn it off).
> since gendarme would just exit with a stack trace.
The console runner would (and gets me the most feedback ;-) but the
wizard would continue execution (and report the problem).
> To not hide
> problems the rule still reports a failure though.
Well a there is a few reasons (but no "god-like" command) for this:
1) It's easy to get a lot of extra (and/or ugly) code since many things
can go wrong. It can hide useful information that can be reported (or
needs more extra code). It's also easy to introduce bugs when ignoring
exceptions (see #2) which makes "aborting" a safe choice.
2) It cannot be done at the runner level (e.g. a catch {} before calling
Check* methods). This could break engines and/or (gendarme's framework
or rule-based) caches (it happened before) and make issues a lot more
complex to diagnose (e.g. adding a lot of false positive defects). All
bets are off if an exceptions occurs wrt engines or updating caches (but
it's not the case in your rule)
3) It works ;-) Seriously I don't get much feedback, even less test
cases, for false positives (but many/most complaints ;-). OTOH I get
feedback (and test cases) for unhandled exceptions since they "stop" the
analysis. Since I'm feedback-driven I need this badly!
Then there are still a few cases where "catch-all" are used by rules (or
runners). Mostly because of the (.net) framework lack of documentation
(exceptions are rarely fully documented) or bad API (e.g. validating a
Regex). I'm happy Try* methods are getting more common in .net 4) and I
intend to remove a few "catch-all" in the future.
> In any case I don't
> feel strongly either way, I don't obfuscate assemblies in the first
> place :)
I don't really care about obfuscated assemblies. OTOH some compilers are
currently under-represented in my test cases (and some do look like
obfuscated assemblies ;-)
> Have in mind that the assembly does not have to be obfuscated in the
> first place, it may happen with any normal assembly, I just haven't
> seen any vb/c# compiler generate such code sequences.
I've been building a quite varied (local) garden of assemblies. I can't
distribute most of them but I do use them for testing Gendarme before
releases. Most of them comes from the feedback I've been given ;-)
Sebastien
p.s. please commit :-)