Ilya Gainulin , Oleg Andreev

Sep 13 2019

Tags:

#CSharp #DevOps

PVS-Studio in the clouds: Azure DevOps

Sep 13 2019

Author: Ilya Gainulin , Oleg Andreev

Let's Start the Configuration
Analysis Results
Conclusions

This is a second article, which focuses on usage of the PVS-Studio analyzer in cloud CI-systems. This time we'll consider the platform Azure DevOps - a cloud CI\CD solution from Microsoft. We'll be analyzing the ShareX project.

To get current information about it follow the updated documentation page "Using with Azure DevOps".

We'll need three components. The first is the PVS-Studio analyzer. The second is Azure DevOps, which we'll integrate the analyzer with. The third is the project that we'll check in order to demonstrate the abilities of PVS-Studio when working in a cloud. So let's get going.

PVS-Studio is a static code analyzer for finding errors and security defects. The tool supports the analysis of C, C++ and C# code.

Azure DevOps. The Azure DevOps platform includes such tools as Azure Pipeline, Azure Board, Azure Artifacts and others that speed up the process of creating software and improve its quality.

ShareX is a free app that lets you capture and record any part of the screen. The project is written in C# and is eminently suitable to show configuration of the static analyzer launch. The project source code is available on GitHub.

The output of the cloc command for the ShareX project:

Language	files	blank	comment	Code
C#	696	20658	24423	102565
MSBuild script	11	1	77	5859

In other words, the project is small, but quite sufficient to demonstrate the work of PVS-Studio together with the cloud platform.

Let's Start the Configuration

To start working in Azure DevOps, let's follow the link and press "Start free with GitHub".

Give the Microsoft application access to the GitHub account data.

You'll have to create a Microsoft account to complete your registration.

After registration, create a project:

Next, we need to move to "Pipelines" - "Builds" and create a new Build pipeline.

When asked where our code is located, we will answer - GitHub.

Authorize Azure Pipelines and choose the repository with the project, for which we'll configure the static analyzer's run.

In the template selection window, choose "Starter pipeline."

We can run static code analysis of the project in two ways: using Microsoft-hosted or self-hosted agents.

First, we'll be using Microsoft-hosted agents. Such agents are ordinary virtual machines that launch when we run our pipeline. They are removed when the task is done. Usage of such agents allows us not to waste time for their support and updating, but imposes certain restrictions, for example - inability to install additional software that is used to build a project.

Let's replace the suggested default configuration for the following one for using Microsoft-hosted agents:

# Setting up run triggers
# Run only for changes in the master branch
trigger:
- master

# Since the installation of random software in virtual machines  
# is prohibited, we'll use a Docker container, 
# launched on a virtual machine with Windows Server 1803
pool:
  vmImage: 'win1803'
container: microsoft/dotnet-framework:4.7.2-sdk-windowsservercore-1803
           
steps:
# Download the analyzer distribution
- task: PowerShell@2
  inputs:
    targetType: 'inline'
    script: 'Invoke-WebRequest 
               -Uri https://files.pvs-studio.com/PVS-Studio_setup.exe 
               -OutFile PVS-Studio_setup.exe'
- task: CmdLine@2
  inputs:
    workingDirectory: $(System.DefaultWorkingDirectory)
    script: |
# Restore the project and download dependencies
      nuget restore .\ShareX.sln
# Create the directory, where files with analyzer reports will be saved
      md .\PVSTestResults
# Install the analyzer
      PVS-Studio_setup.exe /VERYSILENT /SUPPRESSMSGBOXES 
/NORESTART /COMPONENTS=Core
#  Create the file with configuration and license information
         "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
          credentials 
    -u $(PVS_USERNAME) 
    -n $(PVS_KEY)

# Run the static analyzer and convert the report in html. 
    "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
         -t .\ShareX.sln  
         -o .\PVSTestResults\ShareX.plog
      "C:\Program Files (x86)\PVS-Studio\PlogConverter.exe" 
         -t html 
         -o .\PVSTestResults\ 
         .\PVSTestResults\ShareX.plog
    
# Save analyzer reports
- task: PublishBuildArtifacts@1
  inputs:
    pathToPublish: PVSTestResults
    artifactName: PVSTestResults

