Invincible null: digging into nopCommerce source code
Anastasiia Vorobeva
Posted on March 21, 2024
The nopCommerce project is a free open-source platform on ASP.NET Core to create online stores. Today we'll find out what ambiguities lurk in its code.
Few words about project
In software development, code quality plays a key role in the reliability, security, and performance of software products.
The nopCommerce project is an open-source eCommerce solution based on ASP.NET Core. It's one of the leading tools in the field. Even though projects may have an excellent reputation and wide distribution, it's still important to pay attention and analyze to code quality issues.
I took the code from this commit and used PVS-Studio 7.29 for analysis.
Here, in the article, you can explore not only error but also some moot analyzer warnings.
Enjoy the reading!
Brief aside
Our team frequently encounters NRE-related bugs. Here are the links to the cases we've previously covered in our articles:
These are just some of the diagnostic rules that indicate potential null dereference.
As with many other projects, most of the nopCommerce warnings mentioned here are NRE-related.
In some cases, a null dereference may be not a big issue. Developers may handle these cases or just catch an exception during testing. Let's be honest, tests may not cover every possible scenario of the program operation. An error may occur in code that is rarely executed, but users will eventually find it. I suppose the scenario is highly unwanted. That's why it's better to consider the project safety carefully to prevent such cases.
Suspicious Equals
Recently, we have often encountered errors related to the implementation of the Equals method. We've previously shown one such case in the article.
In fact, we've made such a mistake too when implementing the diagnostic rule. By the way, my colleagues have found the error in time and used it as the resource for a new diagnostic rule. It's about checking for the incorrect type in the overridden Equals.
The error in the example is common but still dangerous.
Fragment 1
public partial class CategoryKey
{
....
public bool Equals(CategoryKey y)
{
if (y == null)
return false;
if (Category != null && y.Category != null)
return Category.Id == y.Category.Id;
if (....)
{
return false;
}
return Key.Equals(y.Key);
}
public override bool Equals(object obj)
{
var other = obj as CategoryKey;
return other?.Equals(other) ?? false; // <=
}
}
The PVS-Studio warning: V3062 An object 'other' is used as an argument to its own method. Consider checking the first actual argument of the 'Equals' method. ImportManager.cs 3392
Look at the call to the Equals method in the overridden Equals body. You can see that the method is called for the other variable*.* It's also passed as a parameter. It means that the argument is compared to itself. I doubt that the developers have supposed the Equals method to operate like that.
To fix the error, we can pass this instead of other as the Equals argument.
Unused values
Unused values don't always cause errors that lead to exceptions or changes the program logic. However, these issues can arise as well. In any case, we need to avoid them. At least it'll make the code cleaner and may help prevent incorrect program behavior.
Below are code fragments that contain unused values.
Fragment 2
protected virtual async Task<....> PrepareCheckoutPickupPointsModelAsync(....)
{
....
if (amount > 0)
{
(amount, _) = await
_taxService.GetShippingPriceAsync(amount, customer);
amount = await
_currencyService.ConvertFromPrimaryStoreCurrencyAsync(amount,
currentCurrency);
pickupPointModel.PickupFee = await // <=
_priceFormatter.FormatShippingPriceAsync(amount, true);
}
//adjust rate
var (shippingTotal, _) = await
_orderTotalCalculationService.AdjustShippingRateAsync(point.PickupFee,
cart,
true);
var (rateBase, _) = await
_taxService.GetShippingPriceAsync(shippingTotal, customer);
var rate = await
_currencyService.ConvertFromPrimaryStoreCurrencyAsync(rateBase,
currentCurrency);
pickupPointModel.PickupFee = await // <=
_priceFormatter.FormatShippingPriceAsync(rate, true);
....
}
The PVS-Studio warning: V3008 The 'pickupPointModel.PickupFee' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 210, 203. CheckoutModelFactory.cs 210
After assigning a value to pickupPointModel.PickupFee, the property isn't used until the next time the value is overwritten. Such an assignment may make sense if the set property accessor has special logic. However, this is not the case here: pickupPointModel.PickupFee is a usual auto property. It turns out that the content of the then branch of the if statement doesn't affect the program logic in any way.
Fragment 3
public virtual async Task<....> GetOrderAverageReportLineAsync(....)
{
....
if (!string.IsNullOrEmpty(orderNotes))
{
query = from o in query
join n in _orderNoteRepository.Table on o.Id equals n.OrderId
where n.Note.Contains(orderNotes)
select o;
query.Distinct(); // <=
}
....
}
The PVS-Studio warning: V3010 The return value of function 'Distinct' is required to be utilized. OrderReportService.cs 342
You can use Distinct (the LINQ method) to delete repeating collection items. That's what the developers wanted to do in this code, but something went wrong. The Distinct method doesn't modify the collection for which it's called. So, if you don't use the return value of the method, the call is meaningless. This is exactly the case of the code snippet.
Most likely, the result of the Distinct execution should be assigned to the query variable.
Issues with null
Here are the classic errors (if they can be called that). There isn't much to add. Everyone knows NRE.
Fragment 4
public async Task<....> GetTaxTotalAsync(TaxTotalRequest taxTotalRequest)
{
....
var taxRates = transaction.summary
?.Where(....)
.Select(....)
.ToList();
foreach (var taxRate in taxRates) // <=
{
if (taxTotalResult.TaxRates.ContainsKey(taxRate.Rate))
taxTotalResult.TaxRates[taxRate.Rate] += taxRate.Value;
else
taxTotalResult.TaxRates.Add(taxRate.Rate, taxRate.Value);
}
....
}
The PVS-Studio warning: V3105 The 'taxRates' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. AvalaraTaxProvider.cs 113
When getting a value for taxRates, the transaction.summary property is accessed using the '?.' operator. The developer may have suggested that the property value could be null. If this is the case, null can be assigned to taxRates. After initializing taxRates, the variable is used as a collection and the collection is iterated over in foreach. If taxRates is null, NullReferenceException will be thrown. It happens because the GetEnumerator method is called on the collection implicitly.
It's worth noting that this error pattern is fairly common. We've already discussed it in the article.
Fragment 5
public async Task<....> GoogleAuthenticatorDelete(....)
{
....
//delete configuration
var configuration =
await _googleAuthenticatorService.GetConfigurationByIdAsync(model.Id);
if (configuration != null)
{
await _googleAuthenticatorService
.DeleteConfigurationAsync(configuration);
}
var customer = await _customerService
.GetCustomerByEmailAsync(configuration.Customer) ??
await _customerService
.GetCustomerByUsernameAsync(configuration.Customer);
....
}
The PVS-Studio warning: V3125 The 'configuration' object was used after it was verified against null. Check lines: 139, 135. GoogleAuthenticatorController.cs 139
The configuration variable is checked for null before the first use. However, it isn't checked for null on subsequent uses. Note that the GetConfigurationByIdAsync method used to get the variable value may return null. The developers may have thought that null wouldn't be returned here. Then it's not quite clear why the check for null is needed. Otherwise, a null dereference may cause an exception.
Fragment 6
public async Task<....> RefundAsync(.... refundPaymentRequest)
{
....
var clientReferenceInformation =
new Ptsv2paymentsClientReferenceInformation(Code: refundPaymentRequest
.Order
?.OrderGuid
.ToString(),
....);
....
if (refundPaymentRequest.Order.AllowStoringCreditCardNumber) // <=
{
var cardInformation = new Ptsv2paymentsidrefundsPaymentInformationCard(
Number: CreditCardHelper.RemoveSpecialCharacters(
_encryptionService
.DecryptText(refundPaymentRequest
.Order
?.CardNumber)),
ExpirationMonth: _encryptionService.DecryptText(refundPaymentRequest
.Order
?.CardExpirationMonth),
ExpirationYear: _encryptionService.DecryptText(refundPaymentRequest
.Order
?.CardExpirationYear));
....
}
....
var result = await apiInstance.RefundCaptureAsync(
refundCaptureRequest: requestObj,
id: refundPaymentRequest
.Order
?.CaptureTransactionId
??
refundPaymentRequest
.Order
?.AuthorizationTransactionId);
....
}
The PVS-Studio warning: V3095 The 'refundPaymentRequest.Order' object was used before it was verified against null. Check lines: 597, 600. CyberSourceService.cs 597
Pay attention to the refundPaymentRequest.Order property. It was checked for null six times and used seven times. Something doesn't add up. It's suspicious that refundPaymentRequest.Order is called without '?.' in the if statement. Maybe, the statement can't be null in the context of the method. Then it's worth deleting the check in other cases. If refundPaymentRequest.Order may be null, then sooner or later the RefundAsync call will cause NullReferenceException.
No more than one iteration.
How often do you use while instead of if? Rarely, I think.
Here is a very unusual example of using while.
Fragment 7
protected virtual TrieNode GetOrAddNode(ReadOnlySpan<char> key,
TValue value,
bool overwrite = false)
{
....
while (!node.IsDeleted && node.Children.TryGetValue(c, out nextNode))
{
var label = nextNode.Label.AsSpan();
var i = GetCommonPrefixLength(label, suffix);
// suffix starts with label?
if (i == label.Length)
{
// if the keys are equal, the key has already been inserted
if (i == suffix.Length)
{
if (overwrite)
nextNode.SetValue(value);
return nextNode;
}
// structure has changed since last; try again
break;
}
....
return outNode; // <=
}
....
}
The PVS-Studio warning: V3020 An unconditional 'return' within a loop. ConcurrentTrie.cs 230
The analyzer doubts whether the loop is correctly implemented. Let's find out what's wrong. The loop body contains the return operator issued without a condition. This is not always an error, as there may be the continue statements before return. Because of this, return won't necessarily be executed at the first loop iteration. However, there is no continue. Exiting the loop will always be done at the first iteration.
There are options for exiting the loop:
- with return, which is in the body of one of the if statements;
- with break (also in the if body);
- with return at the very end of the loop.
It's hard to say under what condition the loop should actually exit. But we can definitely say that a loop that has no more than one iteration looks very strange.
It may be a typo, and the continue statement should be used instead of break. There is even a hint in the comments: "try again".
Suspicious check
It's a classic copy-paste error.
Fragment 8
public async Task<bool?> IsConditionMetAsync(string conditionAttributeXml,
string selectedAttributesXml)
{
if (string.IsNullOrEmpty(conditionAttributeXml))
return null;
if (string.IsNullOrEmpty(conditionAttributeXml))
//no condition
return null;
....
}
The PVS-Studio warning: V3022 Expression 'string.IsNullOrEmpty(conditionAttributeXml)' is always false. AttributeParser.cs 499
Note the second if statement. It checks the value of the conditionAttributeXml field. This looks quite odd, since the previous if checked the same field. Obviously, in one of such cases, the selectedAttributesXml parameter should be the argument of the IsNullOrEmpty method.
Are they false positives?
We can't say that all analyzer warnings are necessarily false or, on the contrary, that all of them definitely indicate an error. There are the cases that will be discussed here.
If the analyzer is uncertain about the code, it will likely confuse the programmers who will maintain it. Such warnings are a good reason for refactoring. By the way, we have an article about it.
Fragment 9
protected virtual async Task
PrepareSimpleProductOverviewPriceModelAsync(Product product,
.... priceModel)
{
....
if (product.IsRental)
{
//rental product
priceModel.OldPrice = await _priceFormatter
.FormatRentalProductPeriodAsync(....);
priceModel.OldPriceValue = priceModel.OldPriceValue;
priceModel.Price = await _priceFormatter
.FormatRentalProductPeriodAsync(....);
priceModel.PriceValue = priceModel.PriceValue;
}
....
}
The PVS-Studio warnings:
- V3005 The 'priceModel.OldPriceValue' variable is assigned to itself. ProductModelFactory.cs 503
- V3005 The 'priceModel.PriceValue' variable is assigned to itself. ProductModelFactory.cs 505
The analyzer reports that priceModel.OldPriceValue and priceModel.PriceValue are assigned to themselves. Most likely, there is no error here, but the analyzer warning cannot be called false either. How did that happen? The point is that the code chunk is redundant. If you delete assignments to the priceModel.OldPriceValue and priceModel.PriceValue variables, the program logic won't change.
Now the question arises: have developers intended that properties should be assigned the current value or not? If so, why only these properties?
To reduce the number of questions, there are two things you can do:
- delete redundant assignments;
- leave a comment confirming that the assignments are correct.
Both options will make the code a little better :)
Fragment 10
public abstract partial class BaseNopValidator<TModel>
: AbstractValidator<TModel> where TModel : class
{
protected BaseNopValidator()
{
PostInitialize();
}
/// <summary>
/// Developers can override this method in
/// custom partial classes in order to add
/// some custom initialization code to constructors
/// </summary>
protected virtual void PostInitialize()
{
}
....
}
The PVS-Studio warning: V3068 Calling overrideable class member 'PostInitialize' from constructor is dangerous. BaseNopValidator.cs 20
We can't say that the warning indicates an error. Why? To answer the question, we need to understand the essence of the warning.
Let's imagine that we have a child of the BaseNopValidator class, for example, TestValidator, which overrides the PostInitialize method:
public class TestValidator : BaseNopValidator<object>
{
Logger _logger;
public TestValidator(Logger logger)
{
_logger = logger;
}
protected override void PostInitialize()
{
_logger.Log("Initializing");
}
}
If we create an object of the TestValidator type, NullReferenceException will be thrown. This will happen because when an object is created, the base class constructor will be executed first, and then the TestValidator constructor. So, when the Log method is called, the _logger field will be null.
However, none of the classes overrides the PostInitialize method in the project. Hence, no exception arises. But that's for now.
Conclusion
So, we can say that the code is quite clean but still not perfect :)
I think it would be great if the developers paid attention to the issues described in the article. Please note that the warnings I selected for the article are the most interesting. The code has more issues than described.
You can try PVS-Studio for free to check the project you're interested in.
Posted on March 21, 2024
Join Our Newsletter. No Spam, Only the good stuff.
Sign up to receive the latest update from our blog.
Related
