Some notes from my use of Gengarme...
Andy Hume
My experiences on running Gendarme on one of my code bases. Just some thoughts... I've logged one issue in bugzilla the following are less black-and-white so wasn't sure enough to reports them there, let me know if any should be logged. Apologies for the length.
Various good things that it detected included: some classes that should be sealed, and two instances of "no dispose of member" (one each DisposableFieldsShouldBeDisposedRule and TypesWithDisposableFieldsShouldBeDisposableRule). I'll be looking to integrate it into my build.
This code base runs clean with FxCop, with two rules disabled ("member should be static" and localization stuff) and various [SuppressMessage] applied.
* Marked with SuppressMessage
So my first though it that it would be good if the [SuppressMessage]s could be respected. Local cases included:
AvoidUnusedParametersRule : 4 defects
EnumsShouldDefineAZeroValueRule : 3 defects
EnumsShouldUseInt32Rule : 4 defects
EnumNotEndsWithEnumOrFlagsSuffixRule : 1 defects
AvoidReturningArraysOnPropertiesRule : 4 defects
For instance the enums were for network protocol values so rule Int32/0=None/etc didn't apply to them. However maybe there isn't a direct correlation in the "enum end flags" case with "CA1711:IdentifiersShouldNotHaveIncorrectSuffix" supression...
* SecureGetObjectDataOverridesRule "SerializationFormatter is not a subset of LinkDemand permission set"
Bug 375368.
* Should the corresponding serialization constructor have a similar rule?
One generally sees it recommended that that method have the same demand applied. As creating a instance of the class but supplying bad values for the internal priviledged data could also create a security hole. Maybe in my cases since they're protected stops any reports?
* ConsiderConvertingMethodToPropertyRule
Various reports of that, in many of the cases it wasn't a useful report. I'd SuppressMessages on some. Secondly there were cases where the was a SetXxxYyy method and no GetXxxYyy, so do that fix would then cause a "shouldn't only be a set property" violation.
Also in "class XxxxStream : Stream" cases, SetLength was highlighted. There is already a get_Length property, maybe in that case the rule is correct but I wonder whether here, and in general, violations shouldn't be reported for cases that are simply implementing base class members?
* Duplicated code
See the attached code, set_Size vs ResetSize, kind-of a nullable property, don't know whether this could be detected as different code without affecting the rule's general efficacy.
* DoubleCheckLockingRule
I think a false positive, see the code sample, don't know whether it can be excluded from the pattern matching. Weird code it may be!
* ImplementEqualsAndGetHashCodeInPairRule
When only GetHashCode present, it says "Details: ... but does not implement 'System.Int32 GetHashCode()'". It's the reverse here.
* AvoidLongParameterListsRule
On a delegate it reports a violation on the BeginInvoke method, only the Invoke method should be checked IMHO -- that's the one under user control.
* Report format
** Move the "rules used" section to the bottom. It currently dominates, and I was always clicking it to navigate in the report. :-,( The navigation table should be put in its place include the violation counts also.
** Would be good for the sections to be collapsable nodes. Then one can close a section when its been dealt with.
* Some typos in the report, some on the bounds of personal preference however.
"No (LinkDemand) were found." -- s/were/was/
"Implement...in it's Dispose method." -- s/it's/its/ even my mother once a primary school teacher has trouble *explaining* that one!
"There are same code structure..." -> "There is similar code ..."
"Exists code duplicated with..." -> "Duplicate code in ..." -- just have implicit 'There is'.
"...class too large, ... not be far away." -- adding "away".
MissingSerializableAttributeOnISerializableTypeRule "...add the [Serialization] attribute to its definition." should of course be "[Serializable]".
* FX vs code rule set
FxCop in /general/ reports things that affect the public interface to a class library, and not internal things -- it is "FX" cop after all. Gendarme is doing both, would it be good for the SWF runner to prompt with a choice of default rule-sets "public interface", or "all". It was a bit weird to get all these violations for a code base that ran clean in FxCop. But maybe that's always going to be the case when one runs a new tool on any code base. :-,)
* SWF runner
I haven't investigated this thoroughly but it appear that it one steps back (<Back) and then to completion again, then the target assembly is /not/ re-read even if it has changed. Instead it appears that the runner app must be restarted to pick up changes. I expected it to re-read the target assembly every time it generated a report.
* console runner
Now /does/ generate a report with only the target assembly on the command-line. :-) Though does it use a different rule set to the SWF runner?
* msbuild.exe build still working for me. :-)
Andy
Sebastien Pouliot
I'll start looking/fixing/answering/... it tonight but it may take me
some time to answer everything. Actually I think I'll make multiple
answers...
Thanks :D
Sebastien
> using System.Security.Permissions;
>
> namespace Zzzzzzz
> {
>
> public abstract class MyStream : System.IO.Stream
> {
> // RULE: Should this method be identified as "should be a property"?
> // Note there's no GetXxxx partner, and we're only implementing the base class's interface.
>
> public override void SetLength(Int64 value)
> {
> m_len = value;
> }
> long m_len;
>
> }
>
> public class FooBar
> {
>
> //---------------------------------------------------
>
> // RULE: Should this method be identified as "should be a property"?
> // Note there's no GetXxxx partner.
>
> public void SetAaaa(int zzz)
> {
> Log(zzz.ToString());
> }
>
> //---------------------------------------------------
> int m_size = -1;
> bool m_hasSize;
>
> // RULE: Should this method and the set property be seen as "duplicate code"?
>
> public void ResetValue()
> {
> m_hasSize = false;
> m_size = -1;
> }
>
> public int Size
> {
> set
> {
> m_hasSize = true;
> m_size = value;
> }
> get
> {
> if (!m_hasSize)
> throw new InvalidOperationException();
> return m_size;
> }
> }
>
> public bool HasSize { get { return m_hasSize; } }
>
> //---------------------------------------------------
>
> bool m_aborting = false;
> object m_lock = new object();
>
> public void DoSomethingWithSharedState()
> {
> // RULE: not an instance of double-locking.
>
> if (m_aborting)
> return;
> lock (m_lock) {
> if (m_aborting)
> return;
> //... ...
> Log(m_aborting.ToString());
> //... ...
> }
> }
>
> //---------------------------------------------------
> // infrastructure
> String name = "foo";
>
> private void Log(String x)
> {
> string log = name + ": " + x;
> GC.KeepAlive(log); //dummy usage
> }
> }//class
>
> class FooHasHashCode
> {
> public override int GetHashCode()
> {
> return 0;
> }
> }
>
> // BeginInvoke too many parameters, but Invoke is ok, which is the user controllable member.
> public delegate void InvokeHasAnOkNumber(int a, int b, int c, int d);
>
> //==================================================================
> [Serializable]
> public class My1PermsFlagEqualsException : Exception
> {
> string m_foo;
>
> public My1PermsFlagEqualsException(String foo)
> {
> m_foo = foo;
> }
>
> [SecurityPermission(SecurityAction.LinkDemand, Flags = SecurityPermissionFlag.SerializationFormatter)]
> protected My1PermsFlagEqualsException(
> System.Runtime.Serialization.SerializationInfo info,
> System.Runtime.Serialization.StreamingContext sc)
> {
> m_foo = info.GetString("Foo");
> }
>
> // RULE: I applied the form from the method's page in MSDN, i.e. Flags=SFformatter.
> // Does it differ from the SFormatter=true form?
> // I'm not expert on that aspect but looking at Mono's SecurityPermissionAttribute
> // it appears that (SF=true) and (Flags=SF) are equivalent.
> //
> //Msft's Exception: [SecurityPermission(SecurityAction.LinkDemand,
> // Flags = SecurityPermissionFlag.SerializationFormatter)]
> //Mono's Exception: [SecurityPermission(SecurityAction.LinkDemand, SerializationFormatter=true)]
> //
> //gmcs however converts the first form into the sectond form.
>
> [SecurityPermission(SecurityAction.LinkDemand, Flags = SecurityPermissionFlag.SerializationFormatter)]
> public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info,
> System.Runtime.Serialization.StreamingContext context)
> {
> info.AddValue("Foo", m_foo);
> }
>
> }//class
>
> //RULE: says "...add the [Serialization] attribute...", should say "[Serializable]".
> public class My2NoPermsException : Exception
> {
> string m_foo;
>
> public My2NoPermsException(String foo)
> {
> m_foo = foo;
> }
>
> protected My2NoPermsException(
> System.Runtime.Serialization.SerializationInfo info,
> System.Runtime.Serialization.StreamingContext sc)
> {
> m_foo = info.GetString("Foo");
> }
>
> public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info,
> System.Runtime.Serialization.StreamingContext context)
> {
> info.AddValue("Foo", m_foo);
> }
>
> }//class
>
> [Serializable]
> public class My3PermsPropsException : Exception
> {
> string m_foo;
>
> public My3PermsPropsException(String foo)
> {
> m_foo = foo;
> }
>
> [SecurityPermission(SecurityAction.LinkDemand, SerializationFormatter = true)]
> protected My3PermsPropsException(
> System.Runtime.Serialization.SerializationInfo info,
> System.Runtime.Serialization.StreamingContext sc)
> {
> m_foo = info.GetString("Foo");
> }
>
> // RULE: I applied the form from the method's page in MSDN, i.e. Flags=SFformatter.
> // Does it differ from the SFormatter=true form?
> // I'm not expert on that aspect but looking at Mono's SecurityPermissionAttribute
> // it appears that (SF=true) and (Flags=SF) are equivalent.
> //
> //Mono's Exception: [SecurityPermission(SecurityAction.LinkDemand, SerializationFormatter=true)]
>
> [SecurityPermission(SecurityAction.LinkDemand, SerializationFormatter = true)]
> public override void GetObjectData(System.Runtime.Serialization.SerializationInfo info,
> System.Runtime.Serialization.StreamingContext context)
> {
> info.AddValue("Foo", m_foo);
> }
>
> }//class
>
> class XxxMethodsWithSecPermAttr
> {
> [SecurityPermission(SecurityAction.LinkDemand, SerializationFormatter = true)]
> public void SfEqualsTrue() { }
>
> [SecurityPermission(SecurityAction.LinkDemand, Flags = SecurityPermissionFlag.SerializationFormatter)]
> public void FlagsEqualsSf() { }
>
> } //class
>
> }
Sebastien Pouliot
> * Marked with SuppressMessage
> So my first though it that it would be good if the [SuppressMessage]s could be respected. Local cases included:
Honestly I had not considered (or dreamed) that current FxCop users
would add Gendarme to their arsenal (at least so soon ;-). Also from a
Mono-centric perspective we don't add attributes that differs from the
FX (except for MonoTODO) so [SuppressMessage] is not very helpful
(e.g. it's not even in the 1.1 framework) for a lot of cases (except
where MS used it in some FX 3.0+ assemblies).
The current (and very recent) mechanism to ignore defects is the
"ignore list" (only supported by the console runner*) selected,
mainly, because it's a better fit with Mono. However I made this an
interface because I expected, at some point, to fix this bug. Right
now I'm "tempted" to add a new attribute to rules to map Gendarme/
FxCop rules and extend the ignore logic to support them. E.g.
[FxCopCompatibility ("CA1000")]
Of course this would only be a partial compatibility since some
Gendarme rules:
- implements more than one FxCop rule;
- split FxCop rules into several rules;
- some differs in scope (like checking internals); and
- some just don't exists (yet) in one or the other;
(*) the wizard is not designed to expose every features possible to
runners (e.g. ignore list). Hopefully a "full"(*) GUI runner would
support them and allow the developer to add/remove/edit them.
Anyway that's the way I see it today (subject to change without
notice ;-) but I'm not sure if this will happen before Mono 2.0 or
not...
> AvoidUnusedParametersRule : 4 defects
> EnumsShouldDefineAZeroValueRule : 3 defects
> EnumsShouldUseInt32Rule : 4 defects
> EnumNotEndsWithEnumOrFlagsSuffixRule : 1 defects
> AvoidReturningArraysOnPropertiesRule : 4 defects
> For instance the enums were for network protocol values so rule Int32/0=None/etc didn't apply to them. However maybe there isn't a direct correlation in the "enum end flags" case with "CA1711:IdentifiersShouldNotHaveIncorrectSuffix" supression...
test cases ?
> * SWF runner
> I haven't investigated this thoroughly but it appear that it one steps back (<Back) and then to completion again, then the target assembly is /not/ re-read even if it has changed. Instead it appears that the runner app must be restarted to pick up changes. I expected it to re-read the target assembly every time it generated a report.
purpose - so I could try different options without re-loading the
assemblies every time (which is time consuming*). However I never
considered your scenario which, in retrospect, makes lot of sense: any
assembly that has changed should be reloaded. I'll try to fix this
(without loosing the existing optimization).
(*) it even loads assemblies asynchronously so you can select your
rules while they load (useful if/when loading all assemblies from Mono
2.0 profile :-)
Talking about the wizard there's a few other things I'd like to "fix"
in the wizard before the next release:
- use the treeview "like a tree" for the rules, i.e. add nodes to make
it easy to [un]select all "Design" rules
- merge the two last step into a single one (not 100% sure about this
one right now)
> * console runner
> Now /does/ generate a report with only the target assembly on the command-line. :-) Though does it use a different rule set to the SWF runner?
inside "rules.xml". The rules.xml file shipped includes almost[1]
everything, except the "Smells" rules [2], into "default". The Smells
are included in the "self-test" set (everything but [1]), which
Gendarme use to analyze itself.
The wizard[3] runner simply list all rules it can find (in it's
directory) and let the user select which one it wants. Right now that
means:
- all rules are selected by default
- you have to re-select your rule "set" each time you restart the
wizard [4]
[1] except if the rule is considered too buggy
[2] the smells are a lot slower than most rules and I, personally,
prefer to run them less frequently (except when testing Gendarme
itself)
[3] the wizard is not meant as a full GUI runner and there's a lot of
things I'd like in a GUI runner...
[4] that being said maybe adding a button to "save as default set" to
the wizard would be useful. thoughts ?
More next time (I got a blog post to complete).
Sebastien
Néstor Salceda
> On Apr 7, 5:21 am, "Andy Hume" <andyhum...@yahoo.co.uk> wrote:
First of all, thanks you for your report. It will be used for
improving Gendarme.
> > * Duplicated code
> > See the attached code, set_Size vs ResetSize, kind-of a nullable property, don't know whether this could be detected as different code without affecting the rule's general efficacy.
Comments in the code.
> > * Some typos in the report, some on the bounds of personal preference however.
> > "No (LinkDemand) were found." -- s/were/was/
> > "Implement...in it's Dispose method." -- s/it's/its/ even my mother once a primary school teacher has trouble *explaining* that one!
> > "There are same code structure..." -> "There is similar code ..."
> > "Exists code duplicated with..." -> "Duplicate code in ..." -- just have implicit 'There is'.
> > "...class too large, ... not be far away." -- adding "away".
> > MissingSerializableAttributeOnISerializableTypeRule "...add the [Serialization] attribute to its definition." should of course be "[Serializable]".
I will fix this tonight.
> >
> > // RULE: Should this method and the set property be seen as "duplicate code"?
Answering the question it's difficult. There is similar code but, let
me show a possible refactoring:
> > public void ResetValue()
> > {
> > m_hasSize = false;
> > m_size = -1;
> > }
> >
> > public int Size
> > {
> > set
> > {
> > m_hasSize = true;
> > m_size = value;
> > }
> > get
> > {
> > if (!m_hasSize)
> > throw new InvalidOperationException();
> > return m_size;
> > }
> > }
public void ResetValue ()
{
Size = -1;
}
public int Size {
set
{
m_size = value;
m_hasSize = m_size == -1 ? false : true;
}
}
I think that refactored code looks fine (it's readable), and you don't
repeat de assignment. In this, case I believe the warning is correct,
but we should take care with this because we have the risk of bloating
the code with conditionals.
When I wrote the algorithm for detect duplicated data, I skipped the
instructions for loading parameters, because if the parameters change
but the code structure is the same perhaps you will need extract a
private helper method which sets the values (or perfoms the action) and
you must call the method with different parameters.
Do you agree with the result?
Thanks other time for the report, Andy.
Néstor.
Sebastien Pouliot
> Bug 375368.
attributes. JB fixed this today (in Cecil) and I fixed the security
rules tonight (at least all that use the PermissionSet of a
SecurityDeclaration) to call Resolve. So it should be working with SVN
HEAD (mcs and mono-tools)
> * Should the corresponding serialization constructor have a similar rule?
> One generally sees it recommended that that method have the same demand applied. As creating a instance of the class but supplying bad values for the internal priviledged data could also create a security hole. Maybe in my cases since they're protected stops any reports?
The rule is concerned about untrusted code using serialization to leak
private data out of (trusted) objects. From the constructor point of
view everything is reversed, i.e. it would be trusted code with
untrusted data.
Now this is a very real problem[1] however, since the code is trusted,
no CAS permission is going to help. Sadly I don't see a working way
(at least for serialization[2]) to make a useful rule out of it (short
of reporting all of them).
[1] always sanitize untrusted data, including data coming from
serialization.
[2] Both Nestor and me started a SQL injection detection rule (an
example of a rule that can help with untrusted data) which someday we
need to merge (and commit) into one.
> * DoubleCheckLockingRule
> I think a false positive, see the code sample, don't know whether it can be excluded from the pattern matching. Weird code it may be!
but for "double checking" a lock, i.e. the same condition before and
after the lock. While it looks sane and safe it is not - even if it's
unlikely (like many concurrency issues are) to cause problems in most
applications.
See article for full details: http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
> * Some typos in the report, some on the bounds of personal preference however.
*explaining* that one!"
As you may have guess a lot (well most) of the Gendarme developers are
not (native) English speakers ;-). Don't hesitate to report
grammatical errors (and typos too) to the mailing-list or, if you have
access, even commit fixes directly into SVN (or inside the wiki
documentation).
[afaik Nestor already fixed all reported typos/errors in SVN]
Andy Hume
> > On Apr 7, 5:21 am, "Andy Hume" <andyhum...@yahoo.co.uk> wrote:
>
> Do you agree with the result?
>
Andy
Andy Hume
> Here's a few answers (mostly) about the runners.
>
> > * Marked with SuppressMessage
> > So my first though it that it would be good if the
> [SuppressMessage]s could be respected. Local cases included:
>
> There's already a bug open for this feature:
> https://bugzilla.novell.com/show_bug.cgi?id=340931
>
> Honestly I had not considered (or dreamed) that current FxCop
> users would add Gendarme to their arsenal (at least so soon
> ;-). Also from a Mono-centric perspective we don't add
> attributes that differs from the FX (except for MonoTODO) so
> [SuppressMessage] is not very helpful (e.g. it's not even in
> the 1.1 framework) for a lot of cases (except where MS used
> it in some FX 3.0+ assemblies).
>
metadata when CODE_ANALYSIS is defined. I would have expected to never
see it in release assemblies. I have three configurations in my VS
projects the normal two plus CodeAnalysis which is like Release but has
that #define. That'd need another build of Mono for analysis use...
> The current (and very recent) mechanism to ignore defects is
> the "ignore list" (only supported by the console runner*)
> selected, mainly, because it's a better fit with Mono.
> However I made this an interface because I expected, at some
> point, to fix this bug. Right now I'm "tempted" to add a new
> attribute to rules to map Gendarme/ FxCop rules and extend
> the ignore logic to support them. E.g.
> [FxCopCompatibility ("CA1000")]
>
> Of course this would only be a partial compatibility since
> some Gendarme rules:
> - implements more than one FxCop rule;
> - split FxCop rules into several rules;
> - some differs in scope (like checking internals); and
> - some just don't exists (yet) in one or the other;
>
Yup I see the problems.
> (*) the wizard is not designed to expose every features
> possible to runners (e.g. ignore list). Hopefully a "full"(*)
> GUI runner would support them and allow the developer to
> add/remove/edit them.
>
> Anyway that's the way I see it today (subject to change
> without notice ;-) but I'm not sure if this will happen
> before Mono 2.0 or not...
>
I can cope with that. :-)
> > AvoidUnusedParametersRule : 4 defects
> > EnumsShouldDefineAZeroValueRule : 3 defects
> EnumsShouldUseInt32Rule :
> > 4 defects EnumNotEndsWithEnumOrFlagsSuffixRule : 1 defects
> > AvoidReturningArraysOnPropertiesRule : 4 defects For instance the
> > enums were for network protocol values so rule
> Int32/0=None/etc didn't apply to them. However maybe there
> isn't a direct correlation in the "enum end flags" case with
> "CA1711:IdentifiersShouldNotHaveIncorrectSuffix" supression...
>
> I'm not sure I understood this last bit correctly ? can you
> supply test cases ?
>
Nothing important. I was just giving some examples of the places I had
SuppressMessage applied.
> > * SWF runner
> > I haven't investigated this thoroughly but it appear that
> it one steps back (<Back) and then to completion again, then
> the target assembly is /not/ re-read even if it has changed.
> Instead it appears that the runner app must be restarted to
> pick up changes. I expected it to re-read the target
> assembly every time it generated a report.
>
> You're right, this is the current behavior. Actually I did
> that on purpose - so I could try different options without
> re-loading the assemblies every time (which is time
> consuming*). However I never considered your scenario which,
> in retrospect, makes lot of sense: any assembly that has
> changed should be reloaded. I'll try to fix this (without
> loosing the existing optimization).
>
That'd be good.
> (*) it even loads assemblies asynchronously so you can select
> your rules while they load (useful if/when loading all
> assemblies from Mono 2.0 profile :-)
>
Nice. FxCop does seem a bit slow sometimes. :-)
> Talking about the wizard there's a few other things I'd like to "fix"
> in the wizard before the next release:
> - use the treeview "like a tree" for the rules, i.e. add
> nodes to make it easy to [un]select all "Design" rules
> - merge the two last step into a single one (not 100% sure
> about this one right now)
>
Can I add what seems like a minor one, that the project's icon be set --
it makes it much easier to find the window one's looking for in the
taskbar etc when it has a recognizable icon.
> > * console runner
> > Now /does/ generate a report with only the target assembly
> on the command-line. :-) Though does it use a different rule
> set to the SWF runner?
>
> Yes. The console runner use the "default" rule set as
> configured inside "rules.xml". The rules.xml file shipped
> includes almost[1] everything, except the "Smells" rules [2],
> into "default". The Smells are included in the "self-test"
> set (everything but [1]), which Gendarme use to analyze itself.
>
> The wizard[3] runner simply list all rules it can find (in it's
> directory) and let the user select which one it wants. Right now that
> means:
> - all rules are selected by default
> - you have to re-select your rule "set" each time you restart
> the wizard [4]
>
> [1] except if the rule is considered too buggy [2] the smells
> are a lot slower than most rules and I, personally, prefer to
> run them less frequently (except when testing Gendarme
> itself)
> [3] the wizard is not meant as a full GUI runner and there's
> a lot of things I'd like in a GUI runner...
> [4] that being said maybe adding a button to "save as default
> set" to the wizard would be useful. thoughts ?
>
I'm not experienced enough yet to really comment, I guess one would
expect to see Save & Load Ruleset buttons. And I think I suggested a
set of pre-defined rulesets, I thought of a combobox type thing, but
Load would be ok if the installation included some ruleset files.
FxCop, of course, has a "project" file, which stores the target
assemblies (filename), the list of en-/disabled tests, and also
(attempts) to keep all of the previous violations so on a new run the
new violations can be highlighted (though on load it often says 'didn't
manage to load all the messages'). The 'project' always seems a bit
weird concept but might be necessary.
> More next time (I got a blog post to complete).
:-)
I'm begining to regret the one big post myself! :-,(
Andy
Andy Hume
On Apr 9, 2008 02:52 am Sebastien Pouliot wrote:
> a few more answers...
>
> On Apr 7, 5:21 am, "Andy Hume" <andyhum...@yahoo.co.uk> wrote:
> > * SecureGetObjectDataOverridesRule "SerializationFormatter
> is not a subset of LinkDemand permission set"
> > Bug 375368.
>
> This turned out to be a Cecil bug when enums are used inside
> security attributes. JB fixed this today (in Cecil) and I
> fixed the security rules tonight (at least all that use the
> PermissionSet of a
> SecurityDeclaration) to call Resolve. So it should be working
> with SVN HEAD (mcs and mono-tools)
>
Cool. It isn’t (mis-)reported by the current code. :-)
> > * Should the corresponding serialization constructor have a
> similar rule?
>
> No.
>
> > One generally sees it recommended that that method have the
> same demand applied. As creating a instance of the class but
> supplying bad values for the internal priviledged data could
> also create a security hole. Maybe in my cases since they're
> protected stops any reports?
>
> It's unrelated to protected ctors.
>
> The rule is concerned about untrusted code using
> serialization to leak private data out of (trusted) objects.
> From the constructor point of view everything is reversed,
> i.e. it would be trusted code with untrusted data.
>
> Now this is a very real problem[1] however, since the code is
> trusted, no CAS permission is going to help. Sadly I don't
> see a working way (at least for serialization[2]) to make a
> useful rule out of it (short of reporting all of them).
>
I'm *no* expert. Isn’t it similar:
• The allowed case:
App
=> Serialization classes in FCL
=> My library
• The case to be blocked:
Untrusted app -- manually creates binary stream which contains data that
looks like serialized state of my data and calls the SZ .ctor
=> My library
Therefore my library should demand to ensure that the caller has
permissions for serialization?
But as I say I'm no expert.
> [1] always sanitize untrusted data, including data coming
> from serialization.
> [2] Both Nestor and me started a SQL injection detection rule
> (an example of a rule that can help with untrusted data)
> which someday we need to merge (and commit) into one.
>
> > * DoubleCheckLockingRule
> > I think a false positive, see the code sample, don't know
> whether it can be excluded from the pattern matching. Weird
> code it may be!
>
> It's not a false positive. The rule does not check for double
> locks but for "double checking" a lock, i.e. the same
> condition before and after the lock. While it looks sane and
> safe it is not - even if it's unlikely (like many concurrency
> issues are) to cause problems in most applications.
> See article for full details:
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
>
Ok
> > * Some typos in the report, some on the bounds of personal
> preference however.
>
> "s/it's/its/ even my mother once a primary school teacher has trouble
> *explaining* that one!"
> lol
>
> As you may have guess a lot (well most) of the Gendarme
> developers are not (native) English speakers ;-). Don't
> hesitate to report grammatical errors (and typos too) to the
> mailing-list or, if you have access, even commit fixes
> directly into SVN (or inside the wiki documentation).
>
Will do. :-)
Andy
Sebastien Pouliot
> When only GetHashCode present, it says "Details: ... but does not implement 'System.Int32 GetHashCode()'". It's the reverse here.
> * AvoidLongParameterListsRule
> On a delegate it reports a violation on the BeginInvoke method, only the Invoke method should be checked IMHO -- that's the one under user control.
should be "checked" (or at least reported back to the user).
Looking at the code I found another issue (wrt constructors). This,
and the delegate case, makes me think the rule should inherit from
TypeRule (instead of MethodRule). It could also make the rule
simpler, less loops, to check overrides this way.
I'll chat with Nestor about this... but it will get fixed (one way or
another ;-)
> * Report format
> ** Move the "rules used" section to the bottom. It currently dominates, and I was always clicking it to navigate in the report. :-,( The navigation table should be put in its place include the violation counts also.
> ** Would be good for the sections to be collapsable nodes. Then one can close a section when its been dealt with.
I'll *try* (help welcome).
> * msbuild.exe build still working for me. :-)
keep it up to date but it's not my main development environment.
Actually I only tried msbuild for the first time today and it's...
well colorful ;-). Seriously I'm glad that such a tool exists since I
like to use the command-line even on Windows (and hate cygwin with
passion).
Hopefully my next email will complete the first round :-)
Keep comments coming!
Sebastien
Sebastien Pouliot
> * ConsiderConvertingMethodToPropertyRule
> Various reports of that, in many of the cases it wasn't a useful report. I'd SuppressMessages on some. Secondly there were cases where the was a SetXxxYyy method and no GetXxxYyy, so do that fix would then cause a "shouldn't only be a set property" violation.
if we find a GetX (not a IsX) we'll look for a SetX (that accept the
same type as the GetX returns) and add this information as extra
details to the (same) defect.
> Also in "class XxxxStream : Stream" cases, SetLength was highlighted. There is already a get_Length property, maybe in that case the rule is correct but I wonder whether here, and in general, violations shouldn't be reported for cases that are simply implementing base class members?
- out of the developer control (like the Stream case)
- better reported, once, where the problem starts (on not on every
inherited type that overrides it)
both should be fixed in r100489.
> * FX vs code rule set
> FxCop in /general/ reports things that affect the public interface to a class library, and not internal things -- it is "FX" cop after all. Gendarme is doing both, would it be good for the SWF runner to prompt with a choice of default rule-sets "public interface", or "all". It was a bit weird to get all these violations for a code base that ran clean in FxCop. But maybe that's always going to be the case when one runs a new tool on any code base. :-,)
closely related to a GSoC proposal. Actually the problem is reversed:
we need to check Mono internals while not reporting on visible stuff,
that we can't change to stay compatible with the FX. Anyway the
solution is probably near-identical (or should be ;-)
I think this covers the original set of question... (let me know if I
forgot one). I'll start at round 2 soon :)
Sebastien
Sebastien Pouliot
On Apr 10, 10:57 am, "Andy Hume" <andyhum...@yahoo.co.uk> wrote:
> > > * SWF runner
> > > I haven't investigated this thoroughly but it appear that
> > it one steps back (<Back) and then to completion again, then
> > the target assembly is /not/ re-read even if it has changed.
> > Instead it appears that the runner app must be restarted to
> > pick up changes. I expected it to re-read the target
> > assembly every time it generated a report.
>
> > You're right, this is the current behavior. Actually I did
> > that on purpose - so I could try different options without
> > re-loading the assemblies every time (which is time
> > consuming*). However I never considered your scenario which,
> > in retrospect, makes lot of sense: any assembly that has
> > changed should be reloaded. I'll try to fix this (without
> > loosing the existing optimization).
>
> That'd be good.
>
> > (*) it even loads assemblies asynchronously so you can select
> > your rules while they load (useful if/when loading all
> > assemblies from Mono 2.0 profile :-)
step #2 (add files) but rules were pre-loaded (at step #1, welcome) in
order for the list to be ready at step #3 (select rule).
I made some changes that makes assembly loading asynchronous too and
added a "last minute" check to ensure we got the latest assemblies
before starting the analysis. If you updated from SVN this weekend you
probably have the update.
[1] they are now in SVN ;-)
> Nice. FxCop does seem a bit slow sometimes. :-)
everything even if the rule can't be applied (i.e. like my last blog
entry). That can account for large differences when a small number of
rules (like just one) are used - but, on the whole, I think it's
slower mostly because it has more rules.
> > Talking about the wizard there's a few other things I'd like to "fix"
> > in the wizard before the next release:
> > - use the treeview "like a tree" for the rules, i.e. add
> > nodes to make it easy to [un]select all "Design" rules
> > - merge the two last step into a single one (not 100% sure
> > about this one right now)
>
> Can I add what seems like a minor one, that the project's icon be set --
> it makes it much easier to find the window one's looking for in the
> taskbar etc when it has a recognizable icon.
anymore ;-)
> I'm begining to regret the one big post myself! :-,(
idea to start new threads.
Sebastien
Néstor Salceda
> On Apr 7, 5:21 am, "Andy Hume" <andyhum...@yahoo.co.uk> wrote:
> > * AvoidLongParameterListsRule
> > On a delegate it reports a violation on the BeginInvoke method,
> > only the Invoke method should be checked IMHO -- that's the one
> > under user control.
I have just finished committing this check. Well, now, if a delegate
contains a lot of parameters, the rule show you only a violation. This
means:
delegate void MakeStuff (..{At least 6 parameters}..);
The rule only warns us once about the delegate, not about the Invoke
nor BeginInvoke methods (these methods are out of user control and are
compiler generated).
Well, the code is in the r101110 (What a binary revision! :P)
Thanks other time for your report.
Néstor.
Sandy Armstrong
wrote:
> > * DoubleCheckLockingRule
> > I think a false positive, see the code sample, don't know whether it can be excluded from the pattern matching. Weird code it may be!
>
> It's not a false positive. The rule does notcheckfordoublelocks
> unlikely (like many concurrency issues are) to cause problems in most
> applications.
> See article for full details:http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
justification for this rule is not very good. That article is Java-
specific, and does not apply the CLR. MSDN recommends double-checked
locking for implementing a thread-safe singleton, and even contains a
note about the very article you are referencing:
http://msdn2.microsoft.com/en-us/library/ms998558.aspx
The "good" singleton approach listed on this page...
http://www.mono-project.com/Gendarme.Rules.Concurrency
...unnecessarily hurts performance.
Anyway the only problem [1] with double-checked locking is that it
must be done the exact same way each time, and therefore it might be
considered fragile. But it is very well known and popular in the .NET
community, mostly because it is *the* recommended idiom for
implementing a thread-safe singleton!
Please consider removing this rule. I think it is confusing and
harmful.
Thanks,
Sandy
[1] http://www.yoda.arachsys.com/csharp/singleton.html
knocte
On Apr 28, 5:09 pm, Sandy Armstrong <sanfordarmstr...@gmail.com>
wrote:
day. Maybe the author of the rule missed that this issue doesn't apply
with the CLR (BTW, are we sure that Mono implements this the same way
as .NET?). I would vote, not for removing the rule, but for changing
it in order to *encourage* the use of double-checking.
Regards,
Andres
--
Sebastien Pouliot
> (BTW, are we sure that Mono implements this the same way
> as .NET?).
This is indeed the right question.
The same issue was discussed on another thread (a bit more recent) and
it's something that needs to be validated with the runtime guys (I do
remember Zoltan asking me to change a double-lock, albeit a long time
ago, in CryptoConfig).
I'll a bit short on time this week (got a presentation to finish/present
in two days and stretched a bit my free time this weekend for the rule
day) but feel free to bring the discussion to mono-devel, I'll join in
later :-)
Thanks,
Sebastien
knocte
On Apr 28, 5:53 pm, Sebastien Pouliot <sebastien.poul...@gmail.com>
wrote:
mono-devel but, regardless of this runtime topic, come to think of it,
we should change the rule anyway, because otherwise Gendarme would
rely on a bug (if Mono doesn't implement this like .NET, it's a bug,
because I guess it's included in the CLR spec). Besides, Gendarme is
supposed to target any .NET implementation, right? And doing a double-
check is just harmless in the case Mono doesn't mimic .NET's CLR (and
maybe it's better to have your own code conformant to .NET standards,
this way you won't have to modify it in the future when Mono gets
fixed).
Regards,
Andres
--