Note: according to the documentation, the container used has to be cached in the image of the virtual machine, but at the time of writing the article it's not working and the container is downloaded every time the task starts, which has a negative impact on the execution timing.

Let's save the pipeline and create variables which will be used for creating the license file. To do this, open the pipeline edit window and click "Variables" in the top right corner.

Then, add two variables - PVS_USERNAME and PVS_KEY, containing the user name and license key respectively. When creating the PVS_KEY variable don't forget to select "Keep this value secret" to encrypt values of the variable with a 2048-bit RSA key and to suppress the output of the variable value in the task performance log.

Save variables and run the pipeline by clicking "Run".

The second option to run the analysis - use a self-hosted agent. We can customize and manage self-hosted agents ourselves. Such agents give more opportunities to install software, needed for building and testing our software product.

Before using such agents, you have to configure them according to the instruction and install and configure the static analyzer.

To run the task on a self-hosted agent, we'll replace the suggested configuration with the following:

# Setting up triggers
# Run the analysis for master-branch
trigger:
- master

# The task is run on a self-hosted agent from the pool 'MyPool' 
pool: 'MyPool'

steps:
- task: CmdLine@2
  inputs:
    workingDirectory: $(System.DefaultWorkingDirectory)
    script: |
# Restore the project and download dependencies
      nuget restore .\ShareX.sln
# Create the directory where files with analyzer reports will be saved
      md .\PVSTestResults
# Run the static analyzer and convert the report in html. 
      "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
         -t .\ShareX.sln
         -o .\PVSTestResults\ShareX.plog
      "C:\Program Files (x86)\PVS-Studio\PlogConverter.exe"
         -t html
         -o .\PVSTestResults\
         .\PVSTestResults\ShareX.plog
# Save analyzer reports
- task: PublishBuildArtifacts@1
  inputs:
    pathToPublish: PVSTestResults
    artifactName: PVSTestResults

Once the task is complete, you can download the archive with analyzer reports under the "Summary" tab or you can use the extension Send Mail that enables to configure emailing or consider another convenient tool on Marketplace.

Analysis Results

Now let's look at some bugs found in the tested project, ShareX.

Excessive checks

To warm up, let's start with simple flaws in the code, namely, with redundant checks:

private void PbThumbnail_MouseMove(object sender, MouseEventArgs e)
{
  ....
  IDataObject dataObject 
    = new DataObject(DataFormats.FileDrop,
                     new string[] { Task.Info.FilePath });

  if (dataObject != null)
  {
    Program.MainForm.AllowDrop = false;
    dragBoxFromMouseDown = Rectangle.Empty;
    pbThumbnail.DoDragDrop(dataObject, 
        DragDropEffects.Copy | DragDropEffects.Move);
    Program.MainForm.AllowDrop = true;
  }
  ....
}

PVS-Studio warning: V3022 [CWE-571] Expression 'dataObject != null' is always true. TaskThumbnailPanel.cs 415

Let's pay attention to the check of the dataObject variable for null. Why is it here? dataObject cannot be null in this case, as it's initialized by a reference on a created object. As a result, we have an excessive check. Critical? No. Looks succinct? No. This check is clearly better being removed so as not to clutter the code.

Let's look at another fragment of code which we can comment in a similar way:

private static Image GetDIBImage(MemoryStream ms)
{
  ....
  try
  {
    ....
    return new Bitmap(bmp);
    ....
  }
  finally
  {
    if (gcHandle != IntPtr.Zero)
    {
      GCHandle.FromIntPtr(gcHandle).Free();
    }
  }
  ....
}
private static Image GetImageAlternative()
{
  ....
  using (MemoryStream ms = dataObject.GetData(format) as MemoryStream)
  {
    if (ms != null)
    {
      try
      {
        Image img = GetDIBImage(ms);
        if (img != null)
        {
          return img;
        }
      }
      catch (Exception e)
      {
        DebugHelper.WriteException(e);
      }
    }
  }
  ....
}

PVS-Studio warning: V3022 [CWE-571] Expression 'img != null' is always true. ClipboardHelpers.cs 289

