# PVS\-Studio in the clouds: Azure DevOps 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\]\(https://pvs\-studio\.com/en/docs/manual/0053/\)"\\\. ![0670_Azure_DevOps/image1.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image1.png) 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](https://pvs-studio.com/en/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](https://azure.microsoft.com/en-in/services/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](https://github.com/ShareX/ShareX)\. 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](https://azure.microsoft.com/en-us/services/devops/) and press "Start free with GitHub"\. ![0670_Azure_DevOps/image2.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image2.png) Give the Microsoft application access to the GitHub account data\. ![0670_Azure_DevOps/image3.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image3.png) You'll have to create a Microsoft account to complete your registration\. ![0670_Azure_DevOps/image4.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image4.png) After registration, create a project: ![0670_Azure_DevOps/image5.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image5.png) Next, we need to move to "Pipelines" \- "Builds" and create a new Build pipeline\. ![0670_Azure_DevOps/image6.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image6.png) When asked where our code is located, we will answer \- GitHub\. ![0670_Azure_DevOps/image7.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image7.png) Authorize Azure Pipelines and choose the repository with the project, for which we'll configure the static analyzer's run\. ![0670_Azure_DevOps/image8.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image8.png) In the template selection window, choose "Starter pipeline\." ![0670_Azure_DevOps/image9.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image9.png) 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: ```cpp # 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](https://github.com/Microsoft/azure-pipelines-image-generation/blob/master/images/win/WindowsContainer1803-Readme.md), 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\. ![0670_Azure_DevOps/image10.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image10.png) 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\. ![0670_Azure_DevOps/image11.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image11.png) 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](https://docs.microsoft.com/en-us/azure/devops/pipelines/agents/v2-windows?view=azure-devops) and install and [configure](https://pvs-studio.com/en/docs/manual/0046/) the static analyzer\. To run the task on a self\-hosted agent, we'll replace the suggested configuration with the following: ```cpp # 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](https://marketplace.visualstudio.com/items?itemName=rvo.SendEmailTask&ssr=false) that enables to configure emailing or consider another convenient tool on [Marketplace](https://marketplace.visualstudio.com/search?target=AzureDevOps&category=Azure%20Pipelines&sortBy=Downloads)\. ![0670_Azure_DevOps/image12.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image12.png) ## 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v3022/) warning: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v3022/) warning will be the following fragment of code: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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: ```cpp 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: ```cpp 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: ```cpp 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: ![0670_Azure_DevOps/image13.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image13.png) 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: ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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: ![0670_Azure_DevOps/image14.png](https://import.viva64.com/docx/blog/0670_Azure_DevOps/image14.png) **Potential NullReferenceException** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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: ```cpp private static void Task_TaskCompleted(WorkerTask task) { .... task.KeepImage = false; if (task != null) { if (task.RequestSettingUpdate) { Program.MainForm.UpdateCheckStates(); } .... } .... } ``` **PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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: ```cpp public class WindowsList { public List IgnoreWindows { get; set; } .... public WindowsList() { IgnoreWindows = new List(); } 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp protected void OnNewsLoaded() { if (NewsLoaded != null) { NewsLoaded(this, EventArgs.Empty); } } ``` **PVS\-Studio warning:** [V3083](https://pvs-studio.com/en/docs/warnings/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: ```cpp 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](https://pvs-studio.com/en/blog/posts/csharp/0656/) of my colleague that Microsoft doesn't recommend returning null from the overridden method _ToString_\. PVS\-Studio is well aware of this: ```cpp public override string ToString() { lock (loggerLock) { if (sbMessages != null && sbMessages.Length > 0) { return sbMessages.ToString(); } return null; } } ``` **PVS\-Studio warning:** [V3108](https://pvs-studio.com/en/docs/warnings/v3108/) It is not recommended to return 'null' from 'ToSting\(\)' method\. Logger\.cs 167 **Why assigned if not used?** ```cpp public SeafileCheckAccInfoResponse GetAccountInfo() { string url = URLHelpers.FixPrefix(APIURL); url = URLHelpers.CombineURL(APIURL, "account/info/?format=json"); .... } ``` **PVS\-Studio warning:** [V3008](https://pvs-studio.com/en/docs/warnings/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: ```cpp 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](https://pvs-studio.com/en/pvs-studio/download/) on your projects\.