PVS-Studio goes to the clouds: Azure DevOps

This is the second article about using the PVS-Studio static analyzer in cloud CI systems, and this time we will consider the Azure DevOps platform - a cloud CI \ CD solution from Microsoft. As an analyzed project this time, consider ShareX.

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

PVS-Studio is a static code analyzer for searching for errors and security defects. Performs code analysis in C, C ++, C # and Java.

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

ShareX is a free application that allows you to capture and record any part of the screen. The project is written in C # and is great for demonstrating how to run the static analyzer. The source code for the project is available on GitHub .

The output of the cloc command for the ShareX project:
In other words, the project is small, but quite sufficient to demonstrate the work of PVS-Studio in conjunction with a cloud platform.

Let's set up

To get started in Azure DevOps, click on the link and click the “Start free with GitHub” button.

Grant the Microsoft application access to the data of the GitHub account.

To complete the registration will have to create a Microsoft account.

After registration, create a project:

Next, we need to go to the “Pipelines” - “Builds” section and create a new Build pipeline

To the question where our code is located, we will answer - GitHub.

We authorize the Azure Pipelines application and select the repository with the project for which we will configure the launch of the static analyzer

In the template selection window, select “Starter pipeline”.

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

In the first version, we will use Microsoft-hosted agents. Such agents are ordinary virtual machines that start when we start our pipeline and are deleted after the end of the task. Using such agents allows you not to waste time supporting and updating them, but imposes some restrictions, for example, the impossibility of installing additional software that is used to build the project.

Replace our default configuration with the following for using Microsoft-hosted agents:

#    #      master- trigger: - master #         # ,   Docker-, #      Windows Server 1803 pool: vmImage: 'win1803' container: microsoft/dotnet-framework:4.7.2-sdk-windowsservercore-1803 steps: #    - task: PowerShell@2 inputs: targetType: 'inline' script: 'Invoke-WebRequest -Uri https://files.viva64.com/PVS-Studio_setup.exe -OutFile PVS-Studio_setup.exe' - task: CmdLine@2 inputs: workingDirectory: $(System.DefaultWorkingDirectory) script: | #      nuget restore .\ShareX.sln #  ,        md .\PVSTestResults #   PVS-Studio_setup.exe /VERYSILENT /SUPPRESSMSGBOXES /NORESTART /COMPONENTS=Core #        "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" credentials -u $(PVS_USERNAME) -n $(PVS_KEY) #        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 #    - task: PublishBuildArtifacts@1 inputs: pathToPublish: PVSTestResults artifactName: PVSTestResults 

Note: according to the documentation , the container used must be cached in the image of the virtual machine, but at the time of writing this article does not work and the container is downloaded every time the task starts, which negatively affects the execution time.

Save the pipeline and create the variables that will be used to create the license file. To do this, open the pipeline editing window and in the upper right corner click the “Variables” button.

Add two variables - PVS_USERNAME and PVS_KEY , containing the username and license key, respectively. When creating the PVS_KEY variable, do not forget to check the “Keep this value secret” item for encrypting the variable value with a 2048-bit RSA key, as well as suppressing the output of the variable value to the task execution log.

We save the variables and start the pipeline with the “Run” button.

The second option to run the analysis is to use a self-hosted agent. Self-hosted agents are agents that we configure and manage ourselves. Such agents provide more opportunities for installing software, which is necessary for the assembly and testing of our software product.

Before using such agents, they must be configured according to the instructions , and a static analyzer must be installed and configured .

To start the task on the self-hosted agent, we replace the proposed default configuration with the following:

 #    #    master- trigger: - master #    self-hosted    'MyPool' pool: 'MyPool' steps: - task: CmdLine@2 inputs: workingDirectory: $(System.DefaultWorkingDirectory) script: | #      nuget restore .\ShareX.sln #  ,        md .\PVSTestResults #        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 #    - task: PublishBuildArtifacts@1 inputs: pathToPublish: PVSTestResults artifactName: PVSTestResults 