In the GetImageAlternative method, the img variable is checked that it's not null right after a new instance of the Bitmap class is created. The difference from the previous example here is that we use the GetDIBImage method instead of the constructor to initialize the img variable. The code author suggests that an exception might occur in this method, but he declares only blocks try and finally, omitting catch. Therefore, if an exception occurs, the caller method GetImageAlternative won't get a reference to an object of the Bitmap type, but will have to handle the exception in its own catch block. In this case, the img variable won't be initialized and the execution thread won't even reach the img != null check but will get in the catch block. Consequently, the analyzer did point to an excessive check.

Let's consider the following example of a V3022 warning:

private void btnCopyLink_Click(object sender, EventArgs e)
{
  ....
  if (lvClipboardFormats.SelectedItems.Count == 0)
  {
    url = lvClipboardFormats.Items[0].SubItems[1].Text;
  }
  else if (lvClipboardFormats.SelectedItems.Count > 0)
  {
    url = lvClipboardFormats.SelectedItems[0].SubItems[1].Text;
  }
  ....
}

PVS-Studio warning: V3022 [CWE-571] Expression 'lvClipboardFormats.SelectedItems.Count > 0' is always true. AfterUploadForm.cs 155

Let's take a closer look at the second conditional expression. There we check the value of the read-only Count property. This property shows the number of elements in the instance of the collection SelectedItems. The condition is only executed if the Count property is greater than zero. It all would be fine, but in the external if statement Count is already checked for 0. The instance of the SelectedItems collection cannot have the number of elements less than zero, therefore, Count is either equal or greater than 0. Since we've already performed the Count check for 0 in the first if statement and it was false, there's no point to write another Count check for being greater than zero in the else branch.

The final example of a V3022 warning will be the following fragment of code:

private void DrawCursorGraphics(Graphics g)
{
  ....
  int cursorOffsetX = 10, cursorOffsetY = 10, itemGap = 10, itemCount = 0;
  Size totalSize = Size.Empty;

  int magnifierPosition = 0;
  Bitmap magnifier = null;

  if (Options.ShowMagnifier)
  {
    if (itemCount > 0) totalSize.Height += itemGap;
    ....
  }
  ....
}

PVS-Studio warning: V3022 Expression 'itemCount > 0' is always false. RegionCaptureForm.cs 1100

The analyzer noticed that the condition itemCount > 0 will always be false, as the itemCount variable is declared and at the same time assigned zero above. This variable isn't used anywhere up to the very condition, therefore the analyzer was right about the conditional expression, whose value is always false.

Well, let's now look at something really sapid.

The best way to understand a bug is to visualize a bug

It seems to us that a rather interesting error was found in this place:

public static void Pixelate(Bitmap bmp, int pixelSize)
{
  ....
  float r = 0, g = 0, b = 0, a = 0;
  float weightedCount = 0;

  for (int y2 = y; y2 < yLimit; y2++)
  {
    for (int x2 = x; x2 < xLimit; x2++)
    {
      ColorBgra color = unsafeBitmap.GetPixel(x2, y2);

      float pixelWeight = color.Alpha / 255;

      r += color.Red * pixelWeight;
      g += color.Green * pixelWeight;
      b += color.Blue * pixelWeight;
      a += color.Alpha * pixelWeight;

      weightedCount += pixelWeight;
    }
  }
  ....
  ColorBgra averageColor = new ColorBgra((byte)(b / weightedCount),
    (byte)(g / weightedCount), (byte)(r / weightedCount),
    (byte)(a / pixelCount));
  ....
}

I wouldn't like to show all the cards and reveal what our analyzer has found, so let's put it aside for a while.

By the name of the method, it is easy to guess what it is doing - you give it an image or a fragment of an image, and it pixelates it. The method's code is quite long, so we won't cite it entirely, but just try to explain its algorithm and explain what kind of a bug PVS-Studio managed to find.

This method receives two parameters: an object of the Bitmap type and the value of the int type that indicates the size of pixelation. The operation algorithm is quite simple:

1) Divide the received image fragment into squares with the side equal to the size of pixelation. For instance, if we have the pixelation size equal to 15, we'll get a square, containing 15x15=225 pixels.

2) Further, we traverse each pixel in this square and accumulate the values of the fields Red, Green, Blue and Alpha in intermediate variables, and before that multiply the value of the corresponding color and the alpha channel by the pixelWeight variable, obtained by dividing the Alpha value by 255 (the Alpha variable is of the byte type). Also when traversing pixels we sum up the values, written in pixelWeight into the weightedCount variable. The code fragment that executes the above actions is as follows:

