diff options
author | Sebastien Pouliot <sebastien@ximian.com> | 2008-02-15 22:19:05 +0300 |
---|---|---|
committer | Sebastien Pouliot <sebastien@ximian.com> | 2008-02-15 22:19:05 +0300 |
commit | 027bb08303d5d356ef112f3d0a601a8b46d299dd (patch) | |
tree | 08995ce7dda028130340e70123085fb5a23b2142 /gendarme/rules | |
parent | 4d1867743a82af7368e95c8922e48b44862df903 (diff) |
2008-02-15 Sebastien Pouliot <sebastien@ximian.com>
* GetLastErrorMustBeCalledRightAfterPInvokeRule.cs
* MarshalStringsInPInvokeDeclarationsRule.cs
* PInvokeShouldNotBeVisibleRule.cs
* UseManagedAlternativesToPInvokeRule.cs:
Update rules wrt framework changes.
svn path=/trunk/mono-tools/; revision=95794
Diffstat (limited to 'gendarme/rules')
5 files changed, 89 insertions, 91 deletions
diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog b/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog index 7a2c5fa2..ad256e41 100644 --- a/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog +++ b/gendarme/rules/Gendarme.Rules.Interoperability/ChangeLog @@ -1,3 +1,11 @@ +2008-02-15 Sebastien Pouliot <sebastien@ximian.com> + + * GetLastErrorMustBeCalledRightAfterPInvokeRule.cs + * MarshalStringsInPInvokeDeclarationsRule.cs + * PInvokeShouldNotBeVisibleRule.cs + * UseManagedAlternativesToPInvokeRule.cs: + Update rules wrt framework changes. + 2008-01-30 Sebastien Pouliot <sebastien@ximian.com> * GetLastErrorMustBeCalledRightAfterPInvokeRule.cs: Changed to diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs index e37dba0e..3e303ef8 100644 --- a/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs +++ b/gendarme/rules/Gendarme.Rules.Interoperability/GetLastErrorMustBeCalledRightAfterPInvokeRule.cs @@ -26,17 +26,21 @@ // 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.Helpers; using Gendarme.Framework.Rocks; namespace Gendarme.Rules.Interoperability { - public class GetLastErrorMustBeCalledRightAfterPInvokeRule : IMethodRule { + [Problem ("GetLastError() should be called immediately after this the P/Invoke call.")] + [Solution ("Move the call to GetLastError just after the P/Invoke call.")] + public class GetLastErrorMustBeCalledRightAfterPInvokeRule : Rule, IMethodRule { struct Branch { public readonly Instruction Instruction; @@ -49,6 +53,8 @@ namespace Gendarme.Rules.Interoperability { } } + private const string Message = "GetLastError() should be called immediately after this the PInvoke call."; + private const string GetLastError = "System.Int32 System.Runtime.InteropServices.Marshal::GetLastWin32Error()"; private List<string> AllowedCalls; @@ -62,7 +68,7 @@ namespace Gendarme.Rules.Interoperability { List<Branch> branches = new List<Branch> (); - private bool CheckPInvoke (MethodDefinition pinvoke, Instruction startInstruction) + private bool CheckPInvoke (Instruction startInstruction) { branches.Clear (); branches.Add (new Branch (startInstruction.Next, false)); @@ -102,8 +108,9 @@ namespace Gendarme.Rules.Interoperability { break; if (alternatives != null) { - if (alternatives is Instruction) { - branches.AddIfNew (new Branch ((Instruction) alternatives, dirty)); + Instruction alt_ins = (alternatives as Instruction); + if (alt_ins != null) { + branches.AddIfNew (new Branch (alt_ins, dirty)); } else { Instruction [] alts = (Instruction []) alternatives; foreach (Instruction altIns in alts) @@ -124,12 +131,11 @@ namespace Gendarme.Rules.Interoperability { return true; } - public MessageCollection CheckMethod (MethodDefinition method, Runner runner) + public RuleResult CheckMethod (MethodDefinition method) { + // rule does not apply if the method has no IL if (!method.HasBody) - return runner.RuleSuccess; - - MessageCollection results = null; + return RuleResult.DoesNotApply; foreach (Instruction ins in method.Body.Instructions) { switch (ins.OpCode.Code) { @@ -150,21 +156,18 @@ namespace Gendarme.Rules.Interoperability { break; // check if GetLastError is called near enough this pinvoke call - if (CheckPInvoke (pinvoke, ins)) + if (CheckPInvoke (ins)) break; - if (results == null) - results = new MessageCollection (); - Location loc = new Location (method, ins.Offset); - Message msg = new Message ("GetLastError() should be called immediately after this the PInvoke call.", loc, MessageType.Error); - results.Add (msg); + // code might not work if an error occurs + Runner.Report (method, ins, Severity.High, Confidence.High, String.Empty); break; default: break; } } - return results; + return Runner.CurrentRuleResult; } } } diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs index ab5744eb..643a3397 100644 --- a/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs +++ b/gendarme/rules/Gendarme.Rules.Interoperability/MarshalStringsInPInvokeDeclarationsRule.cs @@ -33,38 +33,39 @@ using Gendarme.Framework; namespace Gendarme.Rules.Interoperability { - public class MarshalStringsInPInvokeDeclarationsRule : IMethodRule { + [Problem ("Marshaling information for string types is incomplete and what is required may be different from what you expected the default to be.")] + [Solution ("Add [DllImport CharSet=] on the method or [MarshalAs] on the parameter(s)")] + public class MarshalStringsInPInvokeDeclarationsRule : Rule, IMethodRule { private static bool IsStringOrSBuilder (TypeReference reference) { - TypeReference original = reference.GetOriginalType (); - return original.FullName == "System.String" || original.FullName == "System.Text.StringBuilder"; + switch (reference.GetOriginalType ().FullName) { + case "System.String": + case "System.Text.StringBuilder": + return true; + default: + return false; + } } - private static void AddBadParameterMessage (ref MessageCollection messages, ParameterDefinition param) + public RuleResult CheckMethod (MethodDefinition method) { - if (messages == null) - messages = new MessageCollection (); + // rule does not apply to non-pinvoke methods + if (!method.IsPInvokeImpl) + return RuleResult.DoesNotApply; - Location loc = new Location (param.Method as MethodDefinition); - string text = string.Format ("Parameter '{0}', of type '{1}', does not have [MarshalAs] attribute, yet no [DllImport CharSet=] is set for the method '{2}'.", - param.Name, param.ParameterType.Name, param.Method.Name); - Message msg = new Message (text, loc, MessageType.Error); - messages.Add (msg); - } + if (!method.PInvokeInfo.IsCharSetNotSpec) + return RuleResult.Success; - public MessageCollection CheckMethod (MethodDefinition method, Runner runner) - { - if (!method.IsPInvokeImpl || !method.PInvokeInfo.IsCharSetNotSpec) - return runner.RuleSuccess; - - MessageCollection messages = runner.RuleSuccess; foreach (ParameterDefinition parameter in method.Parameters) { if (IsStringOrSBuilder (parameter.ParameterType) && (parameter.MarshalSpec == null)) { - AddBadParameterMessage (ref messages, parameter); + string text = string.Format ("Parameter '{0}', of type '{1}', does not have [MarshalAs] attribute, yet no [DllImport CharSet=] is set for the method '{2}'.", + parameter.Name, parameter.ParameterType.Name, parameter.Method.Name); + Runner.Report (parameter, Severity.High, Confidence.Total, text); } } - return messages; + + return Runner.CurrentRuleResult; } } } diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs index 9079dec8..f5768898 100644 --- a/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs +++ b/gendarme/rules/Gendarme.Rules.Interoperability/PInvokeShouldNotBeVisibleRule.cs @@ -31,32 +31,30 @@ using System; using Mono.Cecil; using Gendarme.Framework; +using Gendarme.Framework.Rocks; namespace Gendarme.Rules.Interoperability { - public class PInvokeShouldNotBeVisibleRule : IMethodRule { + [Problem ("P/Invoke declarations should not be visible outside of the assembly.")] + [Solution ("Wrap the p/invoke call into a managed class/method and include parameters, and result, validation(s).")] + public class PInvokeShouldNotBeVisibleRule : Rule, IMethodRule { - public MessageCollection CheckMethod (MethodDefinition method, Runner runner) + public RuleResult CheckMethod (MethodDefinition method) { + // rule does not apply to non-p/invoke if (!method.IsPInvokeImpl) - return runner.RuleSuccess; - - if (!method.IsPublic) - return runner.RuleSuccess; - - TypeDefinition type = (TypeDefinition) method.DeclaringType; - - while (type.IsNested) { - if (!type.IsNestedPublic) - return runner.RuleSuccess; - type = (TypeDefinition) type.DeclaringType; - } - if (!type.IsPublic) - return runner.RuleSuccess; - - Location loc = new Location (method); - Message msg = new Message ("P/Invoke declarations should not be visible outside of the assembly.", loc, MessageType.Warning); - return new MessageCollection (msg); + return RuleResult.DoesNotApply; + + // rule applies + + // ok if method is not visible (this include it's declaring type too) + if (!method.IsVisible ()) + return RuleResult.Success; + + // code will work (low) but it's bad design (non-fx-like validations) and makes + // it easier to expose security vulnerabilities + Runner.Report (method, Severity.Low, Confidence.Total, String.Empty); + return RuleResult.Failure; } } } diff --git a/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs b/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs index c158f3f0..bbe235c9 100644 --- a/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs +++ b/gendarme/rules/Gendarme.Rules.Interoperability/UseManagedAlternativesToPInvokeRule.cs @@ -37,7 +37,9 @@ using Mono.Cecil; namespace Gendarme.Rules.Interoperability { - public class UseManagedAlternativesToPInvokeRule : IMethodRule { + [Problem ("There is a potential managed API for the p/invoke declaration.")] + [Solution ("Use the suggested managed alternative to your p/invoke call and remove it's declaration.")] + public class UseManagedAlternativesToPInvokeRule : Rule, IMethodRule { private struct PInvokeCall { @@ -89,7 +91,7 @@ namespace Gendarme.Rules.Interoperability { } } - private class ManagedAlternatives { + private sealed class ManagedAlternatives { private List<string> alternatives = new List<string> (); private TargetRuntime runtime; @@ -122,28 +124,17 @@ namespace Gendarme.Rules.Interoperability { } } - private static Dictionary<PInvokeCall, ManagedAlternatives> managedAlternatives; - - static UseManagedAlternativesToPInvokeRule () - { - managedAlternatives = new Dictionary<PInvokeCall, ManagedAlternatives> (); - managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "Sleep"), - new ManagedAlternatives ("System.Threading.Thread::Sleep ()")); - managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "FindFirstFile"), - new ManagedAlternatives ("System.IO.Directory::GetDirectories ()", "System.IO.Directory::GetFiles ()", "System.IO.Directory::GetFileSystemEntries ()")); - managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "ReadFile"), - new ManagedAlternatives ("System.IO.FileStream")); - managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "WaitForMultipleObjects"), - new ManagedAlternatives ("System.Threading.WaitHandle::WaitAny ()", "System.Threading.WaitHandle::WaitAll ()")); - managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "GetLastError"), - new ManagedAlternatives ("System.Runtime.InteropServices.Marshal::GetLastWin32Error ()")); - managedAlternatives.Add (new PInvokeCall ("user32.dll", "MessageBox"), - new ManagedAlternatives ("System.Windows.Forms.MessageBox::Show ()")); - managedAlternatives.Add (new PInvokeCall ("kernel32.dll", "Beep"), - new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Console::Beep ()")); - managedAlternatives.Add (new PInvokeCall ("winmm.dll", "PlaySound"), - new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Media.SoundPlayer")); - } + private static Dictionary<PInvokeCall, ManagedAlternatives> managedAlternatives = + new Dictionary<PInvokeCall, ManagedAlternatives> () { + { new PInvokeCall ("kernel32.dll", "Sleep"), new ManagedAlternatives ("System.Threading.Thread::Sleep ()") }, + { new PInvokeCall ("kernel32.dll", "FindFirstFile"), new ManagedAlternatives ("System.IO.Directory::GetDirectories ()", "System.IO.Directory::GetFiles ()", "System.IO.Directory::GetFileSystemEntries ()") }, + { new PInvokeCall ("kernel32.dll", "ReadFile"), new ManagedAlternatives ("System.IO.FileStream") }, + { new PInvokeCall ("kernel32.dll", "WaitForMultipleObjects"), new ManagedAlternatives ("System.Threading.WaitHandle::WaitAny ()", "System.Threading.WaitHandle::WaitAll ()") }, + { new PInvokeCall ("kernel32.dll", "GetLastError"), new ManagedAlternatives ("System.Runtime.InteropServices.Marshal::GetLastWin32Error ()") }, + { new PInvokeCall ("user32.dll", "MessageBox"), new ManagedAlternatives ("System.Windows.Forms.MessageBox::Show ()") }, + { new PInvokeCall ("kernel32.dll", "Beep"), new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Console::Beep ()") }, + { new PInvokeCall ("winmm.dll", "PlaySound"), new ManagedAlternatives (TargetRuntime.NET_2_0, "System.Media.SoundPlayer") } + }; private static ManagedAlternatives GetManagedAlternatives (MethodDefinition method, TargetRuntime runtime) { @@ -151,30 +142,27 @@ namespace Gendarme.Rules.Interoperability { PInvokeCall callInfo = new PInvokeCall (moduleName, method.Name); if (managedAlternatives.ContainsKey (callInfo)) { ManagedAlternatives alts = managedAlternatives [callInfo]; - if (IsRuntimeVersionGreaterOrEqual (runtime, alts.Runtime)) + if (runtime >= alts.Runtime) return alts; } return null; } - private static bool IsRuntimeVersionGreaterOrEqual (TargetRuntime a, TargetRuntime b) - { - return (int) a >= (int) b; // as of now it works (and should work further) - } - - public MessageCollection CheckMethod (MethodDefinition method, Runner runner) + public RuleResult CheckMethod (MethodDefinition method) { + // rule does not apply to non-pinvoke methods if (!method.IsPInvokeImpl) - return runner.RuleSuccess; + return RuleResult.DoesNotApply; + + // rule apply, looks for alternatives ManagedAlternatives alternatives = GetManagedAlternatives (method, method.DeclaringType.Module.Assembly.Runtime); if (alternatives == null) - return runner.RuleSuccess; + return RuleResult.Success; - Location loc = new Location (method); string message = string.Format ("Do not perform platform-dependent call ({0}) if it can be avoided. Use (one of) the following alternative(s) provided by .NET Framework: {1}.", method.Name, alternatives); - Message msg = new Message (message, loc, MessageType.Warning); - return new MessageCollection (msg); + Runner.Report (method, Severity.Low, Confidence.High, message); + return RuleResult.Failure; } } } |