Hello, everyone. Today we have another Microsoft project on the check. By the title of this article, you can guess that this time developers didn't "please" us with a large number of errors. We hope the project's authors won't be offended by the title. After all, a small number of errors is great, isn't it? However, we still managed to find something intriguing in the Azure PowerShell code. We suggest getting to know the features of this project and checking out errors, found using the PVS-Studio C# analyzer.
Azure PowerShell is a set of commandlets (cmdlet) that lets you manage Azure resources right from the PowerShell command line. The main purpose of this set is to simplify the process of studying and quickly start the development for Azure. Azure PowerShell provides administrators and developers with compelling features to create, deploy, and manage Microsoft Azure applications.
Azure PowerShell is developed in the .NET Standard environment, is supported by PowerShell 5.1 for Windows and PowerShell 6.x and later for all platforms. The Azure PowerShell source code is available on GitHub.
Recently I've been often getting Microsoft projects for a check. In my opinion, the quality of these projects is usually top-notch. Although, of course, not without exceptions, as described in the article "WinForms: errors, Holmes". But this time everything is fine. The project is large: 6845 .cs source code files contain approximately 700,000 lines, excluding blank ones (I didn't take into account tests and warnings of the third level of certainty). Very few errors were found for such amount of code: no more than a hundred. There were quite many similar cases, so I chose the most interesting ones for the article. As usually, I sorted errors by the PVS-Studio warnings' numbers.
Also I stumbled upon some code fragments that looked like errors, but couldn't be recognized as definitely erroneous, as I'm not familiar enough with PowerShell development peculiarities. I hope that among readers there will be specialists in this issue who will help me. I'll describe it in detail below.
Before the feedback part, I'd like to mention the specific structure of the project. The Azure PowerShell source code consists of more than seventy Visual Studio solutions. Some solutions include projects from other ones. This structure slowed down the analysis a little (not much). Still the check didn't cause any other difficulties. For convenience, in the error message (in brackets) I will specify the name of the solution where the error was found.
V3001 There are identical sub-expressions 'strTimespan.Contains("M")' to the left and to the right of the '||' operator. AzureServiceBusCmdletBase.cs 187 (EventGrid)
public static TimeSpan ParseTimespan(string strTimespan)
{
....
if (strTimespan.Contains("P")
|| strTimespan.Contains("D")
|| strTimespan.Contains("T")
|| strTimespan.Contains("H")
|| strTimespan.Contains("M")
|| strTimespan.Contains("M"))
....
}An example of a fairly obvious error that only a developer can fix. In this case, it's absolutely unclear whether we deal with code duplication that affects nothing or something else has to take place instead of "M" in one of two last checks.
V3001 There are identical sub-expressions 'this.AggregationType != null' to the left and to the right of the '&&' operator. GetAzureRmMetricCommand.cs 156 (Monitor)
public AggregationType? AggregationType { get; set; }
....
protected override void ProcessRecordInternal()
{
....
string aggregation = (this.AggregationType != null &&
this.AggregationType.HasValue) ?
this.AggregationType.Value.ToString() : null;
....
}There's probably no error here. This is an example of redundant code. Sometimes such code might indicate lack of developer's knowledge. The point is that the checks this.AggregationType != null and this.AggregationType.HasValue are identical. It is enough to use only one of them (any one). Personally, I prefer the option with HasValue:
string aggregation = this.AggregationType.HasValue ?
this.AggregationType.Value.ToString() :
null;V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 152, 163. GetAzureRmRecoveryServicesBackupProtectionPolicy.cs 152 (RecoveryServices)
public override void ExecuteCmdlet()
{
....
if( WorkloadType == Models.WorkloadType.AzureVM )
{
....
}
....
else if( WorkloadType == Models.WorkloadType.AzureFiles )
{
if( BackupManagementType != Models.BackupManagementType.AzureStorage )
{
throw new ArgumentException(
Resources.AzureFileUnsupportedBackupManagementTypeException );
}
serviceClientProviderType = ServiceClientHelpers.
GetServiceClientProviderType( Models.WorkloadType.AzureFiles );
}
else if( WorkloadType == Models.WorkloadType.AzureFiles )
{
if( BackupManagementType != Models.BackupManagementType.AzureStorage )
{
throw new ArgumentException(
Resources.AzureFileUnsupportedBackupManagementTypeException );
}
serviceClientProviderType = ServiceClientHelpers.
GetServiceClientProviderType( Models.WorkloadType.AzureFiles );
}
....
}Two else if blocks are absolutely identical, including both the condition and the body of the block. Such errors are usually made when using the copy-paste method. The issue here is again the error's criticality. If it's not simple code duplication, it may be the absent needed check and appropriate set of actions. The author definitely has to edit the code.
V3005 The 'this.VM.OSProfile.WindowsConfiguration.ProvisionVMAgent' variable is assigned to itself. SetAzureVMOperatingSystemCommand.cs 298 (Compute)
public override void ExecuteCmdlet()
{
....
// OS Profile
this.VM.OSProfile.WindowsConfiguration.ProvisionVMAgent =
this.VM.OSProfile.WindowsConfiguration.ProvisionVMAgent;
....
}The value of the property is self-assigned. Take a look at its declaration:
[JsonProperty(PropertyName = "provisionVMAgent")]
public bool? ProvisionVMAgent { get; set; }The JsonProperty description states: "Instructs the Newtonsoft.Json.JsonSerializer to always serialize the member with the specified name". It seems that everything is guileless and the obvious error has been made. Explicit usage of this to access the property is also quite confusing. Perhaps, another variable wasn't mistakenly specified instead of this. But let's not jump to conclusions. The fact of the matter is that I came across quite a lot of such assignments (a property is self-assigned). Here is an example of an assignment, very similar to an error:
V3005 The 'this.LastHeartbeat' variable is assigned to itself. PSFabricDetails.cs 804 (RecoveryServices)
public ASRInMageAzureV2SpecificRPIDetails(
InMageAzureV2ReplicationDetails details)
{
this.LastHeartbeat = this.LastHeartbeat; // <=
this.RecoveryAvailabilitySetId = details.RecoveryAvailabilitySetId;
this.AgentVersion = details.AgentVersion;
this.DiscoveryType = details.DiscoveryType;
....
}Let's have a closer look at the second and subsequent assignments. In the right part of the expression, details takes place instead of this. Now look at the declaration of the this.LastHeartbeat property:
public DateTime? LastHeartbeat { get; set; }At last, let's find the property with the same name in the InMageAzureV2ReplicationDetails class. Such property is declared there:
public class InMageAzureV2ReplicationDetails :
ReplicationProviderSpecificSettings
{
....
[JsonProperty(PropertyName = "lastHeartbeat")]
public DateTime? LastHeartbeat { get; set; }
....
}Well, in this case, I'm willing to admit it's a real error. But what shall we do with the next warnings? Unlike two previous code fragments, there are multiple self-assigned properties. Well, this looks less like an error:
[Cmdlet(VerbsCommon.Remove,
ResourceManager.Common.AzureRMConstants.AzureRMPrefix +
"ExpressRouteConnection",
DefaultParameterSetName =
CortexParameterSetNames.ByExpressRouteConnectionName,
SupportsShouldProcess = true),
OutputType(typeof(bool))]
public class RemoveExpressRouteConnectionCommand :
ExpressRouteConnectionBaseCmdlet
{
[Parameter(
Mandatory = true,
ParameterSetName = CortexParameterSetNames.ByExpressRouteConnectionName,
HelpMessage = "The resource group name.")]
[ResourceGroupCompleter]
[ValidateNotNullOrEmpty]
public string ResourceGroupName { get; set; }
....
public override void Execute()
{
if (....)
{
this.ResourceGroupName = this.ResourceGroupName;
this.ExpressRouteGatewayName = this.ExpressRouteGatewayName;
this.Name = this.Name;
}
....
}
....
}The Execute method contains three properties' self-assignments in a row. Just in case, I cited full declaration of the class RemoveExpressRouteConnectionCommand and all its attributes, as well as the ResourceGroupName property declaration (other two properties are declared in a similar way). It was these warnings that made me think about the question: "Is it an error?" I suspect that some internal magic of PowerShell development may be happening here. I hope that among readers there will be experts who are informed about this issue. I'm not ready to draw any conclusions in this case.
V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new ArgumentException(FOO). StartAzureRmRecoveryServicesAsrTestFailoverJob.cs 259 (RecoveryServices)
private void StartRPITestFailover()
{
....
if (....)
{
....
}
else
{
new ArgumentException(
Resources
.UnsupportedDirectionForTFO); // Throw Unsupported Direction
// Exception
}
....
}The throw keyword is omitted. And the comment says that the exception just has to be thrown. I encountered several more similar errors in the RecoveryServices solution:
V3022 Expression 'apiType.HasValue' is always false. ApiManagementClient.cs 1134 (ApiManagement)
private string GetApiTypeForImport(...., PsApiManagementApiType? apiType)
{
....
if (apiType.HasValue)
{
switch(apiType.Value)
{
case PsApiManagementApiType.Http: return SoapApiType.SoapToRest;
case PsApiManagementApiType.Soap: return SoapApiType.SoapPassThrough;
default: return SoapApiType.SoapPassThrough;
}
}
return apiType.HasValue ? // <=
apiType.Value.ToString("g") :
PsApiManagementApiType.Http.ToString("g");
}The logic of the work has been broken. If apiType contains a value, the control won't reach the return expression at the end of the method (all switch branches contain return). Otherwise, the method will always return PsApiManagementApiType.Http.ToString("g"), whereas the apiType.Value.ToString("g") value will never be returned.
V3022 Expression 'automationJob != null && automationJob == null' is always false. NodeConfigurationDeployment.cs 199 (Automation)
public NodeConfigurationDeployment(
....,
Management.Automation.Models.Job automationJob = null,
....)
{
....
if (automationJob != null && automationJob == null) return;
....
}Counter-intuitive code. Two checks that contradict each other. Probably the second check for null contains the wrong variable.
V3022 Expression is always false. DataFactoryClient.Encrypt.cs 37 (DataFactory)
public virtual string OnPremisesEncryptString(....)
{
....
if ( linkedServiceType == LinkedServiceType.OnPremisesSqlLinkedService
&& linkedServiceType == LinkedServiceType.OnPremisesOracleLinkedService
&& linkedServiceType == LinkedServiceType.OnPremisesFileSystemLinkedService
&& (value == null || value.Length == 0))
{
throw new ArgumentNullException("value");
}
....
}The check is pointless and the exception will never be thrown. The condition requires simultaneous linkedServiceType variable's equality to three different values. The operators && and || are likely to be confused. Fixed code:
if (( linkedServiceType == LinkedServiceType.OnPremisesSqlLinkedService
|| linkedServiceType == LinkedServiceType.OnPremisesOracleLinkedService
|| linkedServiceType == LinkedServiceType.OnPremisesFileSystemLinkedService)
&& (value == null || value.Length == 0))
....V3022 Expression 'Ekus == null' is always false. PSKeyVaultCertificatePolicy.cs 129 (KeyVault)
internal CertificatePolicy ToCertificatePolicy()
{
....
if (Ekus != null)
{
x509CertificateProperties.Ekus = Ekus == null ?
null : new List<string>(Ekus);
}
....
}Redundant check of the Ekus variable for null. It's probably fine, but the code doesn't look nice.
V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. PolicyRetentionObjects.cs 207 (RecoveryServices)
public virtual void Validate()
{
if (RetentionTimes == null
|| RetentionTimes.Count == 0
|| RetentionTimes.Count != 1)
{
throw new ArgumentException(
Resources.InvalidRetentionTimesInPolicyException);
}
}Here's an excessive check or an excessive condition. The check RetentionTimes.Count == 0 is pointless, as after that, the check RetentionTimes.Count != 1 follows.
V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: this.ResourceGroupName. NewScheduledQueryRuleCommand.cs 117 (Monitor)
protected override void ProcessRecordInternal()
{
....
if (this.ShouldProcess(this.Name,
string.Format("Creating Log Alert Rule '{0}' in resource group {0}",
this.Name, this.ResourceGroupName)))
{
....
}
....
}An error in the formatting line. The specifier {0} is used twice, and the Format method is passed two arguments. Here is the correct version:
if (this.ShouldProcess(this.Name,
string.Format("Creating Log Alert Rule '{0}' in resource group {1}",
this.Name, this.ResourceGroupName)))
....Another similar error:
V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'imageAndOsType' object VirtualMachineScaleSetStrategy.cs 81 (Compute)
internal static ResourceConfig<VirtualMachineScaleSet>
CreateVirtualMachineScaleSetConfig(...., ImageAndOsType imageAndOsType, ....)
{
....
VirtualMachineProfile = new VirtualMachineScaleSetVMProfile
{
OsProfile = new VirtualMachineScaleSetOSProfile
{
....,
WindowsConfiguration =
imageAndOsType.CreateWindowsConfiguration(), // <=
....,
},
StorageProfile = new VirtualMachineScaleSetStorageProfile
{
ImageReference = imageAndOsType?.Image, // <=
DataDisks = DataDiskStrategy.CreateVmssDataDisks(
imageAndOsType?.DataDiskLuns, dataDisks) // <=
},
},
....
}When creating the VirtualMachineScaleSetVMProfile object, the imageAndOsType variable is checked for null without any preliminary check. However, further when creating VirtualMachineScaleSetStorageProfile, this variable is already checked using the conditional access operator even twice. The code doesn't look safe.
V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'existingContacts' object RemoveAzureKeyVaultCertificateContact.cs 123 (KeyVault)
public override void ExecuteCmdlet()
{
....
List<PSKeyVaultCertificateContact> existingContacts;
try
{
existingContacts = this.DataServiceClient.
GetCertificateContacts(VaultName)?.ToList();
}
catch (KeyVaultErrorException exception)
{
....
existingContacts = null;
}
foreach (var email in EmailAddress)
{
existingContacts.RemoveAll(....); // <=
}
....
}In both normal execution and as a result of handling an exception, the variable existingContacts can get the null value, after which the execution will continue. Further in the code, this variable is used without any specific reason.
V3066 Possible incorrect order of arguments passed to 'PersistSyncServerRegistration' method: 'storageSyncServiceUid' and 'discoveryUri'. EcsManagementInteropClient.cs 364 (StorageSync)
public class EcsManagementInteropClient : IEcsManagement
{
....
public int PersistSyncServerRegistration(....)
{
return m_managementObject.PersistSyncServerRegistration(
serviceUri,
subscriptionId,
storageSyncServiceName,
resourceGroupName,
clusterId,
clusterName,
storageSyncServiceUid, // <=
discoveryUri, // <=
serviceLocation,
resourceLocation);
}
....
}The analyzer suspected that the order of arguments of the PersistSyncServerRegistration method is confused. Method's declaration:
public interface IEcsManagement : IDisposable
{
....
int PersistSyncServerRegistration(
[In, MarshalAs(UnmanagedType.BStr)]
string serviceUri,
[In, MarshalAs(UnmanagedType.BStr)]
string subscriptionId,
[In, MarshalAs(UnmanagedType.BStr)]
string storageSyncServiceName,
[In, MarshalAs(UnmanagedType.BStr)]
string resourceGroupName,
[In, MarshalAs(UnmanagedType.BStr)]
string clusterId,
[In, MarshalAs(UnmanagedType.BStr)]
string clusterName,
[In, MarshalAs(UnmanagedType.BStr)]
string discoveryUri, // <=
[In, MarshalAs(UnmanagedType.BStr)]
string storageSyncServiceUid, // <=
[In, MarshalAs(UnmanagedType.BStr)]
string serviceLocation,
[In, MarshalAs(UnmanagedType.BStr)]
string resourceLocation);
....
}Indeed, something's wrong here with the arguments number seven and eight. The author has to check the code.
V3077 The setter of 'GetGuid' property does not utilize its 'value' parameter. RecoveryServicesBackupCmdletBase.cs 54 (RecoveryServices)
public abstract class RecoveryServicesBackupCmdletBase : AzureRMCmdlet
{
....
static string _guid;
protected static string GetGuid
{
get { return _guid; }
set { _guid = Guid.NewGuid().ToString(); }
}
....
}The setter doesn't use the passed parameter. Instead, it creates a new GUID and assigns it to the _guid field. I think most readers would agree that such code looks at least ugly. This construction isn't very convenient to use: when (re)initializing the GetGuid property, one has to assign it a fake value, which is not very obvious. But most of all I was amused by the way authors used this pattern. There's only one place, where GetGuid is handled. Check it out:
public override void ExecuteCmdlet()
{
....
var itemResponse = ServiceClientAdapter.CreateOrUpdateProtectionIntent(
GetGuid ?? Guid.NewGuid().ToString(),
....);
....
}Brilliant!
V3091 Empirical analysis. It is possible that a typo is present inside the string literal: "Management Group Id". The 'Id' word is suspicious. Constants.cs 36 (Resources)
public class HelpMessages
{
public const string SubscriptionId = "Subscription Id of the subscription
associated with the management";
public const string GroupId = "Management Group Id"; // <=
public const string Recurse = "Recursively list the children of the
management group";
public const string ParentId = "Parent Id of the management group";
public const string GroupName = "Management Group Id"; // <=
public const string DisplayName = "Display Name of the management group";
public const string Expand = "Expand the output to list the children of the
management group";
public const string Force = "Force the action and skip confirmations";
public const string InputObject = "Input Object from the Get call";
public const string ParentObject = "Parent Object";
}The analyzer pointed to a possible error in the assigned string for the GroupName constant. The conclusion is based on empirical analysis of other assignments, taking into account the names of variables. I think that in this case the analyzer is right, and the value of the GroupName constant should be a kind of "Management Group name". Probably the error took place due to the fact that the value for the GroupId constant was copied, but not changed.
Another similar error:
V3093 The '|' operator evaluates both operands. Perhaps a short-circuit '||' operator should be used instead. PSKeyVaultCertificatePolicy.cs 114 (KeyVault)
internal CertificatePolicy ToCertificatePolicy()
{
....
if (!string.IsNullOrWhiteSpace(SubjectName) ||
DnsNames != null ||
Ekus != null ||
KeyUsage != null | // <=
ValidityInMonths.HasValue)
{
....
}
....
}In this fragment an error might take place and in the if block between two last conditions the || operator might have been used. But as it often happens, only the developer can give the right answer.
V3095 The 'certificate' object was used before it was verified against null. Check lines: 41, 43. CertificateInfo.cs 41 (Automation)
public CertificateInfo(
....,
Azure.Management.Automation.Models.Certificate certificate)
{
....
this.Name = certificate.Name;
if (certificate == null) return;
....
}Classic. First the object is used and only after that the reference is checked for null. We come across such errors very often. Let's consider another similar error.
V3095 The 'clusterCred' object was used before it was verified against null. Check lines: 115, 118. InvokeHiveCommand.cs 115 (HDInsight)
public override void ExecuteCmdlet()
{
....
_credential = new BasicAuthenticationCloudCredentials
{
Username = clusterCred.UserName,
Password = clusterCred.Password.ConvertToString()
};
if (clusterConnection == null || clusterCred == null)
....
}Here's a couple of similar errors:
V3125 The 'startTime' object was used after it was verified against null. Check lines: 1752, 1738. AutomationPSClientDSC.cs 1752 (Automation)
private string GetNodeReportListFilterString(
....,
DateTimeOffset? startTime,
....,
DateTimeOffset? lastModifiedTime)
{
....
if (startTime.HasValue)
{
odataFilter.Add("properties/startTime ge " +
this.FormatDateTime(startTime.Value)); // <=
}
....
if (lastModifiedTime.HasValue)
{
odataFilter.Add("properties/lastModifiedTime ge " +
this.FormatDateTime(startTime.Value)); // <=
}
....
}It is also quite a wide-spread type of errors. The startTime variable is checked for a value presence when first used. But it is not done in the subsequent usage. Well, the situation can be even worse. Look at the second if block. I think the startTime variable mustn't be here at all. Firstly, there is no check for a value presence before its usage. Secondly, the string formed to be passed to the Add method also confirms my suggestion. Another variable (lastModifiedTime) is mentioned in the first part of this string.
V3125 The 'firstPage' object was used after it was verified against null. Check lines: 113, 108. IntegrationAccountAgreementOperations.cs 113 (LogicApp)
public IList<IntegrationAccountAgreement>
ListIntegrationAccountAgreements(....)
{
var compositeList = new List<IntegrationAccountAgreement>();
var firstPage = this.LogicManagementClient.
IntegrationAccountAgreements.List(....);
if (firstPage != null)
{
compositeList.AddRange(firstPage);
}
if (!string.IsNullOrEmpty(firstPage.NextPageLink)) // <=
{
....
}
....
}Another obvious error. The firstPage variable is unsafely used despite the fact that earlier in the code this variable is already used being preliminary checked for null.
I found even more of V3125 warnings in the Azure PowerShell code than V3095 ones described above. All of them are also of the same type. I think, two of them that we've considered are enough.
V3137 The 'apiVersionSetId' variable is assigned but is not used by the end of the function. GetAzureApiManagementApiVersionSet.cs 69 (ApiManagement)
public String ApiVersionSetId { get; set; }
....
public override void ExecuteApiManagementCmdlet()
{
....
string apiVersionSetId;
if (ParameterSetName.Equals(ContextParameterSet))
{
....
apiVersionSetId = ApiVersionSetId;
}
else
{
apiVersionSetId = ....;
}
if (string.IsNullOrEmpty(ApiVersionSetId)) // <=
{
WriteObject(....);
}
else
{
WriteObject(Client.GetApiVersionSet(...., ApiVersionSetId)) // <=
}
}The analyzer reports that the apiVersionSetId local variable was initialized, but not used in any way. Often this pattern indicates an error. I think, in this case it's most likely an error, especially taking into account the fact that the name of the apiVersionSetId local variable and the name of the ApiVersionSetId property differ only by the case of the first letter. Take a look at the code. After initializing apiVersionSetId (by one way or another), only the ApiVersionSetId property is used further in code. It looks extremely suspicious.
V3137 The 'cacheId' variable is assigned but is not used by the end of the function. RemoveAzureApiManagementCache.cs 94 (ApiManagement)
public String CacheId { get; set; }
....
public override void ExecuteApiManagementCmdlet()
{
....
string cacheId;
if (....)
{
....
cacheId = InputObject.CacheId;
}
else if (....)
{
....
cacheId = cache.CacheId;
}
else
{
....
cacheId = CacheId;
}
var actionDescription = string.Format(...., CacheId); // <=
var actionWarning = string.Format(...., CacheId); // <=
....
Client.CacheRemove(resourceGroupName, serviceName, CacheId); // <=
....
}This is the case which is almost the same as the one described earlier. The cacheId local variable isn't used after initialization in any way. Instead, another property with a very similar name CacheId is used. I don't know for sure, may be that's just a programming pattern of Azure PowerShell developers. Anyway, it looks like an error.
V3143 The 'value' parameter is rewritten inside a property setter, and is not used after that. NewAzureIntegrationAccountPartnerCommand.cs 67 (LogicApp)
[Parameter(Mandatory = false,
HelpMessage = "The integration account partner type.",
ValueFromPipelineByPropertyName = false)]
[ValidateSet("B2B", IgnoreCase = false)]
[ValidateNotNullOrEmpty]
public string PartnerType
{
get { return this.partnerType; }
set { value = this.partnerType; } // <=
}The partnerType field is declared in the following way:
/// <summary>
/// Default partner type.
/// </summary>
private string partnerType = "B2B";Despite the name of the solution (LogicApp) where an error was detected, I don't find logic in it. Modifying value in the setter isn't a rare occurrence, but in this case it deals with a loss of the original value. It looks weird. In the code property is read only once. Perhaps, we need to ask for experts' advice again. Maybe I just don't get it. The point is that I came across several same patterns:
These are all interesting bugs that were found in the Azure PowerShell code. Enthusiasts and those who are interested are welcome to review errors themselves in this (or any other) project. I could probably miss something offbeat. To do the review, you only need to download and install PVS-Studio.
Thank you for reading up to the end. And, of course, bugless code to everyone!