{"id":382751,"date":"2024-06-29T04:28:31","date_gmt":"2024-06-29T04:28:31","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=382751"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=382751","title":{"rendered":"A Spy Undercover: PVS-Studio to Check ILSpy Source Code<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n

In PVS-Studio, we often check various compilers’ code and post the results in our blog. Decompiler programs, however, seem to be a bit neglected. To restore justice in this world, we analyzed the ILSpy decompiler’s source code. Let’s take a look at the peculiar things PVS-Studio found.<\/p>\n

<\/figcaption><\/figure>\n

Introduction<\/h3>\n

Probably almost every programmer used a decompiler at least once. The reasons could vary: to see how a method is implemented, to check if there is an error inside a library used, or to satisfy curiosity and look up some source code. At the mention of a decompiler, most .NET programmers will think of dotPeek or ILSpy. .NET Reflector is not as popular anymore. I remember when I first learned about these utilities and decompiled someone else’s library — a thought of espionage ran through my head. I was obviously not the only one thinking along these lines — I am sure ILSpy’s name is not accidental. I was curious what the spy is made of and wanted to reinforce it with a static analyzer. So I used the PVS-Studio analyzer on ILSpy’s source code and put together an article based on the most interesting and suspicious code fragments I found.<\/p>\n

To be honest, this article on ILSpy just sort of happened. Some of our clients are game development studios. This is one of the reasons why we try to make our tool as helpful and handy as possible for game developers, especially for those who employ Unity and Unreal Engine.<\/p>\n

While I know many clients who work with Unreal Engine, I don’t encounter that many Unity developers who use our analyzer. I want to encourage them to try the PVS-Studio analyzer, because I believe the Unity community can benefit from it. A cool way to demonstrate it would be to analyze a Unity-based open-source game and present the results. But the problem is — I could not find such a game! So please let me know of any ideas<\/a> you have for such games I could analyze with PVS-Studio. When I did try to look for a Unity-based open-source game, my search yielded unexpected results. On one website<\/a>, I found a list of Unity projects that for some mysterious reason included ILSpy. In PVS-Studio, we use a pool of projects to test our C# analyzer. That group includes ILSpy, so it’s odd that we do not have an article on this project yet. But since I failed to find a Unity project for analysis, let’s take a look at ILSpy.<\/p>\n

Here’s the project’s description on GitHub<\/a>: ILSpy is the open-source .NET assembly browser and decompiler.<\/p>\n

Since there was no information on whether ILSpy’s developers use a static analyzer, I am going to assume PVS-Studio is the first one. This makes my tests and research even more interesting. Now, without further discussion, let’s move on to analysis results.<\/p>\n

Replacement That Did Not Work<\/h3>\n

V3038<\/a> The ‘»‘»‘ argument was passed to ‘Replace’ method several times. It is possible that other argument should be passed instead. ICSharpCode.Decompiler ReflectionDisassembler.cs 772<\/p>\n

private static void WriteSimpleValue(ITextOutput output,                                      object value, string typeName) {   switch (typeName)   {     case \"string\":       output.Write(  \"'\"                    + DisassemblerHelpers                       .EscapeString(value.ToString())                       .Replace(\"'\", \"\\'\")                   \/\/ <=                    + \"'\");       break;     case \"type\":     ....   }   .... } <\/code><\/pre>\n
The author seems to be replacing all single quote character occurrences with a string consisting of two characters: a backslash and a single quote character. However, the developer missed a beat and by accident replaced the «‘» with itself, thus performing a meaningless operation. There is no difference between assigning a string variable a value of «‘» or «\\'» — either way the string is initialized with a single quote character. To include «\\'» in a string, use escape characters:  «\\\\'» or @»\\'». Thus, one can change the Replace<\/em> method call in the following way:<\/p>\n
Replace(\"'\", @\"\\'\") <\/code><\/pre>\n
Truth and Nothing but the Truth<\/h3>\n
Warning 1<\/strong><\/p>\n
V3022<\/a> Expression ‘negatedOp == BinaryOperatorType.Any’ is always true. ICSharpCode.Decompiler CSharpUtil.cs <\/p>\n
static Expression InvertConditionInternal(Expression condition) {   var bOp = (BinaryOperatorExpression)condition;    if (   (bOp.Operator == BinaryOperatorType.ConditionalAnd)       || (bOp.Operator == BinaryOperatorType.ConditionalOr))   {     ....   }   else if (   (bOp.Operator == BinaryOperatorType.Equality)            || (bOp.Operator == BinaryOperatorType.InEquality)             || (bOp.Operator == BinaryOperatorType.GreaterThan)            || (bOp.Operator == BinaryOperatorType.GreaterThanOrEqual)            || (bOp.Operator == BinaryOperatorType.LessThan)             || (bOp.Operator == BinaryOperatorType.LessThanOrEqual))   {     ....   }   else   {     var negatedOp = NegateRelationalOperator(bOp.Operator);     if (negatedOp == BinaryOperatorType.Any)                  \/\/ <=       return new UnaryOperatorExpression(....);     bOp = (BinaryOperatorExpression)bOp.Clone();     bOp.Operator = negatedOp;     return bOp;   } } <\/code><\/pre>\n
The analyzer warns that the negatedOp<\/em> variable always equals to the value of Any<\/em> from the BinaryOperatorType<\/em> enumeration. To verify this, let us take a look at the NegateRelationalOperator<\/em> method code that provides a value for the negatedOp<\/em> variable.<\/p>\n
public static BinaryOperatorType NegateRelationalOperator(BinaryOperatorType op) {   switch (op)   {     case BinaryOperatorType.GreaterThan:       return BinaryOperatorType.LessThanOrEqual;     case BinaryOperatorType.GreaterThanOrEqual:       return BinaryOperatorType.LessThan;     case BinaryOperatorType.Equality:       return BinaryOperatorType.InEquality;     case BinaryOperatorType.InEquality:       return BinaryOperatorType.Equality;     case BinaryOperatorType.LessThan:       return BinaryOperatorType.GreaterThanOrEqual;     case BinaryOperatorType.LessThanOrEqual:       return BinaryOperatorType.GreaterThan;     case BinaryOperatorType.ConditionalOr:       return BinaryOperatorType.ConditionalAnd;     case BinaryOperatorType.ConditionalAnd:       return BinaryOperatorType.ConditionalOr;   }   return BinaryOperatorType.Any; } <\/code><\/pre>\n
If by the NegateRelationalOperator<\/em> method call, the bOp.Operator<\/em>‘s value does not match any of the case<\/em> labels, the method returns BinaryOperatorType.Any<\/em>. You can see that the NegateRelationalOperator<\/em> method is called only when if<\/em> and if else<\/em> statements above the method are evaluated to false<\/em>. Moreover, if you look closely, you can notice that the if<\/em> and if<\/em> else<\/em> statements cover all case<\/em> labels the NegateRelationalOperator<\/em> method contains. By the time the NegateRelationalOperator<\/em> method is called, the bOp.Operator<\/em> does not satisfy any of the case<\/em> labels and the method returns the BinaryOperatorType.Any<\/em> value. As a result, negatedOp == BinaryOperatorType.Any<\/em> always evaluates to true<\/em>, and the next line returns the value from the method. In addition, we get unreachable code:<\/p>\n
bOp = (BinaryOperatorExpression)bOp.Clone(); bOp.Operator = negatedOp; return bOp; <\/code><\/pre>\n
By the way, the analyzer kindly issued a warning for this as well: V3142<\/a> Unreachable code detected. It is possible that an error is present. ICSharpCode.Decompiler CSharpUtil.cs 81<\/p>\n
Warning 2<\/strong><\/p>\n
V3022<\/a> Expression ‘pt != null’ is always true. ICSharpCode.Decompiler FunctionPointerType.cs 168<\/p>\n
public override IType VisitChildren(TypeVisitor visitor) {   ....   IType[] pt = (r != ReturnType) ? new IType[ParameterTypes.Length] : null;   ....   if (pt == null)     return this;   else     return new FunctionPointerType(       module, CallingConvention, CustomCallingConventions,       r, ReturnIsRefReadOnly,       pt != null ? pt.ToImmutableArray() : ParameterTypes,    \/\/ <=       ParameterReferenceKinds); } <\/code><\/pre>\n
Here everything is straightforward — the else<\/em> branch is executed if the pt<\/em> variable is not null<\/em>. So I don’t see the need in a ternary operator that checks the pt<\/em> variable for null<\/em>. I suspect that in the past the code did not contain the if<\/em>—else<\/em> statement and the first return<\/em> operator — then this check would have made sense. Right now it’s a good idea to remove the extra ternary operator: <\/p>\n
public override IType VisitChildren(TypeVisitor visitor) {   ....   IType[] pt = (r != ReturnType) ? new IType[ParameterTypes.Length] : null;   ....   if (pt == null)     return this;   else     return new FunctionPointerType(       module, CallingConvention, CustomCallingConventions,       r, ReturnIsRefReadOnly,       pt.ToImmutableArray(), ParameterReferenceKinds); } <\/code><\/pre>\n
Warning 3<\/strong><\/p>\n
V3022<\/a> Expression ‘settings.LoadInMemory’ is always true. ICSharpCode.Decompiler CSharpDecompiler.cs 394<\/p>\n
static PEFile LoadPEFile(string fileName, DecompilerSettings settings) {   settings.LoadInMemory = true;   return new PEFile(     fileName,     new FileStream(fileName, FileMode.Open, FileAccess.Read),     streamOptions: settings.LoadInMemory ?                           \/\/ <=       PEStreamOptions.PrefetchEntireImage : PEStreamOptions.Default,     metadataOptions: settings.ApplyWindowsRuntimeProjections ?          MetadataReaderOptions.ApplyWindowsRuntimeProjections :         MetadataReaderOptions.None   ); } <\/code><\/pre>\n
This case is similar to the previous one — we get an unnecessary ternary operator. The settings.LoadInMemory<\/em> property is set to true<\/em> and this value does not change until the ternary operator checks the value. Here’s the code for the property’s getter and setter:<\/p>\n
public bool LoadInMemory {   get { return loadInMemory; }   set {       if (loadInMemory != value)       {         loadInMemory = value;         OnPropertyChanged();       }   } } <\/code><\/pre>\n
It’s easy to exclude the unnecessary ternary operator and fix this code. There is probably no need to provide it here.<\/p>\n
Warning 4<\/strong><\/p>\n
V3022<\/a> Expression ‘ta’ is always not null. The operator ‘??’ is excessive. ICSharpCode.Decompiler ParameterizedType.cs 354<\/p>\n
public IType VisitChildren(TypeVisitor visitor) {   ....   if (ta == null)       return this;   else       return new ParameterizedType(g, ta ?? typeArguments);     \/\/ <= } <\/code><\/pre>\n
We can see the unnecessary null<\/em> coalescing<\/em> operator right away. When the ta<\/em> variable gets to the else<\/em> branch, it always has a value that is not null<\/em>. Consequently, the ?? operator is excessive.<\/p>\n
I got a total of 31 warnings under the number of V3022<\/a>.<\/p>\n
You Don’t Belong Here<\/h3>\n
Warning 1<\/strong><\/p>\n
V3025<\/a> Incorrect format. A different number of format items is expected while calling ‘Format’ function. Arguments not used: End. ICSharpCode.Decompiler Interval.cs 269<\/p>\n
public override string ToString() {   if (End == long.MinValue)   {     if (Start == long.MinValue)       return string.Format(\"[long.MinValue..long.MaxValue]\", End); \/\/ <=     else       return string.Format(\"[{0}..long.MaxValue]\", Start);   }   else if (Start == long.MinValue)   {     return string.Format(\"[long.MinValue..{0})\", End);   }   else   {     return string.Format(\"[{0}..{1})\", Start, End);   } } <\/code><\/pre>\n
In the first string.Format<\/em> method call, the format string does not match the arguments the method receives. The End<\/em> variable’s value, passed as an argument, cannot be inserted into the format string, because the string lacks the {0} format element. Following the method’s logic, this is not an error and the return<\/em> operator returns the string the code authors intended. This, of course, does not cancel the fact, that the code includes a useless string.Format<\/em> method call with an unused argument. It’s a good idea to fix this to make the code clean and easy to read.<\/p>\n
Warning 2<\/strong><\/p>\n
V3025<\/a> Incorrect format. A different number of format items is expected while calling ‘AppendFormat’ function. Arguments not used: angle. ILSpy.BamlDecompiler XamlPathDeserializer.cs 177<\/p>\n
public static string Deserialize(BinaryReader reader) {   ....   var sb = new StringBuilder();   ....   sb.AppendFormat(CultureInfo.InvariantCulture,                   \"A{0} {2:R} {2} {3} {4}\",                   size, angle, largeArc ? '1' : '0',                   sweepDirection ? '1' : '0', pt1);   .... } <\/code><\/pre>\n
In this case the angle<\/em> variable was left out. Though the developer passed the variable to the AppendFormat<\/em> method, the variable remains unused, because the format string contains two of {2} format elements and lacks the {1} format element. The authors probably intended to produce the following string:«A{0} {1:R} {2} {3} {4}»<\/em>.<\/p>\n
Double Standards<\/h3>\n
Warning 1<\/strong><\/p>\n
V3095<\/a> The ‘roslynProject’ object was used before it was verified against null. Check lines: 96, 97. ILSpy.AddIn OpenILSpyCommand.cs 96<\/p>\n
protected Dictionary<string, detectedreference=\"\"> GetReferences(....) {   ....   var roslynProject =  owner.Workspace                             .CurrentSolution                             .GetProject(projectReference.ProjectId);   var project = FindProject(owner.DTE.Solution                                  .Projects.OfType<envdte.project>(),                             roslynProject.FilePath);              \/\/ <=    if (roslynProject != null && project != null)                   \/\/ <=   .... } <\/code><\/pre>\n
First we get a roslynProject<\/em> object’s FilePath<\/em> property with no worry that the roslynProject<\/em> value may be null<\/em>, and in the next line we check roslynProject<\/em> for null<\/em>. Such code does not look safe and may produce a NullReferenceException<\/em> exception. To fix this code, one can use the FilePath<\/em> property along with a null-conditional operator. The second step is to plan for the FindProject<\/em> method to potentially get a null<\/em> value as the last parameter.<\/p>\n
Warning 2<\/strong><\/p>\n
V3095<\/a> The ‘listBox’ object was used before it was verified against null. Check lines: 46, 52. ILSpy FlagsFilterControl.xaml.cs 46<\/p>\n
public override void OnApplyTemplate() {   base.OnApplyTemplate();    listBox = Template.FindName(\"ListBox\", this) as ListBox;   listBox.ItemsSource = FlagGroup.GetFlags(....);         \/\/ <=    var filter = Filter;    if (filter == null || filter.Mask == -1)   {     listBox?.SelectAll();                                 \/\/ <=   } } <\/code><\/pre>\n
This case is similar to the previous example. First, we assign a value to the ItemsSource<\/em> property and do not check whether the listBox<\/em> variable contains null<\/em>. Then, several lines later, I can see the listBox<\/em> variable with the null-conditional operator. Note that between these two calls the listBox<\/em> variable did not get a new value.<\/p>\n
Our analyzer displayed 10 warnings with number V3095<\/a>. Here is a list of those warnings:<\/p>\n