ColorBgra color = unsafeBitmap.GetPixel(x2, y2);

float pixelWeight = color.Alpha / 255;

r += color.Red * pixelWeight;
g += color.Green * pixelWeight;
b += color.Blue * pixelWeight;
a += color.Alpha * pixelWeight;

weightedCount += pixelWeight;

By the way, note that if the value of the Alpha variable is zero, pixelWeight won't add to the weightedCount variable any value for this pixel. We'll need that in the future.

3) After traversing all pixels in the current square, we can make a common "average" color for this square. The code doing this looks as follows:

ColorBgra averageColor = new ColorBgra((byte)(b / weightedCount),
    (byte)(g / weightedCount), (byte)(r / weightedCount),
    (byte)(a / pixelCount));

4) Now when we got the final color and wrote it in the averageColor variable, we can again traverse each pixel of the square and assign it a value from averageColor.

5) Go back to the point 2 while we have unhandled squares.

Once again, the weightedCount variable isn't equal to the number of all pixels in a square. For example, if an image contains a completely transparent pixel (zero value in the alpha channel), the pixelWeight variable will be zero for this pixel (0 / 255 = 0). Therefore, this pixel won't effect formation of the weightedCount variable. It's quite logical - there's no point to take into account colors of a completely transparent pixel.

So it all seems reasonable - pixelation must work correctly. And it actually does. That's just not for png images that include pixels with values in the alpha channel below 255 and unequal to zero. Notice the pixelated picture below:

Have you seen the pixelation? Neither have we. Okay, now let's reveal this little intrigue and explain where exactly the bug is hiding in this method. The error crept into the line of the pixelWeight variable computation:

float pixelWeight = color.Alpha / 255;

The fact of the matter is that when declaring the pixelWeight variable as float, the code author implied that when dividing the Alpha field by 255, he'll get fractional numbers in addition to zero and one. This is where the problem hides, as the Alpha variable is of the byte type. When diving it by 255, we get an integer value. Only after that it'll be implicitly cast to the float type, meaning that the fractional part gets lost.

It's easy to explain why it's impossible to pixelate png images with some transparency. Since for these pixels values of the alpha channel are in the range 0 < Alpha < 255, the Alpha variable divided by 255 will always result in 0. Therefore, values of the variables pixelWeight, r, g, b, a, weightedCount will also always be 0. As a result, our averageColor will be with zero values in all channels: red - 0, blue - 0, green - 0, alpha - 0. By painting a square in this color, we do not change the original color of the pixels, as the averageColor is absolutely transparent. To fix this error, we just need to explicitly cast the Alpha field to the float type. Fixed version of the code line might look like this:

float pixelWeight = (float)color.Alpha / 255;

Well, it's high time to cite the message of PVS-Studio for the incorrect code:

PVS-Studio warning: V3041 [CWE-682] The expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. ImageHelpers.cs 1119

For comparison, let us cite the screenshot of a truly pixelated image, obtained on the corrected application version:

Potential NullReferenceException

public static bool AddMetadata(Image img, int id, string text)
{
  ....
  pi.Value = bytesText;

  if (pi != null)
  {
    img.SetPropertyItem(pi);
    return true;
  }
  ....
}

PVS-Studio warning: V3095 [CWE-476] The 'pi' object was used before it was verified against null. Check lines: 801, 803. ImageHelpers.cs 801

This code fragment shows that the author expected that the pi variable can be null, that is why before calling the method SetPropertyItem, the check pi != null takes place. It's strange that before this check the property is assigned an array of bytes, because if pi is null, an exception of the NullReferenceException type will be thrown.

A similar situation has been noticed in another place:

private static void Task_TaskCompleted(WorkerTask task)
{
  ....
  task.KeepImage = false;

  if (task != null)
  {
    if (task.RequestSettingUpdate)
    {
      Program.MainForm.UpdateCheckStates();
    }
    ....
  }
  ....
}

PVS-Studio warning: V3095 [CWE-476] The 'task' object was used before it was verified against null. Check lines: 268, 270. TaskManager.cs 268

PVS-Studio found another similar error. The point is the same, so there is no great need to cite the code fragment, the analyzer message will be enough.

