In 2021 we published several articles and showed you errors found in open-source projects. The year 2021 ends, which means it's time to present you the traditional top 10 of the most interesting bugs. Enjoy!
As in the 2020 article, we ranged warnings according to the following principles:
We must admit that there were few articles about C# projects check. The warnings in this list are often from the same projects. Somehow it happened that most of the warnings were taken from articles about DNN and PeachPie.
On the other hand, the errors found this year don't look alike — all the warnings were issued by different diagnostics!
With a heavy heart I crossed out warnings that were good but less interesting than others. Sometimes I had to cross out warnings for the sake of top variety. So, if you like reviews of the analyzer warnings, you can look at other articles. Who knows, maybe you'll be impressed by something I haven't written about. Comment with your own top 10 – I'll happily read them :).
We start our top with a warning from the PeachPie article:
using System_DateTime = System.DateTime;
internal static System_DateTime MakeDateTime(....) { .... }
public static long mktime(....)
{
var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
var local = MakeDateTime(hour, minute, second, month, day, year);
switch (daylightSaving)
{
case -1:
if (zone.IsDaylightSavingTime(local))
local.AddHours(-1); // <=
break;
case 0:
break;
case 1:
local.AddHours(-1); // <=
break;
default:
PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
break;
}
return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local,
....));
}PVS-Studio warnings:
These warning say the same thing, so I decided to unite them.
The analyzer says that the call results should be written somewhere. Otherwise they just don't make sense. Methods like AddHours don't change the source object. Instead, they return a new object, which differs from the source one by the number of hours written in the argument call. It's hard to say how severe the error is, but the code works incorrectly.
Such errors are often related to strings, but sometimes you can meet them when working with other types. They happen because developers misunderstand the work of the "changing" methods.
The 9th place is for the warning from the Ryujinx article:
public uint this[int index]
{
get
{
if (index == 0)
{
return element0;
}
else if (index == 1)
{
return element1;
}
else if (index == 2)
{
return element2;
}
else if (index == 2) // <=
{
return element3;
}
throw new IndexOutOfRangeException();
}
}PVS-Studio warning: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 26, 30. ZbcSetTableArguments.cs 26
Obviously, everything will be fine until someone wants to get the third element. And if they do so, an exception is thrown. That's okay, but why is there a never-running block with element3?
Surprisingly, situations caused by typos with numbers 0,1,2 are frequent in developing. There's a whole article about that — I highly recommend you read it. And we're moving on.
I took this warning from the PeachPie article mentioned above. It's fascinating that the code looks completely normal and not suspicious at all:
public static bool mail(....)
{
// to and subject cannot contain newlines, replace with spaces
to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
: "";
Debug.WriteLine("MAILER",
"mail('{0}','{1}','{2}','{3}')",
to,
subject,
message,
additional_headers);
var config = ctx.Configuration.Core;
....
}What's wrong with it? Everything looks okay. Assignments are made, then an overload of Debug.WriteLine is called. As a first argument, this overload takes... Wait! What's the correct order of the arguments here?
Well, let's look at the Debug.WriteLine declaration:
public static void WriteLine(string format, params object[] args);According to the signature, the format string should be passed as the first argument. In the code fragment, the first argument is "MAILER", and the actual format goes into the args array, which doesn't affect anything at all.
PVS-Studio warns that all formatting arguments are ignored: V3025: Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: 1st, 2nd, 3rd, 4th, 5th. Mail.cs 25
As a result, the output will simply be "MAILER" without any other useful information. But we'd like to see it :(.
The 7th place is again for the warning from PeachPie.
Often developers miss null checks. A particularly interesting situation is when a variable was checked in one place and wasn't in another (where it can also be null). Maybe developers forgot to do that or just ignored it. We can only guess whether the check was redundant or we need to add another check somewhere in code. The checks for null don't always require comparison operators: for example, in the code fragment below the developer used a null-conditional operator:
public static string get_parent_class(....)
{
if (caller.Equals(default))
{
return null;
}
var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
return tinfo.BaseType?.Name;
}Warning V3105: The 'tinfo' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Objects.cs 189
The developer thinks that the Type.GetTypeFromHandle(caller) call can return null. That's why they used "?." to call GetPhpTypeInfo. According to the documentation, it's possible.
Yes, "?." saves from one exception. If the GetTypeFromHandle call does return null, then null is also written to the tinfo variable. However, if we try to access the BaseType property, another exception is thrown. When I looked through the code, I came to the conclusion that another "?" is missing: return tinfo?.BaseType?.Name;
However, only developers can fix this problem. That's exactly what they did after we sent them a bug report. Instead of an additional null check they decided to explicitly throw an exception if GetTypeFromHandle returns null:
public static string get_parent_class(....)
{
if (caller.Equals(default))
{
return null;
}
// cannot be null; caller is either default or an invalid handle
var t = Type.GetTypeFromHandle(caller)
?? throw new ArgumentException("", nameof(caller));
var tinfo = t.GetPhpTypeInfo();
return tinfo.BaseType?.Name;
}We had to format the code for this article. You can find this method by following the link.
Sometimes it seems that time slows down. You feel like a whole week has passed, but it's been only one day. Well, on the 6th place we have a warning from the DotNetNuke article. The analyzer was triggered by the code where a week contains only one day:
private static DateTime CalculateTime(int lapse, string measurement)
{
var nextTime = new DateTime();
switch (measurement)
{
case "s":
nextTime = DateTime.Now.AddSeconds(lapse);
break;
case "m":
nextTime = DateTime.Now.AddMinutes(lapse);
break;
case "h":
nextTime = DateTime.Now.AddHours(lapse);
break;
case "d":
nextTime = DateTime.Now.AddDays(lapse); // <=
break;
case "w":
nextTime = DateTime.Now.AddDays(lapse); // <=
break;
case "mo":
nextTime = DateTime.Now.AddMonths(lapse);
break;
case "y":
nextTime = DateTime.Now.AddYears(lapse);
break;
}
return nextTime;
}PVS-Studio warning: V3139 Two or more case-branches perform the same actions. DotNetNuke.Tests.Core PropertyAccessTests.cs 118
Obviously, the function should return DateTime that corresponds to some point in time after the current one. Somehow it happened that the 'w' letter (implying 'week') is processed the same way as 'd'. If we try to get a date, a week from the current moment, we'll get the next day!
Note that there's no error with changing immutable objects. Still, it's weird that the code for branches 'd' and 'w' is the same. Of course, there's no AddWeeks standard method in DateTime, but you can add 7 days :).
The 5th place is taken by one of my favorite warnings from the PeachPie article. I suggest that you first take a closer look at this fragment and find an error here.
public static bool IsAutoloadDeprecated(Version langVersion)
{
// >= 7.2
return langVersion != null
&& langVersion.Major > 7
|| (langVersion.Major == 7 && langVersion.Minor >= 2);
}Where's the problem?
I think you've easily found an error here. Indeed easy, if you know where to look :). I have to admit that I tried to confuse you and changed formatting a bit. In fact, the logical construction was written in one line.
Now let's look at the version formatted according to operator priorities:
public static bool IsAutoloadDeprecated(Version langVersion)
{
// >= 7.2
return langVersion != null && langVersion.Major > 7
|| (langVersion.Major == 7 && langVersion.Minor >= 2);
}PVS-Studio warning V3080: Possible null dereference. Consider inspecting 'langVersion'. AnalysisFacts.cs 20
The code checks that the passed langVersion parameter is not null. The developer assumed that null could be passed when we call the IsAutoloadDeprecated method. Does the check save us?
No. If the langVersion variable is null, the value of the first part of the expression is false. When we calculate the second part, an exception is thrown.
Judging by the comment, either the priorities of operators were mixed up, or developers simply put the bracket incorrectly. By the way, this and other errors are gone (I believe) — we sent a bug report to the developers, and they quickly fixed them. You can see a new version of the IsAutoloadDeprecated function here.
We are almost close to the finalists. But before that — the 4th place. And here we have the warning from the last article about Umbraco. What do we have here?
public ActionResult<PagedResult<EntityBasic>> GetPagedChildren(....
int pageNumber,
....)
{
if (pageNumber <= 0)
{
return NotFound();
}
....
if (objectType.HasValue)
{
if (id == Constants.System.Root &&
startNodes.Length > 0 &&
startNodes.Contains(Constants.System.Root) == false &&
!ignoreUserStartNodes)
{
if (pageNumber > 0) // <=
{
return new PagedResult<EntityBasic>(0, 0, 0);
}
IEntitySlim[] nodes = _entityService.GetAll(objectType.Value,
startNodes).ToArray();
if (nodes.Length == 0)
{
return new PagedResult<EntityBasic>(0, 0, 0);
}
if (pageSize < nodes.Length)
{
pageSize = nodes.Length; // bah
}
var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
{
Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
};
return pr;
}
}
}PVS-Studio warning: V3022 Expression 'pageNumber > 0' is always true. EntityController.cs 625
So, pageNumber is a parameter that isn't changing inside the method. If its value is less than or equal to 0, we exit from the function. Further on, the code checks whether pageNumber is greater than 0.
Here we have a question: what value should we pass to pageNumber to make conditions pageNumber <= 0 and pageNumber > 0 false?
Obviously, there's no such value. If check pageNumber <= 0 is false, then pageNumber > 0 is always true. Is it scary? Let's look at the code after the always-true check:
if (pageNumber > 0)
{
return new PagedResult<EntityBasic>(0, 0, 0);
}
IEntitySlim[] nodes = _entityService.GetAll(objectType.Value,
startNodes).ToArray();
if (nodes.Length == 0)
{
return new PagedResult<EntityBasic>(0, 0, 0);
}
if (pageSize < nodes.Length)
{
pageSize = nodes.Length; // bah
}
var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
{
Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
};
return pr;Since the check in the beginning of this fragment is always true, the method always exits. And what about the code below? It contains a bunch of meaningful operations, but none of them is ever executed!
It looks suspicious. If pageNumber is less than or equal to 0, the default result is returned – NotFound(). Seems logical. However, if the parameter is greater than 0, we get... something that looks like the default result – new PagedResult<EntityBasic>(0, 0, 0). And how do we get a normal result? Unclear :(.
So, here are the finalists. The third place is for the V3122 diagnostic that haven't detected errors in open-source projects for a long time. Finally, in 2021 we checked DotNetNuke and found even 2 warnings V3122!
So, I present you the 3d place:
public static string LocalResourceDirectory
{
get
{
return "App_LocalResources";
}
}
private static bool HasLocalResources(string path)
{
var folderInfo = new DirectoryInfo(path);
if (path.ToLowerInvariant().EndsWith(Localization.LocalResourceDirectory))
{
return true;
}
....
}PVS-Studio warning: V3122 The 'path.ToLowerInvariant()' lowercase string is compared with the 'Localization.LocalResourceDirectory' mixed case string. Dnn.PersonaBar.Extensions LanguagesController.cs 644
The developers convert the path value to lowercase. Then, they check whether it ends in a string that contains uppercase characters – "App_LocalResources" (the literal returned from the LocalResourceDirectory property). Obviously, this check always returns false and everything looks suspicious.
This warning reminds me that no matter how many errors we've seen, there's always something that can surprise us. Let's go further :).
The second place is for an excellent warning from the ILSpy article written in the beginning of 2021:
private static void WriteSimpleValue(ITextOutput output,
object value, string typeName)
{
switch (typeName)
{
case "string":
output.Write( "'"
+ DisassemblerHelpers
.EscapeString(value.ToString())
.Replace("'", "\'") // <=
+ "'");
break;
case "type":
....
}
....
}V3038 The '"'"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. ICSharpCode.Decompiler ReflectionDisassembler.cs 772
Seems like the developer wanted to replace all single quote character occurrences with a string consisting of two characters: a backslash and a single quote character. However, due to peculiarities with escape sequences, the second argument is just a single quote character. Therefore, no replacing here.
I came up with two ideas:
So, we have finally reached the most interesting and unusual error of 2021. This error is from the DotNetNuke article mentioned above.
What's even more interesting, the error is primitive, but the human eye misses errors like this one without static analysis tools. Bold statement? Well then, try to find an error here (if there is one, of course):
private void ParseTemplateInternal(...., string templatePath, ....)
{
....
string path = Path.Combine(templatePath, "admin.template");
if (!File.Exists(path))
{
// if the template is a merged copy of a localized templte the
// admin.template may be one director up
path = Path.Combine(templatePath, "..\admin.template");
}
....
}Well, how's it going? I won't be surprised if you find an error. After all, if you know it exists, you'll quickly see it. And if you didn't find — well, no surprise either. It's not so easy to see a typo in the comment — 'templte' instead of 'template' :).
Joking. Of course there is a real error the disrupts the program's work. Let's look at the code again:
private void ParseTemplateInternal(...., string templatePath, ....)
{
....
string path = Path.Combine(templatePath, "admin.template");
if (!File.Exists(path))
{
// if the template is a merged copy of a localized templte the
// admin.template may be one director up
path = Path.Combine(templatePath, "..\admin.template");
}
....
}PVS-Studio warning: V3057 The 'Combine' function is expected to receive a valid path string. Inspect the second argument. DotNetNuke.Library PortalController.cs 3538
Here we have two operations to construct a path (the Path.Combine call). The first one is fine, but the second one is not. Clearly, in the second case, the developers wanted to take the 'admin.template' file not from the templatePath directory, but from the parent one. Alas! After they added ..\, the path became invalid since an escape sequence was formed: ..\admin.template.
Looks like the previous warning, right? Not exactly. Still, the solution is the same: add '@' before the string, or an additional '\'.
Well, since the first element of the collection has index 0, our collection should also have 0 place!
Of course, the error here is special, going beyond the usual top. And yet it's worth mentioning, since the error was found in the beloved Visual Studio 2022. And what does static analysis have to do with it? Well, let's find an answer to it.
My teammate, Sergey Vasiliev, found this problem and described it in article "How Visual Studio 2022 ate up 100 GB of memory and what XML bombs had to do with it". Here I'll briefly describe the situation.
In Visual Studio 2022 Preview 3.1, a particular XML file added to a project makes the IDE lag. Which means everything will suffer along with this IDE. Here's an example of such a file:
<?xml version="1.0"?>
<!DOCTYPE lolz [
<!ENTITY lol "lol">
<!ELEMENT lolz (#PCDATA)>
<!ENTITY lol1 "&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;">
<!ENTITY lol2 "&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;">
<!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
<!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
<!ENTITY lol5 "&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;">
<!ENTITY lol6 "&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;">
<!ENTITY lol7 "&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;">
<!ENTITY lol8 "&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;">
<!ENTITY lol9 "&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;">
<!ENTITY lol10 "&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;">
<!ENTITY lol11
"&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;">
<!ENTITY lol12
"&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;">
<!ENTITY lol13
"&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;">
<!ENTITY lol14
"&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;">
<!ENTITY lol15
"&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;">
]>
<lolz>&lol15;</lolz>As it turned out, Visual Studio was vulnerable to an XEE attack. Trying to expand all these lol entities, and IDE froze and ate up enormous amount of RAM. In the end, it just ate up all memory possible :(.
This problem was caused by the use of an insecurely configured XML parser. This parser allows DTD processing and doesn't set limitations on entities. My advice: don't read external files from unknown source with an XML parser. This will lead to a DoS attack.
Static analysis helps find such problems. By the way, PVS-Studio has recently introduced a new diagnostic to detect potential XEE — V5615.
We sent Visual Studio a bug report about that, and they fixed it in the new version. Good job, Microsoft! :)
Unfortunately, in 2021 we haven't written so many articles about real project checks. On the other hand, we wrote a number of other articles related to C#. You can find the links in the end of this article.
It was easy to choose interesting warnings for this top. But it wasn't easy to choose the 10 best ones since there were much more of them.
Rating them was also a hell of a task — the top is subjective, so don't take the places too close to heart :). One way or another, all these warnings are serious and once again remind us that we're doing the right thing.
Are you sure that your code doesn't have such problems? Are you sure that the errors don't hide between the lines? Perhaps, you can never be sure about that with a large project. However, this article shows that small (and not very small) errors can be found with static analyzer. That's why I invite you to try PVS-Studio on your projects.
Well, that's all. Happy New Year and see you soon!
I have collected several articles that you can catch up with during long winter evenings :).