PVS-Studio is now in Chocolatey: checking Chocolatey under Azure DevOps

• About Chocolatey usage
• Configuration
• Analysis results
• Conclusion
• P.S.

We continue making the use of PVS-Studio more convenient. Our analyzer is now available in Chocolatey, the package manager for Windows. We believe this will make it easier to deploy PVS-Studio, particularly in cloud services. So right off the bat, we also checked the source code of the same Chocolatey. Azure DevOps took on the role of the CI system.

Here's the list of our other articles on cloud integration:

• PVS-Studio in the Clouds: Azure DevOps
• PVS-Studio in the Clouds: CircleCI
• PVS-Studio in the Clouds: Travis CI
• PVS-Studio in the Clouds: GitLab CI/CD

I suggest that you pay attention to the first article on integration with Azure DevOps, as some points will be omitted to avoid repeating some nuances.

So, the main characters of this article are:

PVS-Studio is a static code analyzer for detecting errors and potential vulnerabilities in source code of programs, written in C, C++, C#, and Java. Works in 64-bit systems on Windows, Linux, and macOS and can analyze code for 32-bit, 64-bit and embedded ARM platforms. If it's the first time you're going to try static code analysis for checking your projects, we'd like to recommend reading the article on how to quickly check out the most interesting PVS-Studio warnings and assess the abilities of this tool.

Azure DevOps is a set of cloud services that jointly cover the entire development process. This platform includes tools such as Azure Pipelines, Azure Boards, Azure Artifacts, Azure Repos, Azure Test Plans to speed up the process of creating software and improve its quality.

Chocolatey is an open source package manager for Windows. The goal of the project is to automate the entire lifecycle of the software from installation to upgrade and deletion in Windows operating systems.

About Chocolatey usage

To see how to install the package manager itself, follow this link. Full documentation on the installation of the analyzer is available by the link in the "Installation using Chocolatey package manager" section. In a nutshell, I'll repeat some of the points from there.

The command for latest analyzer version installation:

choco install pvs-studio

The command for a specific PVS-Studio package installation:

choco install pvs-studio --version=7.05.35617.2075

By default, only the core of the analyzer, which is the the Core component, is installed. All other flags (Standalone, JavaCore, IDEA, MSVS2010, MSVS2012, MSVS2013, MSVS2015, MSVS2017, MSVS2019) can be passed via --package-parameters.

An example of a command that will install the analyzer with the plugin for Visual Studio 2019:

choco install pvs-studio --package-parameters="'/MSVS2019'"

Here's an example of convenient analyzer usage under Azure DevOps.

Configuration

Let me remind you that the article mentioned above gives all needed information about such things as creating Build Pipeline and account synchronization with the project in a GitHub repository. In our case, the configuration will start right with writing a configuration file.

To begin with, we'll set up a startup trigger, indicating that we only run for changes in the master branch:

trigger:
- master

Next, we need to choose a virtual machine. At this point, it will be Microsoft hosted agent with the Windows Server 2019 and Visual Studio 2019:

pool:
  vmImage: 'windows-latest'

Let's move on to the body of the configuration file (the steps block). Even though you can't install random software on a virtual machine, I didn't add a Docker container. We can add Chocolatey as an extension for Azure DevOps. To do this, follow the link. Select Get it free. Further, if you are already logged in, just choose your account, and if not, do the same after authorization.

Here we need to choose where we add the extension and click Install.

After successful installation click Proceed to organization:

Now you can see the template for the Chocolatey task in the tasks window when editing the configuration file azure-pipelines.yml:

Click Chocolatey and you'll see the list of fields:

Here we need to choose install in the field with commands. In Nuspec File Name, specify the name of the needed package - pvs-studio. Without version specification, the latest one will be installed, which is absolutely great for us. Click add and see the new task in the configuration file.

steps:
- task: ChocolateyCommand@0
  inputs:
    command: 'install'
    installPackageId: 'pvs-studio'

Next, let's move on to the main part of our file:

- task: CmdLine@2
  inputs:
    script:

Now we need to create a file with the analyzer license. Here PVSNAME and PVSKEY are the names of variables we specify in the settings. They will store the PVS-Studio login and license key. To set their values, open the menu Variables->New variable. Then create variables: PVSNAME - for the login, and PVSKEY- for the analyzer key. Don't forget to tick Keep this value secret for PVSKEY. The command code:

call "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" credentials 
–u $(PVSNAME) –n $(PVSKEY)

Build the project using the bat file from the repository.

call build.bat

After that, create the repository for files with the analyzer results:

 call mkdir PVSTestResults

Next, run project analysis.

call "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
–t .\src\chocolatey.sln –o .\PVSTestResults\Choco.plog

Convert the report into the html format by the PlogConverter utility:

