This is a brief story of how PVS-Studio collaborated with RavenDB. PVS-Studio is a static code analyzer. RavenDB is an open-source database. How can searching for errors in one project benefit both? Let's find out by going over the bug fixes and RavenDB developers' comments.
Before we get started, let me briefly introduce RavenDB and PVS-Studio. RavenDB is an open-source ACID NoSQL document-oriented database. PVS-Studio is a static code analyser for safety, security (SAST) and code quality control of programs written in C, C++, C# and Java.
The RavenDB team chose PVS-Studio for checking the project's source code. That sparked our collaboration idea: we agreed to analyze the project first, then examine a few of the most interesting bugs we found, and share them with our readers. So that happened.
We downloaded the project's source code and analyzed it. It's worth noting that the program's source code is clear and well-written – this is the merit of the development team, my hats off to them. After checking the code, we sent RavenDB the list of 10 warnings issued for the most suspicious code fragments.
We got a reply very soon. Paweł Pekról, the Head of Development at RavenDB, sent us feedback on PVS-Studio warnings and made a pull request to fix the described bugs.
RavenDB developers immediately responded to the suspicious code snippets and fixed the detected errors. Let's take a look at the most intriguing flaws and see how the developers fixed the code.
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
The HasFlag method always returns true for a flag with a zero value (see more details in the documentation): So, the part of the expression is always true and makes no sense.
The code fragment has been fixed by removing the check with the HasFlag method.
Here you can find the commit.
Error 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
The value of the args.Length expression cannot be both less than 4 and greater than 5, therefore this condition is always false. This error is easily fixed by replacing '&&' with '||'.
However, RavenDB developers were on the lookout and spotted another error in this code fragment. As it turned out, the upper bound for the number of arguments should be 6, not 5.
The correct code:
private static JsValue Spatial_Distance(JsValue self, JsValue[] args)
{
if (args.Length is < 4 or > 6)
throw new ArgumentException("....");
}Here are the commits: for the first error and for the second.
Error 3
public override DynamicJsonValue ToJson()fcg
{
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
Since the value of the DataDirectory property is written to json[nameof(DataDirectory)], then the value of the JournalStoragePath property should be written to json[nameof(JournalStoragePath)]. That's exactly the way RavenDB developers fixed this bug:
json[nameof(JournalStoragePath)] = JournalStoragePath;The commit.
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 priority of other operators in its left part. RevisionsCollectionConfiguration.cs 40
Here PVS-Studio issues a warning for the '??' operator's priority. And that's true, since the priority of the '??' operator is lower than the priority of the '^' operator. If MinimumRevisionAgeToKeep is null, the result of hashCode in the specified line will always be 0. Parenthesizing the '??' expression fixes the error.
By the way, RavenDB developers found another bug here: The MaximumRevisionsToDeleteUponDocumentUpdate property was missed when calculating hashCode.
The correct 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 commit.
Head of Development Paweł Pekról commented on this error:
Point 4 - gold :)
Error 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
The expr.Arguments.Count != 1 check is performed immediately after expr.Arguments.Count == 0 is checked. Obviously, if expr.Arguments.Count is 0, it is not 1. If the execution flow enters the upper if statement, an exception will be thrown. As a result, _sb.Append("this") will never be executed.
It seems that the code was just "lucky" during the testing: it wasn't executed, otherwise an exception would have been thrown. However, such bugs may go unnoticed during testing, but users will eventually run into them.
This has been fixed by removing the if statement that throws an exception.
Here you can find the commit.
The title states that it was a win-win collaboration. What does RavenDB benefit from? Every product needs to keep enhancing. This time the project enhanced not by adding new features, but by improving the quality of the source code. What does it mean for PVS-Studio? When studying the analysis report, we found several false positives. Our developers have already fixed some of them, other false positives are waiting their turn.
Head of Development RavenDB Paweł Pekról:
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 cannot but totally agree. There's no way for full code coverage. If we do care about the quality of the source code, it would be wise to use multiple tools that make the development process much easier.
Get rid of bugs and make your code safer with PVS-Studio! Store and process your data easily with RavenDB!
Enjoy using RavenDB and PVS-Studio!