PVS-Studio warning: V3095 [CWE-476] The 'Config.PhotobucketAccountInfo' object was used before it was verified against null. Check lines: 216, 219. UploadersConfigForm.cs 216

The same return value

A suspicious code fragment was found in the EvalWindows method of the WindowsList class, which returns true in all cases:

public class WindowsList
{
  public List<IntPtr> IgnoreWindows { get; set; }
  ....
  public WindowsList()
  {
    IgnoreWindows = new List<IntPtr>();
  }

  public WindowsList(IntPtr ignoreWindow) : this()
  {
    IgnoreWindows.Add(ignoreWindow);
  }
  ....
  private bool EvalWindows(IntPtr hWnd, IntPtr lParam)
  {
    if (IgnoreWindows.Any(window => hWnd == window))
    {
      return true;  // <=
    }

    windows.Add(new WindowInfo(hWnd));

    return true;  // <=
  }
}

PVS-Studio warning: V3009 It's odd that this method always returns one and the same value of 'true'. WindowsList.cs 82

In seems logical that if in the list named IgnoreWindows there is a pointer with the same name as hWnd, the method must return false.

The IgnoreWindows list can be filled either when calling the constructor WindowsList(IntPtr ignoreWindow) or directly through accessing the property as it's public. Anyway, according to Visual Studio, at the moment in the code this list is not filled. This is another strange place of this method.

Note. After talking to one of the ShareX developers, we found out that the EvalWindows method that always returns true value was intentionally written like that.

Unsafe call of event handlers

protected void OnNewsLoaded()
{
  if (NewsLoaded != null)
  {
    NewsLoaded(this, EventArgs.Empty);
  }
}

PVS-Studio warning: V3083 [CWE-367] Unsafe invocation of event 'NewsLoaded', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. NewsListControl.cs 111

Here a very nasty case might occur. After checking the NewsLoaded variable for null, the method, which handles an event, can be unsubscribed, for example, in another thread. In this case, by the time we get into the body of the if statement, the variable NewsLoaded will already be null. A NullReferenceException might occur when trying to call subscribers from the event NewsLoaded, which is null. It is much safer to use a null-conditional operator and rewrite the code above as follows:

protected void OnNewsLoaded()
{
  NewsLoaded?.Invoke(this, EventArgs.Empty);
}

The analyzer pointed to 68 similar fragments. We won't describe them all - they all have a similar call pattern.

Return null from ToString

Recently I've found out from an interesting article of my colleague that Microsoft doesn't recommend returning null from the overridden method ToString. PVS-Studio is well aware of this:

public override string ToString()
{
  lock (loggerLock)
  {
    if (sbMessages != null && sbMessages.Length > 0)
    {
      return sbMessages.ToString();
    }

    return null;
   }
 }

PVS-Studio warning: V3108 It is not recommended to return 'null' from 'ToSting()' method. Logger.cs 167

Why assigned if not used?

public SeafileCheckAccInfoResponse GetAccountInfo()
{
  string url = URLHelpers.FixPrefix(APIURL);
  url = URLHelpers.CombineURL(APIURL, "account/info/?format=json");
....
}

PVS-Studio warning: V3008 The 'url' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 197, 196. Seafile.cs 197

As we can see from the example, when declaring the url variable, it is assigned a value, returned from the method FixPrefix. In the next line, we clear the obtained value even without using it anywhere. We get something similar to dead code: it works, but doesn't effect the result. Most likely, this error is a result of a copy-paste, as such code fragments take place in 9 more methods. As an example, we'll cite two methods with a similar first line:

public bool CheckAuthToken()
{
  string url = URLHelpers.FixPrefix(APIURL);
  url = URLHelpers.CombineURL(APIURL, "auth/ping/?format=json");
  ....
}
....
public bool CheckAPIURL()
{
  string url = URLHelpers.FixPrefix(APIURL);
  url = URLHelpers.CombineURL(APIURL, "ping/?format=json");
  ....
}

Conclusions

As we can see, configuration complexity of automatic analyzer checks doesn't depend on a chosen CI-system. It took us literally 15 minutes and several mouse clicks to configure checking of our project code with a static analyzer.

In conclusion, we invite you to download and try the analyzer on your projects.

#CSharp #DevOps