After completing the task, the archive with the analyzer reports can be downloaded on the “Summary” tab, or we can use the Send Mail extension, which allows you to configure the sending of e-mail, or search for a more convenient tool on the Marketplace .

About analysis results

Now let's look at some of the errors that were found in the checked project - ShareX.

Redundant 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

Pay attention to checking the variable dataObject for null . What is she here for? dataObject simply cannot be null in this case, since it is initialized with a reference to the created object. As a result, we have redundant verification. Is it critical? No. Looks concise? No. This check is clearly better removed so as not to clutter up the code.

Let's take a look at another piece of code, to which you can make similar comments:

 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

The GetImageAlternative method again checks that the img variable is not null immediately after creating a new instance of the Bitmap class. The difference from the previous example here is that to initialize the img variable we use not the constructor, but the GetDIBImage method. The author of the code assumes that an exception may occur in this method, but declares only try and finally blocks, omitting catch . Therefore, if an exception occurs, the calling method - GetImageAlternative - will not receive a reference to an object of type Bitmap, but will be forced to handle the exception in its own catch block . In this case, the img variable will not be initialized, and the thread of execution will not even reach the check img! = Null , but will immediately fall into the catch block . Therefore, the analyzer did indicate redundant validation.

Consider the following warning example with code V3022 :

 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 look at the second conditional expression. There we check the value of the read-only Count property. This property indicates the number of elements in an instance of the SelectedItems collection. The condition is satisfied only if the Count property is greater than zero. Everything would be fine, but it is only in the external if statement that the Count is already checked. An instance of the SelectedItems collection cannot have the number of elements less than zero, therefore, Count takes a value either equal to zero or greater than zero. Since we have already performed a check in the first if statement that Count is zero, and it turned out to be false, it makes no sense to write another check on the else branch that Count is greater than zero.

The final example of error number V3022 is the following code fragment:

 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, since a little higher declaration is performed and the itemCount variable is set to zero at the same time. Up to the very condition, this variable is not used anywhere and does not change, therefore, the analyzer made the correct conclusion about the conditional expression, the value of which is always false.

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

The best way to understand the bug is to visualize the 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 don’t want to immediately reveal all the cards and show what our analyzer found here, so let's postpone this moment for a short while.

By the name of the method, it is easy to guess what it does - you submit an image or a fragment of the image to it as an input, and it performs its pixelation. The method code is quite long, so we won’t give it here in its entirety, but just try to explain its algorithm and explain what kind of bug PVS-Studio found here.

This method takes two parameters as an input: an object of type Bitmap and a value of type int , which indicates the size of the pixelization. The operation algorithm is quite simple:

1) We break the fragment of the image received at the input into squares with a side equal to the size of the pixelation. For example, if we have a pixelization size of 15, then we get a square containing 15x15 = 225 pixels.

2) Next, we go around each pixel in this square and accumulate the values ​​of the Red , Green , Blue and Alpha fields into intermediate variables, and previously multiplying the corresponding color value and the alpha channel value by the pixelWeight variable, obtained by dividing the Alpha value by 255 (the Alpha variable has type byte ). Also, when traversing pixels, we sum the values ​​recorded in pixelWeight into a variable called weightedCount .

The code snippet that performs the above steps 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, pay attention to the fact that if the value of the Alpha variable is equal to zero, then pixelWeight will not add any value to the variable weightedCount for this pixel. We will need this in the future.

3) After we have bypassed all the pixels in the current square, we can make up the overall “average” color for this square. The code that performs these actions is as follows:

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

4) Now that we have got the final color and recorded it in the variable averageColor , we can again go around each pixel of the square and assign it a value from averageColor .

5) We return to step 2 as long as there are still raw squares.

Once again, we note that the variable weightedCount is not equal to the number of all pixels squared. For example, if an absolutely transparent pixel is found in the image (the value is zero on the alpha channel), then the pixelWeight variable will be zero for this pixel ( 0/255 = 0), therefore, this pixel will not make any contribution to the formation of the value of the weightedCount variable. This is logical - it makes no sense to take into account the colors of an absolutely transparent pixel.

