Clever Geek Handbook

Azure PowerShell: “basically harmless”





Picture 6






Hello to all. Today we have another Microsoft project on the test. By the title of the article, you can guess that this time the developers could not please us with a lot of mistakes. We hope that the authors of the project will not be offended by the name. After all, a small number of errors is excellent, is not it? However, something interesting was found in the Azure PowerShell code. We suggest that you familiarize yourself with the features of this project and take a look at the errors found with the PV # C-analyzer PVS-Studio.



about the project



Azure PowerShell is a cmdlet that lets you manage Azure resources directly from the PowerShell command line. The main purpose of this set is to simplify learning and quick start of development for Azure. Azure PowerShell provides administrators and developers with powerful features for creating, deploying, and managing Microsoft Azure applications.



Azure PowerShell is developed in the .NET Standard environment and is supported by PowerShell 5.1 for Windows and PowerShell 6.x and higher for all platforms. Azure PowerShell source code is available on GitHub.



Recently, Microsoft projects often come to my attention. In my opinion, the quality of these projects is usually at their best. Although, of course, not without exceptions, as was the article " WinForms: errors, Holmes ". However, this time everything is normal. The project is large: 6845 files .cs source code contain approximately 700 thousand lines, excluding empty (tests, as well as warnings of the third level of reliability, I did not take into account). There were very few errors for such a volume of code: not more than a hundred. There are quite a few situations of the same type, so for the article I selected the most interesting positives. Errors, as I usually do, are sorted by the PVS-Studio diagnostic numbers.



I also came across code fragments that look like errors, but I couldn’t make an unambiguous conclusion about the existence of a real problem, as I’m not familiar enough with the design features for PowerShell. I hope that among readers there will be experts in this matter and will help me. About it below.



Before the start of the “debriefing,” I note the design feature in terms of its structure. Azure PowerShell source code consists of more than seventy Visual Studio solutions. Some solutions include projects from other solutions. Such a structure slowed down the analysis a little (not much). The rest of the verification did not cause difficulties. For convenience, in the line of the error message (in brackets) I will indicate the name of the solution in which this error was detected.



Validation Results



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, which only the developer is able to fix. In this case, it is completely incomprehensible whether we are dealing with duplication of code that does not affect anything, or instead of “M” something else should appear in one of the last two 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 is probably no mistake here. This is an example of redundant code. Sometimes such code may indicate an insufficient level of developer knowledge. The thing is, this.AggregationType! = Null and this.AggregationType.HasValue checks are identical. It is enough to use one of them (any). Personally, I prefer the HasValue option : 



 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 ); } .... }


The 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 in work. The question again is the criticality of this error. If this is not a simple duplication of code, then we can talk about the lack of the necessary verification and the corresponding set of actions. The author needs 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 assigned to itself. Take a look at his ad: 



 [JsonProperty(PropertyName = "provisionVMAgent")] public bool? ProvisionVMAgent { get; set; }


The JsonProperty description says “Instructs the Newtonsoft.Json.JsonSerializer to always serialize the member with the specified name”. Everything seems to be unsophisticated and an obvious mistake has been made. Also confusing is the explicit use of this to access a property. Perhaps, by mistake, some other variable was not specified instead of this . But for now, we will not draw conclusions. The fact is that in the Azure PowerShell code I met quite a lot of such assignments (the property is assigned to itself). Here's another example of an assignment that is 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; .... }


Pay attention to the second and subsequent assignments. On the right side of the expression, it is not this that appears at all, but details . Take a look at the declaration of this.LastHeartbeat property: 



 public DateTime? LastHeartbeat { get; set; }


Finally, look for a property with the same name in the InMageAzureV2ReplicationDetails class. Such a property is declared there: 



 public class InMageAzureV2ReplicationDetails : ReplicationProviderSpecificSettings { .... [JsonProperty(PropertyName = "lastHeartbeat")] public DateTime? LastHeartbeat { get; set; } .... }


Well, in this case, I am ready to admit that this operation is a real mistake. But what to do with the following triggers? In them, unlike the two previous code fragments, there is a multiple assignment of properties to themselves. This is already 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 consecutive assignments of properties to themselves. Just in case, I gave a complete declaration of the RemoveExpressRouteConnectionCommand class with all its attributes, as well as a declaration of the ResourceGroupName property (the other two properties are declared in the same way). It was such triggering that made me think about the question: “Is this a mistake?” I suspect that some kind of internal magic of PowerShell development can happen here. I hope that among the readers there are experts who are knowledgeable in this matter. I am not ready to make any conclusion 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 } .... }