call "C:\Program Files (x86)\PVS-Studio\PlogConverter.exe" 
–t html –o \PVSTestResults\ .\PVSTestResults\Choco.plog

Now you need to create a task so that you can download the report.

- task: PublishBuildArtifacts@1
  inputs:
    pathToPublish: PVSTestResults
    artifactName: PVSTestResults
    condition: always()

Full configuration file looks like this:

trigger:
- master

pool:
  vmImage: 'windows-latest'

steps:
- task: ChocolateyCommand@0
  inputs:
    command: 'install'
    installPackageId: 'pvs-studio'

- task: CmdLine@2
  inputs:
    script: |
      call "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
      credentials –u $(PVSNAME) –n $(PVSKEY)
      call build.bat
      call mkdir PVSTestResults
      call "C:\Program Files (x86)\PVS-Studio\PVS-Studio_Cmd.exe" 
      –t .\src\chocolatey.sln –o .\PVSTestResults\Choco.plog
      call "C:\Program Files (x86)\PVS-Studio\PlogConverter.exe" 
      –t html –o .\PVSTestResults\ .\PVSTestResults\Choco.plog

- task: PublishBuildArtifacts@1
  inputs:
    pathToPublish: PVSTestResults
    artifactName: PVSTestResults
    condition: always()

Click Save->Save->Run to run the task. Go to the task tab and download the report.

The Chocolatey project contains only 37615 lines of C# code. Let's consider some of detected errors.

Analysis results

Warning N1

Analyzer warning: V3005 The 'Provider' variable is assigned to itself. CrytpoHashProviderSpecs.cs 38

public abstract class CrytpoHashProviderSpecsBase : TinySpec
{
  ....
  protected CryptoHashProvider Provider;
  ....
  public override void Context()
  {
    Provider = Provider = new CryptoHashProvider(FileSystem.Object);
  }
}

The analyzer has detected variable assignment to itself, which makes no sense. Most likely, another variable should have been here instead of one of these. Or it's a typo, and the extra assignment can simply be removed.

Warning N2

Analyzer warning: V3093 [CWE-480] The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. Platform.cs 64

public static PlatformType get_platform()
{
  switch (Environment.OSVersion.Platform)
  {
    case PlatformID.MacOSX:
    {
      ....
    }
    case PlatformID.Unix:
    if(file_system.directory_exists("/Applications")
      & file_system.directory_exists("/System")
      & file_system.directory_exists("/Users")
      & file_system.directory_exists("/Volumes"))
      {
        return PlatformType.Mac;
      }
        else
          return PlatformType.Linux;
    default:
      return PlatformType.Windows;
  }
}

The difference between & and && operators is that if the left part of the expression is false, the right part will be evaluated anyway if & is used, which, in this case, implies unnecessary calls of the system.directory_exists method.

In the fragment considered, it's a minor flaw. Yes, this condition can be optimized by replacing the & operator with &&, but from a practical point of view, it doesn't affect anything. However, in other cases, confusion between & and && can cause serious problems, when the right part of the expression handles incorrect/invalid values. For example, here's the case from our collection of errors detected by the V3093 diagnostic:

if ((k < nct) & (s[k] != 0.0))

Even if the k index is incorrect, it will be used to access the array element. As a result, IndexOutOfRangeException will be generated.

Warnings N3, N4

Analyzer warning: V3022 [CWE-571] Expression 'shortPrompt' is always true. InteractivePrompt.cs 101

Analyzer warning: V3022 [CWE-571] Expression 'shortPrompt' is always true. InteractivePrompt.cs 105

public static string 
prompt_for_confirmation(.... bool shortPrompt = false, ....)
{
  ....
  if (shortPrompt)
  {
    var choicePrompt = choice.is_equal_to(defaultChoice) //1
    ?
    shortPrompt //2
    ?
    "[[{0}]{1}]".format_with(choice.Substring(0, 1).ToUpperInvariant(), //3
    choice.Substring(1,choice.Length - 1))
    :
    "[{0}]".format_with(choice.ToUpperInvariant()) //0
    : 
    shortPrompt //4
    ? 
    "[{0}]{1}".format_with(choice.Substring(0,1).ToUpperInvariant(), //5
    choice.Substring(1,choice.Length - 1)) 
    :
    choice; //0
    ....
  }
  ....
}

In this case, the logic of the ternary operator is strange. Let's peek it under the hood: if the condition that I marked with number 1 is fulfilled, we'll move on to the condition 2, which is always true, which means that the line 3 will execute. If number 1 condition is false, we'll jump to the line, marked as 4; its condition is also always true, which means that the line 5 will execute. Thus, conditions marked with comment 0 will never execute, which may be not the logic the programmer wanted.

Warning N5

