Nikita Panevin

Feb 10 2023

Tags:

#CSharp

Should we check libraries before using them? MudBlazor helps us find the answer

• Questionable call
• Unexpected NullReferenceException
• Unreasonable assignment
• Wrong priorities
• Summing up

There was a need in our company to use a library for Blazor components. We chose MudBlazor and checked its code quality before implementation. The result is a number of strange things and even a reproducible crash. Want to know more? Keep on reading this article.

MudBlazor is one of the most popular libraries for Blazor applications. In many ways, we chose it because of the number of components and the beauty of their implementation. The library's source code is written in C#, so we decided to analyze it before using it directly. For analysis, we took the source files of the v6.1.7 version. Next, let's talk about the errors found.

Questionable call

Issue 1

public EndSlopeSpline(....):base(....)
{
  m = new Matrix(n);
  gauss = new MatrixSolver(n, m);
  a = new double[n];
  b = new double[n];
  c = new double[n];
  d = new double[n];
  h = new double[n];

  CalcParameters(firstSlopeDegrees, lastSlopeDegrees);
  Integrate();                                            // <=
  Interpolate();
}

The PVS-Studio warning: V3010 The return value of function 'Integrate' is required to be utilized. EndSlopeSpline.cs 26

To understand the essence of the warning, we need to look at the implementation of the Integrate method:

public double Integrate()
{
  double integral = 0;
  for (int i = 0; i < h.Length; i++)
  {
    double termA = a[i] * h[i];
    double termB = b[i] * Math.Pow(h[i], 2) / 2.0;
    double termC = c[i] * Math.Pow(h[i], 3) / 3.0;
    double termD = d[i] * Math.Pow(h[i], 4) / 4.0;
    integral += termA + termB + termC + termD;
  }
  return integral;
}

In the method, the state of members does not change (e.g. fields or properties). Therefore, we can conclude that the only purpose of this method is to calculate and then return the value of the integral variable. This makes it pointless to call Integrate without using the return value.

Note that the CalcParameters and Interpolate methods (called next to Integrate) exactly change the states of a number of properties and fields.

Issue 2

internal void DateValueChanged(DateTime? value)
{
  _valueDate = value;

  if (value != null)
  {
    var date = value.Value.Date;

    // get the time component and add it to the date.
    if (_valueTime != null)
    {
      date.Add(_valueTime.Value);                    // <=
    }

    _filterDefinition.Value = date;
  }
  else
    _filterDefinition.Value = value;

  _dataGrid.GroupItems();
}

The PVS-Studio warning: V3010 The return value of function 'Add' is required to be utilized. Filter.cs 140

The warning is similar to the previous one. The return value of the Add method called on a DateTime variable was not used here. Most likely, the developer wanted to add the _valueTime.Value value to date. However, they did not take into account that the Add method for the DateTime object returns the result of the addition, and does not change the original object. The result is a useless call that was probably meant to have an impact.

Unexpected NullReferenceException

Issue 3

public static bool operator ==(ResizeOptions l, ResizeOptions r) 
                                                       => l.Equals(r);

public static bool operator !=(ResizeOptions l, ResizeOptions r) 
                                                       => !l.Equals(r);

The PVS-Studio warnings:

• V3115 Passing 'null' to '==' operator should not result in 'NullReferenceException'. ResizeOptions.cs 34
• V3115 Passing 'null' to '!=' operator should not result in 'NullReferenceException'. ResizeOptions.cs 35

When overloading the equality and inequality operators, it is worth considering the possibility that one of the operands may have the null value. Microsoft documentation gives similar recommendations.

However, in the implementation of '==' and '!=' overloading, the left operand is not checked for null. If the value of the l parameter is null, an exception of the NullReferenceException type will be thrown when the Equals method is called. The argument of the Equals method, by the way, is also dereferenced without checking:

public bool Equals(ResizeOptions other)
{
  if (ReportRate != other.ReportRate || ....)     // <=
  {
    return false;
  }
  ....
}

This behavior can be a very unpleasant surprise for the user if the class is public. This is exactly what ResizeOptions is.

To make sure there is a problem, let's try to compare an object of the ResizeOptions type with null.

Quite expectedly, we get an exception of the NullReferenceException type. If the right operand is null, the behavior will be similar.

Unreasonable assignment

Issue 4

protected override void OnParametersSet()
{
  if (....)
  {
    ....
    foreach (....)
    {
      if (firstTime)
      {
        ....
        gridValueY = verticalStartSpace;                   // <=
      }
      else
      {
        ....
        gridValueY = verticalStartSpace;                   // <=
      }
      gridValueY = yValue;                                 // <=
      ....
    }
  }
  else
  {
    ....
  }
  ....
}

The PVS-Studio warning: V3008 The 'gridValueY' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 191, 189. Line.razor.cs 191

Let's look at the nested if. Some value is assigned to the gridValueY variable in both branches of this operator. It is worth noting that the value is the same in both the first and the second case. It turns out that this assignment should have been taken out of the if. However, this is not the main problem: the value of gridValueY changes immediately after if, although it was not used.

The else block of the top-level if statement is similar to the then block:

foreach (....)
{
  if (firstTime)
  {
    ....
    gridValueY = verticalStartSpace;
  }
  else
  {
    ....
    gridValueY = verticalStartSpace;
  }
  ....
  gridValueY = boundHeight - (gridValueY + gridValue);   // <=
  ....
}

In this case, the value of gridValueY is also overwritten immediately after if, but the old value is used.

Wrong priorities

Issue 5

internal async Task<bool> StartResizeColumn(....)
{
  ....
  // In case resize mode is column, we have to find any column right
  // of the current one that can also be resized and is not hidden.

  var nextResizableColumn = _columns.Skip(_....) + 1)
                                    .FirstOrDefault(c =>
                                        c.Resizable ?? true && !c.Hidden); // <=
  ....
}

The PVS-Studio warning: V3177 The 'true' literal belongs to the '&&' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. DataGridColumnResizeService.cs 52

Let's look at the c.Resizable ?? true && !c.Hidden expression. It is worth saying that the priority of '??' is lower than that of '&&'. Based on this, if c.Resizable is null, then the null-coalescing operator will return the result of the true &&!c.Hidden operation. The "&&" operation with the true literal is pointless. Most likely, the correct option looks as follows:

(c.Resizable ?? true) && !c.Hidden

The difference between these variants will manifest itself if the values of c.Resizable and c.Hidden are true.

There is another thing indicating that a mistake was made. A line above has a comment that the column must be resizable (information about this can be found in c.Resizable) and not hidden (this can be found in c.Hidden). If c.Resizable is true, then the logic of the operation will be broken, since the value of c.Hidden will not affect the result in any way.

Summing up

It seems to me that each of the warnings we looked at may indicate an error that changes the logic of the program. In one of the cases, we were able to prove that. Before writing the article, I created an issue on GitHub. However, I still haven't heard back from the developers. I hope they will look into the problems listed and be able to improve the quality of their project code :).

If you want to try the analyzer used to check MudBlazor, you can download it here. By the way, PVS-Studio recently introduced the ability to analyze blocks of C# code in Blazor components. Some bugs found in such components are described in this article.

#CSharp

0

0

0

0

SHARE

Tags:

#CSharp

We can email you a selection of our best articles once a month