Skipped the throw keyword. Moreover, the comment says that the exception should just be thrown. In the RecoveryServices solution, I encountered several more similar errors:





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 work is violated. If apiType contains a value, then control will not 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") , and apiType.Value.ToString ("g") , respectively, 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; .... }


The paradoxical code. Two checks that contradict each other. Probably the second null equality check 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"); } .... }


Checking is pointless, and an exception will never be thrown. The condition requires the simultaneous equality of the linkedServiceType variable to three different values. The && and || operators are probably confused. Corrected 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); } .... }


Extra check of Ekus variable for equality null . Probably nothing wrong, but the code looks ugly.



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); } }


Excessive check, or erroneous condition. Checking RetentionTimes.Count! = 1 is meaningless, because before that, checking RetentionTimes.Count == 0 .



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))) { .... } .... }


Error in format string. The {0} qualifier is used twice, with two arguments passed to the Format method. The correct option: 



 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 a VirtualMachineScaleSetVMProfile object, the imageAndOsType variable is used without checking for null . However, further, when creating VirtualMachineScaleSetStorageProfile , this variable is already checked using the conditional access operator, and twice. The code looks unsafe.



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(....); // <= } .... }


As in the case of normal execution, and as a result of handling the exception, the existingContacts variable can get null , after which work will continue. Further in the code this variable is used without any verification.



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 passing arguments to the PersistSyncServerRegistration method was confused . Method 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 is mixed up here with arguments number seven and eight. Verification of the code by the author is required.



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 does not use the transmitted parameter. Instead, they get a new GUID and assign it to the _guid field. I think most readers will agree that such code looks at least ugly. Using such a construction is not quite convenient: to (re) initialize the GetGuid property, you must assign it some fake value, which is completely unobvious. But most of all I was amused by the moment how the authors themselves use this construction. There is only one place in the code where GetGuid works . Take a look: 



 public override void ExecuteCmdlet() { .... var itemResponse = ServiceClientAdapter.CreateOrUpdateProtectionIntent( GetGuid ?? Guid.NewGuid().ToString(), ....); .... }


Sumptuously!



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 indicated a possible error in the assigned string for the GroupName constant. The conclusion is based on an empirical analysis of other assignments taking into account variable names. I think that in this case the analyzer is right, and the value of the GroupName constant should be of the form “Management Group name”. The error was probably caused by the fact that the value for the GroupId constant was copied , but they forgot to change it.



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) { .... } .... }


There is a suspicion that a mistake has been made, and the || operator should also be used between the last conditions in the if block. The exact answer, as often happens, can only be given by the developer.



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 then is the link checked for null . We encounter such errors very often . 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) .... }


And a couple more 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)); // <= } .... }


Also a fairly common type of error. The variable startTime was checked for the presence of a value upon first use. The next time you use this, do not. But it could be even worse. Take a look at the second if block. I think there should not be a startTime variable here at all. This is indicated by both the absence of its check for the presence of a value before use, and the string formed for passing to the Add method. The first part of this line mentions another variable ( lastModifiedTime ).



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 mistake. The firstPage variable is used insecurely, although earlier in the code there is a use of this variable with a preliminary check for null equality.



V3125 in Azure PowerShell code was even more successful than V3095 described earlier. All of them are also of the same type. I think we can limit ourselves to the two that we examined.



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 local variable apiVersionSetId has been initialized, but has not been used. Often such a pattern indicates an error. I think in this case the probability of error is high, given that the name of the local variable apiVersionSetId and the name of the property ApiVersionSetId differ only in the first letter case. Take a look at the code. After initializing apiVersionSetId (in one way or another), then only the ApiVersionSetId property is used in the code. Very, very 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); // <= .... }


The situation, almost exactly repeating the previously considered. The local variable cacheId is not used after initialization. Instead, they use a property with a very similar name, CacheId . I don’t know, maybe this is such a pattern among Azure PowerShell developers. But it looks like a mistake.



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 like this: 



 /// <summary> /// Default partner type. /// </summary> private string partnerType = "B2B";


Contrary to the name of the solution in which this error was detected (LogicApp), I do not see the logic here. Using value in the setter for recording is not such a rare occurrence, but in this case we are talking about losing the original value. It looks weird. The code has a single read access to this property. Perhaps you should again seek expert advice. Maybe I don’t understand something. The fact is that I met a few more of the exact same patterns:





Conclusion



That's all the interesting bugs that were found in the Azure PowerShell code. Enthusiasts and just interested, I suggest conducting an independent study of errors in the code of this (or any other) project.I probably could have missed something else interesting. To do this, just download and install PVS-Studio .



Thank you for reading. A codeless code!











If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Azure PowerShell: Mostly Harmless .


More articles:


All Articles