Analyzer warning: V3123 [CWE-783] Perhaps the '?:' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its condition. Options.cs 1019

private static string GetArgumentName (...., string description)
{
  string[] nameStart;
  if (maxIndex == 1)
  {
    nameStart = new string[]{"{0:", "{"};
  }
  else
  {
    nameStart = new string[]{"{" + index + ":"};
  }
  for (int i = 0; i < nameStart.Length; ++i) 
  {
    int start, j = 0;
    do 
    {
      start = description.IndexOf (nameStart [i], j);
    } 
    while (start >= 0 && j != 0 ? description [j++ - 1] == '{' : false);
    ....
    return maxIndex == 1 ? "VALUE" : "VALUE" + (index + 1);
  }
}

The diagnostic triggered for the following line:

while (start >= 0 && j != 0 ? description [j++ - 1] == '{' : false)

Since the j variable is initialized by 0 a few lines earlier, the ternary operator will return false. Due to this condition, the loop body will execute only one time. It seems to me that this piece of code doesn't work the way the programmer intended.

Warning N6

Analyzer warning: V3022 [CWE-571] Expression 'installedPackageVersions.Count != 1' is always true. NuGetService.cs 1405

private void remove_nuget_cache_for_package(....)
{
  if (!config.AllVersions && installedPackageVersions.Count > 1)
  {
    const string allVersionsChoice = "All versions";
    if (installedPackageVersions.Count != 1)
    {
      choices.Add(allVersionsChoice);
    }
    ....
  }
  ....
}

The nested condition installedPackageVersions.Count != 1, which is always true, is quite dubious. Often such a warning indicates a logical error in code, or just a redundant check.

Warning N7

Analyzer warning: V3001 There are identical sub-expressions 'commandArguments.contains("-apikey")' to the left and to the right of the '||' operator. ArgumentsUtility.cs 42

public static bool arguments_contain_sensitive_information(string
 commandArguments)
{
  return commandArguments.contains("-install-arguments-sensitive")
  || commandArguments.contains("-package-parameters-sensitive")
  || commandArguments.contains("apikey ")
  || commandArguments.contains("config ")
  || commandArguments.contains("push ")
  || commandArguments.contains("-p ")
  || commandArguments.contains("-p=")
  || commandArguments.contains("-password")
  || commandArguments.contains("-cp ")
  || commandArguments.contains("-cp=")
  || commandArguments.contains("-certpassword")
  || commandArguments.contains("-k ")
  || commandArguments.contains("-k=")
  || commandArguments.contains("-key ")
  || commandArguments.contains("-key=")
  || commandArguments.contains("-apikey")
  || commandArguments.contains("-api-key")
  || commandArguments.contains("-apikey")
  || commandArguments.contains("-api-key");
}

The programmer who wrote this section of the code copied the last two lines and forgot to edit them. Because of this, Chocolatey users were unable to apply the apikey parameter in a couple of other ways. The same as with the parameter above, I can suggest the following options:

commandArguments.contains("-apikey=");
commandArguments.contains("-api-key=");

Copy-paste errors have a great chance to appear sooner or later in any project with a large source code base, and one of the best means of fighting against them is static analysis.

P.S. And as always, this error tends to appear at the end of the multi-line condition :). See the post "Last Line Effect".

Warning N8

Analyzer warning: V3095 [CWE-476] The 'installedPackage' object was used before it was verified against null. Check lines: 910, 917. NuGetService.cs 910

public virtual ConcurrentDictionary<string, PackageResult> get_outdated(....)
{
  ....
  var pinnedPackageResult = outdatedPackages.GetOrAdd(
    packageName, 
    new PackageResult(installedPackage, 
                      _fileSystem.combine_paths(
                        ApplicationParameters.PackagesLocation, 
                        installedPackage.Id)));
  ....
  if (   installedPackage != null
      && !string.IsNullOrWhiteSpace(installedPackage.Version.SpecialVersion) 
      && !config.UpgradeCommand.ExcludePrerelease)
  {
    ....
  }
  ....
}

A classic bug: first the installedPackage object is used, and then checked for null. This diagnostic tells us about one of the problems in the program: either installedPackage is never null (which is questionable), and the check is redundant, or we can potentially get a serious error in the code - an attempt to access a null reference.

Conclusion

So we have made another small step - now to use PVS-Studio has become even easier and more convenient. I also would like to note that Chocolatey is a fine package manager with a low number of code errors that could have been even smaller if checked by PVS-Studio.

I kindly invite you to download and try PVS-Studio. Regular usage of static analyzer will enhance the quality and reliability of the code you write and will help prevent many zero-day vulnerabilities.

P.S.

Before publishing the article, we sent one to the Chocolatey developers which were pretty fine with it. We haven't found any critical bugs there but they liked an error related to "api-key", for example.