Closed GoogleCodeExporter closed 9 years ago
Original comment by sebastie...@gmail.com
on 21 Dec 2007 at 6:11
I claim this task.
Original comment by andreas....@gmail.com
on 25 Dec 2007 at 12:47
Original comment by jpo...@gmail.com
on 25 Dec 2007 at 1:55
I have tested some approaches to the problem and settled for a method blacklist
(MethodNotSupportedRule ). The rule checks each call/calli/callvirt/newobj
against a
blacklist (Ping.ctor, Process.set_PriorityClass).
If the code is ok, I'll start on the documentation.
Original comment by andreas....@gmail.com
on 26 Dec 2007 at 4:59
Attachments:
Nice code :) but this is no quite what's needed (at least in this case). What
you did
is very similar to what MoMA checks for (and could prove useful too).
* While the name of the rule match what your doing, it doesn't match the task
description. I.e. in both case the methods *are* supported on Mono, but only if
the
user is root (or has equivalent privileges). Please use the
FeatureRequiresRootPrivilegeOnUnixRule name (this will be a Portability rule).
* Both the Process and Ping classes are not sealed. So the rule wouldn't work
on:
public class MyPing : Ping {}
public class MyProcess : Process {}
Note: please add both cases to the unit tests.
* IIRC setting PriorityClass to Normal is ok for all users. Setting it to any
other
value requires root privileges. So you need to check the value assigned to the
property and warn if it's not Normal. Note: this is not a 100% safe solution
(e.g. if
a variable, not a constant, is assigned) but it will reduce false-positives.
* Small fix: Please change the old pattern
if ((method.Body == null) || (method.Body.Instructions == null))
return null;
to the newer one
if (method.HasBody)
return runner.RuleSuccess;
as this will become more important in future version of Gendarme (where an enum
will be returned).
* Small fix: For performance reason (rules can be executed thousands of times)
avoid
creating MessageCollection unless you really need it. Several rules have been
updated
to do this recently.
Thanks!
Original comment by sebastie...@gmail.com
on 26 Dec 2007 at 5:30
* The Ping part is now covert by a ModuleRule. It checks the TypeReferences for
a
reference to the Ping type.
* Although Process is not sealed, set_PriorityClass is (methods are sealed by
default). The call opcode references Process.set_PriorityClass, even if it is
invoked
on a subclass. I have added a unit test to confirm this.
* The rule now only warns if a value != ProcessPriorityClass.Normal is used.
(very
primitive check). But since ProcessPriorityClass.Normal is the default (?)
false-positives should be rare anyways.
* Fixed the skeleton code; added lazy MessageCollection creation; (I took the
original code from the NewLineLiteralRule (Portability). This rule still uses
the old
pattern.)
Original comment by andreas....@gmail.com
on 26 Dec 2007 at 8:07
Attachments:
Wow, your IModuleRule is a really nice trick :) and is actually a good match for
something (a different model) we discussed for executing rules at the last mono
summit. I'm impressed :)
However it lacks precision, since you cannot pinpoint which method(s) are using
the
Ping class.
I suggest that you
* keep the module rule to set a boolean flag (for a ping reference) and return
success at this level;
* add an additional check in the IMethodRule.CheckMethod part (if the ping flag
is
set) to see if the method create or use Ping
Note: I'll check the Process code later tonight.
Original comment by sebastie...@gmail.com
on 26 Dec 2007 at 10:03
Thanks :)
But my current version works without the TypeReferences. The constructor of
MyPing
calls Ping..ctor :)
Current status:
new Ping() <- warning
class MyPing : Ping {} <- warning
new MyPing() <- no warning
process.PriorityClass = PriorityClass.Normal <- no warning
process.PriorityClass = PriorityClass.AboveNormal etc <- warning
myProcess.PriorityClass = PriorityClass.AboveNormal <- warning
Original comment by andreas....@gmail.com
on 26 Dec 2007 at 11:53
Attachments:
Looks great :) but there's one case that I think should be handled as well ;-)
Here's the test case...
public void UsePing (Ping ping)
{
// e.g. Ping could be supplied from another assembly
// but Gendarme should still flag it's usage inside the analyzed assembly
ping.Send ("127.0.0.1");
}
[Test]
public void TestUsePing ()
{
// use an already created Ping instance
MethodDefinition method = GetTest ("UsePing");
Assert.IsNotNull (rule.CheckMethod (method, new MinimalRunner ()));
}
p.s. sorry I missed you earlier on IRC :(
Original comment by sebastie...@gmail.com
on 27 Dec 2007 at 1:36
Original comment by andreas....@gmail.com
on 27 Dec 2007 at 2:13
Attachments:
Gendarme.Rules.Portability
FeatureRequiresRootPrivilegeOnUnixRule
This will check for usage of restricted features.
* System.Net.NetworkInformation.Ping: This type can only be used by root on unix
systems. You can execute the ping command and parse it's result as an
alternative.
* System.Diagnostics.Process: The PriorityClass property can only be set to
Normal by
non root users. Do a platform check before assigning it:
if ( Environment.OSVersion.Platform != PlatformID.Unix )
process.PriorityClass = ProcessPriorityClass.AboveNormal;
Original comment by andreas....@gmail.com
on 27 Dec 2007 at 2:45
Great job!
Original comment by sebastie...@gmail.com
on 27 Dec 2007 at 2:47
Committed to SVN r91911/2.
Original comment by sebastie...@gmail.com
on 27 Dec 2007 at 3:56
Original issue reported on code.google.com by
sebastie...@gmail.com
on 21 Dec 2007 at 4:55