Everything seems quite reasonable - pixelation should work correctly. And it really works right. That's just not for png images that include pixels with values ​​in the alpha channel less than 255 and unequal to zero. Pay attention to the pixelated image below:

Have you seen pixelation? And we are not. Ok, now let's reveal this little intrigue and explain where exactly the bug is hidden in this method. The error crept into the line for calculating the value of the pixelWeight variable:

 float pixelWeight = color.Alpha / 255; 

The fact is that the author of the code, declaring the pixelWeight variable as float , implied that when dividing the Alpha field by 255, in addition to zero and one, fractional numbers should be obtained. This is where the problem lies, since the Alpha variable is of type byte , and when dividing it by 255 we get an integer value, and only then it will be implicitly cast to float , therefore, the fractional part is lost.

The inability to pixelize PNG images that have some degree of transparency is easy to explain. Since the alpha channel values ​​for these pixels lie in the range 0 <Alpha <255, when dividing the Alpha variable by 255, we will always get 0. Therefore, the values ​​of the pixelWeight , r , g , b , a , weightedCount variables are also always will be zero. As a result, our average color averageColor will be with zero values ​​across all channels: red - 0, blue - 0, green - 0, alpha - 0. Filling the square with this color, we do not change the original color of the pixels, since averageColor is absolutely transparent . In order to fix this error, you just need to explicitly cast the Alpha field to float type. The corrected line of code may look like this:

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

And it's time to cite the message that PVS-Studio gave out to 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.

And for comparison, we give a screenshot of a truly pixelated image obtained on a fixed version of the application:

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 its author expected the pi variable to be null , which is why the pi! = Null check is performed before calling the SetPropertyItem method. It is strange that before this check, an array of bytes is assigned to the pi.Value property, because if pi is null , an exception of type NullReferenceException will be thrown.

A similar situation was noticed elsewhere:

 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 meaning is the same, so there is no great need to give a code fragment, we will restrict ourselves to the analyzer message.

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 piece of code was discovered in the EvalWindows method of the WindowsList class, which returns true under any circumstances:

 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

It seems logical that if a pointer with the same value as hWnd were found in a list with the name IgnoreWindows , then the method should return false .

The IgnoreWindows list can be populated either by calling the WindowsList constructor (IntPtr ignoreWindow) , or directly through access to the property, since it is public. One way or another, according to Visual Studio, at the moment this list is not populated in the code. This is another strange place of this method.

Insecure call to 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

In this case, the following unpleasant situation may occur: after checking the NewsLoaded variable for null inequality, the method that processes the event can be unsubscribed, for example, in another thread, and when we get into the body of the conditional if statement , the NewsLoaded variable will already be equals null . Attempting to call subscribers on a NewsLoaded event that is null will throw a NullReferenceException . It is much safer to use the null conditional operator and rewrite the code above as follows:

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

The analyzer indicated 68 more similar places. We will not describe them here - the pattern of the event call in them is similar.

Return null from ToString

Not so long ago, from a colleague’s interesting article, I found out that Microsoft doesn’t recommend returning null from an overridden ToString method. 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 appropriate if not using?

 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 you can see from the example, when declaring the url variable, it is assigned some value returned from the FixPrefix method. In the next line, we “grind” the resulting value, even without using it anywhere. We get something similar to "dead code" - it does the work, it does not affect the final result. This error, most likely, is the result of copy-paste, since similar code fragments are found in 9 more methods.
For an example, we give 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"); .... } 

Total

As we can see, the complexity of setting up automatic verification by the analyzer does not depend on the selected CI system - in just 15 minutes and a few clicks of the mouse, we set up the verification of our project code with a static analyzer.

In conclusion, we suggest that you download and try the analyzer on your projects.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Oleg Andreev, Ilya Gainulin. PVS-Studio in the Clouds: Azure DevOps .