Invincible null: digging into nopCommerce source code
Tags: csharp, dotnet, opensource, programming
Published: 2024-03-21 11:15:41

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\.

![](https://import.viva64.com/docx/blog/1109_NopCommerce/image1.png)

## 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](https://github.com/nopSolutions/nopCommerce/tree/45d416f9936eb3a446d5193c61930f0a3b3c9a79) and used [PVS\-Studio](https://pvs-studio.com/en/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](https://pvs-studio.com/en/blog/terms/6694/)\-related bugs\. Here are the links to the cases we've previously covered in our articles:

* [the V3080 diagnostic rule cases](https://pvs-studio.com/en/blog/examples/v3080/)
* [the V3095 diagnostic rule cases](https://pvs-studio.com/en/blog/examples/v3095/)
* [the V3105 diagnostic rule cases](https://pvs-studio.com/en/blog/examples/v3105/)\.

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](https://pvs-studio.com/en/blog/posts/csharp/1032/)\.

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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0832/)\.

**Fragment 5**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/cpp/0968/)\.

**Fragment 9**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3005/) The 'priceModel\.OldPriceValue' variable is assigned to itself\. ProductModelFactory\.cs 503
* [V3005](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

```cpp
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](https://pvs-studio.com/en/pvs-studio/try-free/?utm_source=website&utm_medium=devto&utm_campaign=article&utm_content=1109) to check the project you're interested in\.