collaboration that benefits everyone
A short story about cooperation between PVS-Studio and RavenDB. PVS-Studio is a static analyzer for code improvement. RavenDB – Open Source database. How does finding bugs in one project lead to the improvement of two at once? We will answer this question, and also look at the fixes of the problems found and the comments of the developers.
The beginning of the collaboration
Before the story begins, let me remind you about RavenDB And PVS Studio. RavenDB is an Open Source document-based ACID a database using the NoSQL approach. PVS-Studio is a static analysis tool aimed at improving the quality, security (SAST) and security of C, C++, C# and Java code.
The RavenDB team decided to use PVS-Studio to check the project’s code. We made a counter-proposal: first, conduct a one-time analysis, analyze a few of the most interesting errors and talk about them. In the end, we decided to do so.
We deflated sources and began to analyze. It is worth saying that the code is written quite well and is readable. The developers can only be commended for this. After verification, we sent the RavenDB team the 10 most suspicious code snippets, in our opinion.
The answer was not long in coming. head of development Pavel Pekrol sent us feedback on PVS-Studio warnings and created pull request with corrections of the described problems.
RavenDB developers responded very quickly to suspicious code fragments and fixed the bugs they found. Let’s go through the most interesting of them, analyze the errors and see how the developers fixed the code.
Bugs and fixes
Error 1
[Flags]
public enum TypeAttributes
{
....
NotPublic = 0,
....
}
private static bool CheckIfAnonymousType(Type type)
{
// hack: the only way to detect anonymous types right now
return type.IsDefined(typeof(CompilerGeneratedAttribute), false)
&& type.IsGenericType && type.Name.Contains("AnonymousType")
&& (type.Name.StartsWith("<>") || type.Name.StartsWith("VB$"))
&& type.Attributes.HasFlag(TypeAttributes.NotPublic); // <=
}
V3180 The ‘HasFlag’ method always returns ‘true’ because the value of ‘TypeAttributes.NotPublic’ is ‘0’, and it is passed as the method’s argument. ExpressionStringBuilder.cs 2117
Method HasFlag always returns a value trueif it takes a flag with the value 0 as an argument (see documentation). It turns out that part of the expression is always equal to true and doesn’t make sense.
The code was fixed by removing the check with HasFlag.
Mistake 2
private JsValue Spatial_Distance(JsValue self, JsValue[] args)
{
if (args.Length < 4 && args.Length > 5) // <=
throw new ArgumentException("....");
}
V3022 Expression ‘args.Length < 4 && args.Length > 5′ is always false. Probably the ‘||’ operator should be used here. ScriptRunner.cs 930
Expression value args.Length cannot be less than 4 and greater than 5 at the same time, hence this condition is always false. The fix is obvious – change ‘&&’ to ‘||’.
However, the RavenDB developers noticed another bug. As it turns out, the upper limit for the number of arguments should be 6, not 5.
The corrected code looks like this:
private static JsValue Spatial_Distance(JsValue self, JsValue[] args)
{
if (args.Length is < 4 or > 6)
throw new ArgumentException("....");
}
Commits with fixes: once, two.
Mistake 3
public override DynamicJsonValue ToJson()
{
var json = base.ToJson();
json[nameof(DataDirectory)] = DataDirectory;
json[nameof(JournalStoragePath)] = DataDirectory; // <=
json[nameof(SnapshotRestore)] = SnapshotRestore.ToJson();
return json;
}
V3127 Two similar code fragments were found. Perhaps this is a typo and ‘JournalStoragePath’ variable should be used instead of ‘DataDirectory’ RestoreResult.cs 24
Based on the fact that in json[nameof(DataDirectory)] property value is written DataDirectoryV json[nameof(JournalStoragePath)] property value should be written JournalStoragePath. This is how the developers fixed the error:
json[nameof(JournalStoragePath)] = JournalStoragePath;
Error 4
public override int GetHashCode()
{
unchecked
{
var hashCode = MinimumRevisionsToKeep?.GetHashCode() ?? 0;
hashCode = (hashCode * 397) ^
MinimumRevisionAgeToKeep?.GetHashCode() ?? 0; // <=
hashCode = (hashCode * 397) ^
Disabled.GetHashCode();
hashCode = (hashCode * 397) ^
PurgeOnDelete.GetHashCode();
return hashCode;
}
}
V3123 Perhaps the ‘??’ operator works in a different way than it was expected. Its priority is lower than the priority of other operators in its left part. RevisionsCollectionConfiguration.cs 40
Here PVS-Studio indicates that an error with the ‘??’ operator precedence is possible. Indeed, the precedence of the ‘^’ operator is higher than that of ‘??’. If MinimumRevisionAgeToKeep will matter nullthen the result of the calculation hashCode in the selected line will always be equal to 0. To correct it, it is enough to take an expression with ‘??’ in brackets.
By the way, the developers noticed another mistake. When calculating hashCode forgot about property MaximumRevisionsToDeleteUponDocumentUpdate.
Final corrected code:
public override int GetHashCode()
{
unchecked
{
var hashCode = MinimumRevisionsToKeep?.GetHashCode() ?? 0;
hashCode = (hashCode * 397) ^
(MinimumRevisionAgeToKeep?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^
(MaximumRevisionsToDeleteUponDocumentUpdate?.GetHashCode() ?? 0);
hashCode = (hashCode * 397) ^
Disabled.GetHashCode();
hashCode = (hashCode * 397) ^
PurgeOnDelete.GetHashCode();
return hashCode;
}
}
The words of the development manager Pavel Pekrol about this error:
Point 4 – gold 🙂
Mistake 5
public override void VisitMethod(MethodExpression expr)
{
if (expr.Name.Value == "id" && expr.Arguments.Count == 0)
{
if (expr.Arguments.Count != 1) // <=
{
throw new InvalidOperationException("....");
}
_sb.Append("this");
}
}
V3022 Expression ‘expr.Arguments.Count != 1’ is always true. JavascriptCodeQueryVisitor.cs 188
Examination expr.Arguments.Count != 1 executed immediately after verification expr.Arguments.Count == 0. Obviously, if the value expr.Arguments.Count is 0, then it is not equal to 1 either. If the thread of execution enters the top statement if, then an exception will always be thrown. Consequently, _sb.append(“this”) will never be executed.
It turns out that the tests were simply “lucky”: this code was not executed, otherwise an exception would have occurred. Such errors are scary in that they do not shoot in tests, but they can shoot at the user.
The fix was to remove the operator if with throwing an exception.
conclusions
The title of the article says that everyone wins. What won RavenDB? Naturally, each product is somehow developed and improved. This time, the improvement was not through new features, but through improving the quality of the source code. What did PVS-Studio win in? While reviewing the bug report, we found several false warnings. Some of them we have already fixed, and some are yet to be reached.
RavenDB Development Lead Pavel Pekrol:
Going over the list made me realize that devs are only humans and no matter how many tests you have there will always be bugs. The tool is very helpful in finding obvious and non-obvious bugs.
I can’t help but agree. The developer simply cannot fully cover the code with tests. If you care about the quality of the code and the project, then it is worth using various tools that will help make development easier.
Get rid of bugs and make your code safer with PVS Studio! Conveniently and easily store and process data with RavenDB!
Enjoy using RavenDB and PVS-Studio!
If you want to share this article with an English-speaking audience, please use the link to the translation: Artem Rovenskii. RavenDB and PVS-Studio: win-win collaboration.
