Millions of people use web applications based on ASP.NET Core. This is why we enhanced the mechanics of how PVS-Studio analyzes such projects. How does PVS-Studio work now? Let's see! We checked several open-source projects and gathered the top 10 warnings in this article.
We often talk about the static analysis technologies we employ. One of them is code annotations. What is this and how can this be helpful?
It often happens that the analyzer cannot expand a method's body. For example, if that method is declared in a library whose source code is not available. And even if the code is open, sometimes the analyzer may have a problem making high-level conclusions about how the function works. That's where the analyzer needs some hints. Annotations are a simple and convenient way for us to help the analyzer understand how a method works. As PVS-Studio developers, we are the ones who can supply the analyzer with all the necessary information. Our annotations may describe values a method returns or explain which values one should or should not pass as arguments.
A while back we published a note on annotating Unity methods. We talked about the difficulties we encountered. For example, we crashed the editor by passing null as one of a method's arguments. Today's article is different.
We'll focus on two things: what enhancements we've implemented to help PVS-Studio analyze ASP.NET Core projects better — and what interesting problems we encountered in those projects along the way. For this article, we chose the most interesting cases that the analyzer found — no matter whether the analyzer found them only after we've added new annotations or could have found them without. By the way, the projects, that we used to test the analyzer on, were taken from here. The main selection criteria: the project is active at the time of analysis and the project compiles with no compilation errors.
We decided to annotate the most frequently used classes — same as we did with Unity. To figure out which classes need annotating the most, we used a utility that we wrote on Roslyn specifically for this purpose. You can learn more about this tool in the note we've mentioned earlier — the one about annotating Unity methods. Our utility helped us identify classes that were used in 17 ASP.NET Core projects that we selected:
These were the classes we needed to annotate first.
For example, let's take a look at the PhysicalFile(String, String) method from class ControllerBase. The documentation says that this method accepts an absolute file path and the file's contents type. It's also important to remember that this method has a return value. This is information is already sufficient to write an annotation, but you can find out even more.
There are 2 ways to get more information:
As a result, we received the following information:
After we've found all the details we need, we add all the acquired data in declarative form in the analyzer code. Now, when the analyzer encounters an annotated method, it knows how to process it.
The rating we've compiled is subjective — it's possible you see some errors differently. Maybe you would move some warnings up or down in our top 10. Please do tell us in the comments.
Time to get our hands dirty and check some projects!
We'll start with warnings issued for the Cloudscribe project. Yes, that's right — here we have not just one warning, but two. So, technically, today we are looking at 11 warnings, not 10. :)
protected override ValidationResult IsValid(....)
{
....
if (field != null)
{
....
// compare the value against the target value
if ((dependentValue == null && TargetValue == null) ||
(dependentValue != null &&
(TargetValue.Equals("*") ||
dependentValue.Equals(TargetValue))))
{
....
}
}
return ValidationResult.Success;
}The analyzer warns: V3080 Possible null dereference. Consider inspecting 'TargetValue'. RequiredWhenAttribute.cs 78
The analyzer noticed a that a null reference could be dereferenced. If the dependentValue variable is not null, and TargetValue is null, then you'll get the exception everyone loves so much — NullReferenceException.
Here's another example where the code uses a null reference to access data:
public async Task<IActionResult> Index(ConsentInputModel model)
{
// user clicked 'no' - send back the standard
// 'access_denied' response
if (model.Button == "no")
{
response = ConsentResponse.Denied;
}
// user clicked 'yes' - validate the data
else if (model.Button == "yes" && model != null)
{
....
}
....
}The analyzer warns: V3027 The variable 'model' was utilized in the logical expression before it was verified against null in the same logical expression. ConsentController.cs 87
In the code above, first, the model variable is used, and only after is its value checked for null. It needs to be the other way round.
It's worth noting that, in other projects, the analyzer also found errors that could cause a NullReferenceException type exception. However, they were in less significant scenarios, and we didn't get too many of them.
Let's move on to the next triggering. Here we'll look at the eShopOnContainers project.
private bool CheckSameOrigin(string urlHook, string url)
{
var firstUrl = new Uri(urlHook, UriKind.Absolute);
var secondUrl = new Uri(url, UriKind.Absolute);
return firstUrl.Scheme == secondUrl.Scheme &&
firstUrl.Port == secondUrl.Port &&
firstUrl.Host == firstUrl.Host;
}The analyzer warns: V3001 There are identical sub-expressions 'firstUrl.Host' to the left and to the right of the '==' operator. GrantUrlTesterService.cs 48
This error is easy to notice with the human eye. However, this requires knowing that the method contains an error. The analyzer found a code fragment that contains a number of comparisons. The last of them is an anomaly. The firstUrl object's Host property is compared to itself. It is difficult to determine how critical this oversight is, but, most likely, the application contains a logic violation somewhere due to the incorrect return value that this code fragment produces.
The most interesting cases are when two typical error patterns are involved: the last line effect and an error in comparison functions.
The analyzer issued this warning for the Cloudscribe project after we added ASP.NET annotations.
public async Task<IdentityResult> TryCreateAccountForExternalUser(....)
{
....
var user = new SiteUser
{
SiteId = Site.Id,
UserName = userName,
Email = email,
FirstName = info.Principal.FindFirstValue(ClaimTypes.GivenName),
LastName = info.Principal.FindFirstValue(ClaimTypes.Surname),
AccountApproved = Site.RequireApprovalBeforeLogin ? false : true
};
user.DisplayName = _displayNameResolver.ResolveDisplayName(user);
var result = await CreateAsync(user as TUser);
if(result.Succeeded)
{
result = await AddLoginAsync(user as TUser, info);
}
return result;
}The analyzer warns: V3156 The first argument of the 'AddLoginAsync' method is not expected to be null. Potential null value: user as TUser. SiteUserManager.cs 257
Let's take a closer look at this error.
It is possible that null is passed to AddLoginAsync when the method is called. The as operator will produce null if the conversion fails.
By the way, thanks to the fact that we annotated this method, the analyzer knows that null must not be passed as the first parameter.
Another interesting moment has to do with the user object of type SiteUser. This object is cast to TUser that is a generic parameter. Let's take a look at what a universal parameter is:
public class SiteUserManager<TUser> : UserManager<TUser> where TUser : SiteUserThe idea here is that the TUser item can be substituted here for SiteUser or any other type that inherits from SiteUser.
Let's take one more look at the code:
public async Task<IdentityResult> TryCreateAccountForExternalUser(....)
{
....
var user = new SiteUser
{
....
};
user.DisplayName = _displayNameResolver.ResolveDisplayName(user);
var result = await CreateAsync(user as TUser);
if(result.Succeeded)
{
result = await AddLoginAsync(user as TUser, info);
}
return result;
}So here's what happens here. Any time an instance of a SiteUser derived class — and not an instance of the SiteUser class — is passed to CreateAsync or AddLoginAsync as TUser, the methods receive null.
In this case there's a question. Why use a generic parameter if the code only works with one specific type? This could just be a peculiarity of this exact function, but that's not very obvious.
An error from the Piranha project came seventh. Let's play a little game and see how attentive you are. Try and see if you can find an error in the following code snippet.
public override async Task InitializeAsync()
{
using (var api = CreateApi())
{
// Import content types
new ContentTypeBuilder(api)
.AddType(typeof(BlogArchive))
.Build();
new ContentTypeBuilder(api)
.AddType(typeof(BlogPost))
.Build();
// Add site
var site = new Site
{
Id = SITE_ID,
Title = "Comment Site",
InternalId = "CommentSite",
IsDefault = true
};
await api.Sites.SaveAsync(site);
// Add archive
var blog = await BlogArchive.CreateAsync(api);
blog.Id = BLOG_ID;
blog.SiteId = SITE_ID;
blog.Title = "Blog";
blog.EnableComments = true;
blog.Published = DateTime.Now;
await api.Pages.SaveAsync(blog);
var news = await BlogArchive.CreateAsync(api);
news.Id = NEWS_ID;
news.SiteId = SITE_ID;
news.Title = "News";
blog.EnableComments = true;
news.Published = DateTime.Now;
await api.Pages.SaveAsync(news);
// Add posts
var blogPost = await BlogPost.CreateAsync(api);
blogPost.Id = BLOGPOST_ID;
blogPost.BlogId = BLOG_ID;
blogPost.Category = "The Category";
blogPost.Title = "Welcome To The Blog";
blogPost.Published = DateTime.Now;
await api.Posts.SaveAsync(blogPost);
var newsPost = await BlogPost.CreateAsync(api);
newsPost.Id = NEWSPOST_ID;
newsPost.BlogId = NEWS_ID;
newsPost.Category = "The Category";
newsPost.Title = "Welcome To The News";
newsPost.Published = DateTime.Now;
await api.Posts.SaveAsync(newsPost);
}
}I hope you were scrupulous when examining the code, yet did not get too tired. Now let's take a look at the shortened version of the code – and the analyzer's warning.
public override async Task InitializeAsync()
{
using (var api = CreateApi())
{
....
// Add archive
var blog = await BlogArchive.CreateAsync(api);
blog.Id = BLOG_ID;
blog.SiteId = SITE_ID;
blog.Title = "Blog";
blog.EnableComments = true;
blog.Published = DateTime.Now;
await api.Pages.SaveAsync(blog);
var news = await BlogArchive.CreateAsync(api);
news.Id = NEWS_ID;
news.SiteId = SITE_ID;
news.Title = "News";
blog.EnableComments = true; // <=
news.Published = DateTime.Now;
await api.Pages.SaveAsync(news);
....
}
}The analyzer warns: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'news' variable should be used instead of 'blog' CommentTests.cs 94
The code contains two blocks that are similar in structure. The analyzer points out a possible typo in the second block, in the following code line: blog.EnableComments = true. The author probably made this mistake when copy-pasting the first code block — and forgot to replace blog with news in that one place. It's slightly amusing that all programmers make these mistakes, no matter how much experience they have.
The next error we discovered was in the OrchardCore project.
public async Task ConfigureOAuthAsync(HttpRequestMessage request)
{
var container = await _siteService.GetSiteSettingsAsync();
var settings = container.As<TwitterSettings>();
var protrector = _dataProtectionProvider
.CreateProtector(TwitterConstants
.Features
.Twitter);
var queryString = request.RequestUri.Query;
if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret))
settings.ConsumerSecret =
protrector.Unprotect(settings.ConsumerSecret);
if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret))
settings.AccessTokenSecret =
protrector.Unprotect(settings.AccessTokenSecret);
....
}The analyzer warns: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'AccessTokenSecret' variable should be used instead of 'ConsumerSecret' TwitterClientMessageHandler.cs 51
The analyzer warns about two identical checks. The developer is accessing the settings object's ConsumerSecret property, although, most likely, the intent was to use AccessTokenSecret, because that's what really exists.
Here the developer's mistake changes the logic of how a security system works. Warnings that point to potential security breaches are more valuable than others.
So, we finally got to the top 5. The further we go, the more interesting it gets. A warning the analyzer issued for Squindex scored fifth.
public Task EnhanceAsync(UploadAssetCommand command)
{
try
{
using (var file = Create(new FileAbstraction(command.File),
ReadStyle.Average))
{
....
var pw = file.Properties.PhotoWidth;
var ph = file.Properties.PhotoHeight;
if (pw > 0 && pw > 0) // <=
{
command.Metadata.SetPixelWidth(pw);
command.Metadata.SetPixelHeight(ph);
}
....
}
return Task.CompletedTask;
}
catch
{
return Task.CompletedTask;
}
}The analyzer warns: V3001 There are identical sub-expressions 'pw > 0' to the left and to the right of the '&&' operator. FileTagAssetMetadataSource.cs 80
The analyzer reports that the operator has the same expressions to the left and to the right. Most likely, the if statement must make sure that the height and width are greater than 0. Instead, it checks the width twice. The program does not check the image size correctly, which means the program does not work as expected.
PVS-Studio issued this warning for the BTCPay Server project after we annotated methods.
public async Task<IActionResult> CalculateAmount(....)
{
try
{
....
while (true)
{
if (callCounter > 10)
{
BadRequest(); // <=
}
var computedAmount = await client.GetExchangeAmount(....);
callCounter++;
if (computedAmount < toCurrencyAmount)
{
....
}
else
{
return Ok(currentAmount);
}
}
}
catch (Exception e)
{
return BadRequest(new BitpayErrorModel()
{
Error = e.Message
});
}
}The analyzer warns: V3010 The return value of function 'BadRequest' is required to be utilized. ChangellyController.cs 72
PVS-Studio says that the call makes no sense if the returned value is not used. The analyzer cannot expand the BadRequest method's body. However, thanks to annotations, the analyzer received information about the need to use the value that was returned.
Looks like someone missed the return statement here. This oversight can disturb the logic of the CalculateAmount method. The missed return in BadRequest causes at least a big number of iterations — or makes the application crash.
Well, we are almost at the top. We are about to look at the top 3 warnings. Third is the warning that the analyzer issued for the Squidex project.
private static AssetFolderDto CreateLinks(AssetFolderDto response,
Resources resources)
{
var values = new { app = resources.App, id = response.Id };
if (resources.CanUpdateAsset)
{
response.AddPutLink("update", resources.Url<AssetFoldersController>(x =>
nameof(x.PutAssetFolder), values));
response.AddPutLink("move", resources.Url<AssetFoldersController>(x =>
nameof(x.PutAssetFolderParent), values));
}
if (resources.CanUpdateAsset)
{
response.AddDeleteLink("delete", resources.Url<AssetFoldersController>(x =>
nameof(x.DeleteAssetFolder), values));
}
return response;
}The analyzer warns: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 50, 57. AssetFolderDto.cs 50
The analyzer detected two if statements that have the same condition statements and that are next to each other in code. There is clearly something wrong here. I think everyone would expect to see resources.CanDeleteAsset in the second if statement. This property is indeed available and used in a similar method.
private static AssetDto CreateLinks(AssetDto response,
Resources resources)
{
....
if (resources.CanUpdateAsset)
....
if (resources.CanUploadAsset)
....
if (resources.CanDeleteAsset)
....
....
}This time, silver goes to the Squidex project and to the error the analyzer found there.
private IEnumerable<IMigration?> ResolveMigrators(int version)
{
yield return serviceProvider.GetRequiredService<StopEventConsumers>();
// Version 06: Convert Event store. Must always be executed first.
if (version < 6)
{
yield return serviceProvider.GetRequiredService<ConvertEventStore>();
}
// Version 22: Integrate Domain Id.
if (version < 22)
{
yield return serviceProvider.GetRequiredService<AddAppIdToEventStream>();
}
// Version 07: Introduces AppId for backups.
else if (version < 7) // <=
{
yield return serviceProvider
.GetRequiredService<ConvertEventStoreAppId>();
}
// Version 05: Fixes the broken command architecture and requires a
// rebuild of all snapshots.
if (version < 5)
{
yield return serviceProvider.GetRequiredService<RebuildSnapshots>();
}
else
{
// Version 09: Grain indexes.
if (version < 9)
{
yield return serviceProvider.GetService<ConvertOldSnapshotStores>();
}
....
}
// Version 13: Json refactoring
if (version < 13)
{
yield return serviceProvider.GetRequiredService<ConvertRuleEventsJson>();
}
yield return serviceProvider.GetRequiredService<StartEventConsumers>();
}The analyzer warns: V3022 Expression 'version < 7' is always false. MigrationPath.cs 55
Note that in the code above, "...." stands for a few more checks — I just skipped them to improve readability. You can find the method's complete code here.
The analyzer points out that the version < 7 condition is always false. The execution flow never reaches the else branch because version < 22 always includes everything that meets the version < 7 condition. Such errors are difficult to find when writing code — especially when there are many conditional branches. But when the analyzer points them out — they become obvious.
The error that the analyzer found in the OrchardCore project is slightly more interesting, so it takes gold in our rating.
public async ValueTask<Completion> WriteToAsync(....)
{
....
if (displayFor != null)
{
....
}
else if (editFor != null)
{
....
}
else if (adminFor != null)
{
....
}
else if (removeFor != null)
{
contentItem = removeFor;
var metadata =
await contentManager
.PopulateAspectAsync<ContentItemMetadata>(removeFor);
if (metadata.RemoveRouteValues != null)
{
if (routeValues != null)
{
foreach (var attribute in routeValues)
{
metadata.RemoveRouteValues.Add(attribute.Key, attribute.Value);
}
}
customAttributes["href"] = urlHelper
.Action(metadata.RemoveRouteValues["action"]
.ToString(), metadata.RemoveRouteValues);
}
}
else if (createFor != null)
{
contentItem = createFor;
var metadata =
await contentManager
.PopulateAspectAsync<ContentItemMetadata>(createFor);
if (metadata.CreateRouteValues == null)
{
if (routeValues != null)
{
foreach (var attribute in routeValues)
{
metadata.CreateRouteValues.Add(attribute.Key, attribute.Value);
}
}
customAttributes["href"] = urlHelper
.Action(metadata.CreateRouteValues["action"]
.ToString(), metadata.CreateRouteValues);
}
}
....
}The analyzer warns: V3080 Possible null dereference. Consider inspecting 'metadata.CreateRouteValues'. ContentAnchorTag.cs 188
The analyzer found code that can execute null-reference access.
Although I've shortened the original code before posting the example above, the fragment is still large. Let's simplify it a bit more:
public async ValueTask<Completion> WriteToAsync(....)
{
....
if (metadata.CreateRouteValues == null)
{
if (routeValues != null)
{
foreach (var attribute in routeValues)
{
metadata.CreateRouteValues.Add(attribute.Key, attribute.Value);
}
}
....
}
....
}We can see a check there: if the metadata.CreateRouteValues property is null, the Add method is called for it. Of course, that's a mistake. The project's code contains many more similar code blocks. For a better understanding, I left one of them in the large example above. In all cases except for the last one, the != null check precedes them. The developer must have made a typo when copying the code.
Annotating ASP.NET Core methods obviously had a positive effect on how PVS-Studio analyzes projects that use ASP.NET Core. Annotating methods is useful not just to get new useful warnings, but also helps eliminate false positives.
We provided annotations only for select classes — those that we found to be frequently used in the projects we've gone through. Know of any ASP.NET Core projects where the analyzer does not issue a warning or works incorrectly? Please leave us a comment below! Especially if you have a few cases where annotations would really come in handy.
This rating is another proof that static analysis really does help find interesting errors in projects. This relates not just to ASP projects, but also to everything else. And what do you think? Can PVS-Studio find something in your projects? I invite you to visit our website! Go ahead and try PVS-Studio on your project :).