# Huawei cloud: it's cloudy in PVS\-Studio today Nowadays everyone knows about cloud services\. Many companies have cracked this market segment and created their own cloud services of various purposes\. Recently our team has also been interested in these services in terms of integrating the PVS\-Studio code analyzer into them\. Chances are, our regular readers have already guessed what type of project we will check this time\. The choice fell on the code of Huawei cloud services\. ![0688_HuaweiCloud/image1.png](https://import.viva64.com/docx/blog/0688_HuaweiCloud/image1.png) ## Introduction If you're following PVS\-Studio team posts, you've probably noticed that we had been digging deep in cloud technologies lately\. We have already published several articles covering this topic: * [PVS\-Studio in the clouds: Azure DevOps](https://pvs-studio.com/en/blog/posts/csharp/0670/) * [PVS\-Studio in the clouds: Travis CI](https://pvs-studio.com/en/blog/posts/cpp/0636/) * [PVS\-Studio in the clouds: CircleCI](https://pvs-studio.com/en/blog/posts/cpp/0676/) * [PVS\-Studio in the clouds: GitLab CI/CD](https://pvs-studio.com/en/blog/posts/cpp/0686/) Right when I was looking for an unusual project for the upcoming article, I got an email with a job offer from [Huawei](https://www.huawei.com/en/)\. After collecting some information about this company, it turned out that they had their own cloud services, but the main thing is that the source code of these services is available on GitHub\. This was the main reason for choosing this company for this article\. As one Chinese sage said: "The accidents are not accidental"\. Let me give you some details about our analyzer\. PVS\-Studio is a static analyzer for bug detection in the source code of programs, written in C, C\+\+, C\#, and Java\. The analyzer works on Windows, Linux, and macOS\. In addition to plugins for classic development environments, such as Visual Studio or IntelliJ IDEA, the analyzer has the ability to integrate into SonarQube and Jenkins: * [Integration of PVS\-Studio analysis results into SonarQube](https://pvs-studio.com/en/docs/manual/0037/) * [PVS\-Studio running in Jenkins](https://pvs-studio.com/en/docs/manual/0048/) ## Project analysis When I was doing some research for the article, I found out that Huawei had a [developer center](https://developer.huaweicloud.com/en-us) with available information, manuals, and sources of their cloud services\. A wide variety of programming languages were used to create these services, but languages such as Go, Java and Python were the most prevailing\. Since I specialize in Java, the projects have been selected in keeping with my knowledge and skills\. You can get project sources analyzed in the article in a GitHub repository [huaweicloud](https://github.com/huaweicloud/)\. To analyze projects, I needed only a few things to do: * Get projects from the repository; * Use start\-up Java analyzer [instructions](https://pvs-studio.com/en/docs/manual/6703/) and run the analysis on each project\. Having analyzed the projects, we selected only three of them, which we would like to pay attention to\. It is because of the fact that the size of the rest Java projects turned out to be too small\. Project analysis results \(number of warnings and number of files\): * [huaweicloud\-sdk\-java](https://github.com/huaweicloud/huaweicloud-sdk-java): 31 \- High, 2 \- Medium and 16 \- Low, 2700\+ files\. * [huaweicloud\-dis\-agent](https://github.com/huaweicloud/huaweicloud-dis-agent): 7 \- High, 6 \- Medium and 6 \- Low, 100\+ files\. * [huaweicloud\-sdk\-java\-dis](https://github.com/huaweicloud/huaweicloud-sdk-java-dis): 15 \- High, 6 \- Medium and 16 \- Low, 270\+ files\. There were few warnings, which tells us about high quality of code, all the more so since not all warnings point at real errors\. This is due to the fact that the analyzer sometimes lacks information to distinguish the correct code from the erroneous one\. For this reason we tweak analyzer's diagnostics day by day with recourse to the information from users\. You're welcome to see the article "[The way static analyzers fight against false positives, and why they do it](https://pvs-studio.com/en/blog/posts/cpp/0488/)"\. As of analyzing the project I picked over only the most hotshot warnings, which I'll talk about in this article\. ## Fields initialization order [V6050](https://pvs-studio.com/en/docs/warnings/v6050/) Class initialization cycle is present\. Initialization of 'INSTANCE' appears before the initialization of 'LOG'\. UntrustedSSL\.java\(32\), UntrustedSSL\.java\(59\), UntrustedSSL\.java\(33\) ```cpp public class UntrustedSSL { private static final UntrustedSSL INSTANCE = new UntrustedSSL(); private static final Logger LOG = LoggerFactory.getLogger(UntrustedSSL.class); .... private UntrustedSSL() { try { .... } catch (Throwable t) { LOG.error(t.getMessage(), t); // <= } } } ``` If there is any exception in the _UntrustedSSL _class constructor, the information about this exception is logged in the _catch _block using the _LOG_ logger\. However, due to the initialization order of static fields, at the moment of initializing the _INSTANCE _field, _LOG _isn't initialized yet\. Therefore, if you log information about the exception in the constructor, it will result in _NullPointerException_\. This exception is the reason for another exception _ExceptionInInitializerError_, which is thrown if there had been an exception when the static field had been initialized\. What you need to solve this problem is to place _LOG_ initialization before _INSTANCE_ initializing_\._ ## Inconspicuous typo [V6005](https://pvs-studio.com/en/docs/warnings/v6005/) The variable 'this\.metricSchema' is assigned to itself\. OpenTSDBSchema\.java\(72\) ```cpp public class OpenTSDBSchema { @JsonProperty("metric") private List metricSchema; .... public void setMetricsSchema(List metricsSchema) { this.metricSchema = metricSchema; // <= } public void setMetricSchema(List metricSchema) { this.metricSchema = metricSchema; } .... } ``` Both methods set the _metricSchema _field, but the method's names differ by one 's' symbol\. The programmer named the arguments of these methods according to the name of the method\. As a result, in the line the analyzer points to, the _metricSchema _field is_ _assigned to itself, and the _metricsSchema_ method's argument is not used\. [V6005](https://pvs-studio.com/en/docs/warnings/v6005/) The variable 'suspend' is assigned to itself\. SuspendTransferTaskRequest\.java\(77\) ```cpp public class SuspendTransferTaskRequest { .... private boolean suspend; .... public void setSuspend(boolean suspend) { suspend = suspend; } .... } ``` Here is a trivial error related to carelessness, because of which the _suspend _argument is assigned to itself\. As a result, the _suspend_ field won't be assigned the value of the obtained argument as implied\. The correct version: ```cpp public void setSuspend(boolean suspend) { this.suspend = suspend; } ``` ## Conditions predetermination As often happens, the [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) rule breaks ahead in terms of warnings quantity\. [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'firewallPolicyId \=\= null' is always false\. FirewallPolicyServiceImpl\.java\(125\) ```cpp public FirewallPolicy removeFirewallRuleFromPolicy(String firewallPolicyId, String firewallRuleId) { checkNotNull(firewallPolicyId); checkNotNull(firewallRuleId); checkState(!(firewallPolicyId == null && firewallRuleId == null), "Either a Firewall Policy or Firewall Rule identifier must be set"); .... } ``` In this method arguments are checked for _null_ by the _checkNotNull_ method: ```cpp @CanIgnoreReturnValue public static T checkNotNull(T reference) { if (reference == null) { throw new NullPointerException(); } else { return reference; } } ``` After checking the argument by the _checkNotNull _method, you can be 100% sure that the argument passed to this method is not equal to _null_\. Since both arguments of the _removeFirewallRuleFromPolicy _method are checked by the _checkNotNull _method, their further check for _null_ makes no sense\. However, the expression, where _firewallPolicyId_ and _firewallRuleId _arguments are re\-checked for _null_, is passed as the first argument to the _checkState_ method\. A similar warning is issued for _firewallRuleId_ as well: * V6007 Expression 'firewallRuleId \=\= null' is always false\. FirewallPolicyServiceImpl\.java\(125\) [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'filteringParams \!\= null' is always true\. NetworkPolicyServiceImpl\.java\(60\) ```cpp private Invocation buildInvocation(Map filteringParams) { .... if (filteringParams == null) { return servicePoliciesInvocation; } if (filteringParams != null) { // <= .... } return servicePoliciesInvocation; } ``` In this method, if the _filteringParams_ argument is _null_, the method returns a value\. This is why the check that the analyzer points to will always be true which, in turns, means that this check is meaningless\. 13 more classes are similar: * V6007 Expression 'filteringParams \!\= null' is always true\. PolicyRuleServiceImpl\.java\(58\) * V6007 Expression 'filteringParams \!\= null' is always true\. GroupServiceImpl\.java\(58\) * V6007 Expression 'filteringParams \!\= null' is always true\. ExternalSegmentServiceImpl\.java\(57\) * V6007 Expression 'filteringParams \!\= null' is always true\. L3policyServiceImpl\.java\(57\) * V6007 Expression 'filteringParams \!\= null' is always true\. PolicyRuleSetServiceImpl\.java\(58\) * and so on \. \. \. ## Null reference [V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Potential null dereference of 'm\.blockDeviceMapping'\. NovaServerCreate\.java\(390\) ```cpp @Override public ServerCreateBuilder blockDevice(BlockDeviceMappingCreate blockDevice) { if (blockDevice != null && m.blockDeviceMapping == null) { m.blockDeviceMapping = Lists.newArrayList(); } m.blockDeviceMapping.add(blockDevice); // <= return this; } ``` In this method, the initialization of the _m\.blockDeviceMapping_ reference field won't happen if the _blockDevice_ argument is _null_\. This field is initialized only in this method, so when calling the _add_ method from the _m\.blockDeviceMapping_ field, a _NullPointerException_ will happen\. [V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Potential null dereference of 'FileId\.get\(path\)' in function ''\. TrackedFile\.java\(140\), TrackedFile\.java\(115\) ```cpp public TrackedFile(FileFlow flow, Path path) throws IOException { this(flow, path, FileId.get(path), ....); } ``` The constructor of the _TrackedFile_ class receives the result of the static _FileId\.get\(path\) _method as a third argument\. But this method can return _null_: ```cpp public static FileId get(Path file) throws IOException { if (!Files.exists(file)) { return null; } .... } ``` In the constructor, called via _this_, the _id_ argument doesn't change until its first use: ```cpp public TrackedFile(...., ...., FileId id, ....) throws IOException { .... FileId newId = FileId.get(path); if (!id.equals(newId)) { .... } } ``` As we can see, if _null_ is passed as the third argument to the method, an exception will occur\. Here is another similar case: * V6008 Potential null dereference of 'buffer'\. PublishingQueue\.java\(518\) [V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Potential null dereference of 'dataTmpFile'\. CacheManager\.java\(91\) ```cpp @Override public void putToCache(PutRecordsRequest putRecordsRequest) { .... if (dataTmpFile == null || !dataTmpFile.exists()) { try { dataTmpFile.createNewFile(); // <= } catch (IOException e) { LOGGER.error("Failed to create cache tmp file, return.", e); return ; } } .... } ``` NPE again\. A number of checks in the conditional operator allows the zero object _dataTmpFile_ for further dereference\. I think there are two typos here and the check should actually look like this: ```cpp if (dataTmpFile != null && !dataTmpFile.exists()) ``` ## Substrings and negative numbers [V6009](https://pvs-studio.com/en/docs/warnings/v6009/) The 'substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect argument: 2\. RemoveVersionProjectIdFromURL\.java\(37\) ```cpp @Override public String apply(String url) { String urlRmovePojectId = url.substring(0, url.lastIndexOf("/")); return urlRmovePojectId.substring(0, urlRmovePojectId.lastIndexOf("/")); } ``` The implication is that this method gets a URL as a string, which is not validated in any way\. Later, this string is cut off several times using the _lastIndexOf_ method\. If the method _lastIndexOf_ doesn't find a match in the string, it will return \-1\. This will lead to _StringIndexOutOfBoundsException_, as the arguments of the _substring_ method have to be non\-negative numbers\. For correct method's operation, one has to add an input argument validation or check that the results of the _lastIndexOf_ method are non\-negative numbers\. Here are some other snippets with a similar way things are: * V6009 The 'substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect argument: 2\. RemoveProjectIdFromURL\.java\(37\) * V6009 The 'substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect argument: 2\. RemoveVersionProjectIdFromURL\.java\(38\) ## Forgotten result [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'concat' is required to be utilized\. AKSK\.java\(278\) ```cpp public static String buildCanonicalHost(URL url) { String host = url.getHost(); int port = url.getPort(); if (port > -1) { host.concat(":" + Integer.toString(port)); } return host; } ``` When writing this code, its author didn't take into account that a call of the _concat _method won't change the _host _string due to immutability of the _String _type objects\. For correct method's operation, the result of the _concat _method has to be assigned to the _host _variable in the _if_ block\. The correct version: ```cpp if (port > -1) { host = host.concat(":" + Integer.toString(port)); } ``` ## Unused variables [V6021](https://pvs-studio.com/en/docs/warnings/v6021/) Variable 'url' is not used\. TriggerV2Service\.java\(95\) ```cpp public ActionResponse deleteAllTriggersForFunction(String functionUrn) { checkArgument(!Strings.isNullOrEmpty(functionUrn), ....); String url = ClientConstants.FGS_TRIGGERS_V2 + ClientConstants.URI_SEP + functionUrn; return deleteWithResponse(uri(triggersUrlFmt, functionUrn)).execute(); } ``` In this method, the _url_ variable isn't used after its initialization\. Most likely, the _url_ variable has to be passed to the _uri_ method as a second argument instead of _functionUrn_, as the _functionUrn_ variable takes part in the initialization of the _url_ variable\. ## Argument not used the constructor [V6022](https://pvs-studio.com/en/docs/warnings/v6022/) Parameter 'returnType' is not used inside constructor body\. HttpRequest\.java\(68\) ```cpp public class HttpReQuest { .... Class returnType; .... public HttpRequest(...., Class returnType) // <= { this.endpoint = endpoint; this.path = path; this.method = method; this.entity = entity; } .... public Class getReturnType() { return returnType; } .... } ``` In this constructor, the programmer forgot to use the _returnType _argument, and assign its value to the _returnType _field\. That's why when calling the _getReturnType _method from the object, created by this constructor, _null_ will be returned by default\. But most likely, the programmer intended to get the object, previously passed to the constructor\. ## Same functionality [V6032](https://pvs-studio.com/en/docs/warnings/v6032/) It is odd that the body of method 'enable' is fully equivalent to the body of another method 'disable'\. ServiceAction\.java\(32\), ServiceAction\.java\(36\) ```cpp public class ServiceAction implements ModelEntity { private String binary; private String host; private ServiceAction(String binary, String host) { this.binary = binary; this.host = host; } public static ServiceAction enable(String binary, String host) { // <= return new ServiceAction(binary, host); } public static ServiceAction disable(String binary, String host) { // <= return new ServiceAction(binary, host); } .... } ``` Having two identical methods is not a mistake, but the fact that two methods perform the same action is at least strange\. Looking at the names of the above methods, we can assume that they should perform the opposite actions\. In fact, both methods do the same thing \- create and return the _ServiceAction _object\. Most likely, the _disable _method was created by copying the _enable_ method's code, but the method's body remained the same\. ## Forgot to check the main thing [V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'params' reference was utilized before it was verified against null\. DomainService\.java\(49\), DomainService\.java\(46\) ```cpp public Domains list(Map params) { Preconditions.checkNotNull(params.get("page_size"), ....); Preconditions.checkNotNull(params.get("page_number"), ....); Invocation domainInvocation = get(Domains.class, uri("/domains")); if (params != null) { // <= .... } return domainInvocation.execute(this.buildExecutionOptions(Domains.class)); } ``` In this method, the author decided to check the contents of a structure of the _Map _type for _null_\. To do this, the _get_ method is called twice from the _params _argument\. The result of the _get_ method is passed to the _checkNotNull _method\. Everything seems logical, but it's not like that\! The _params _argument is checked for _null_ in _if_\. After this it is expected that the input argument might be _null_, but before this check, the _get _method has already been called twice from _params\._ If _null _is passed as an argument to this method, the first time you call the _get_ method, an exception will be thrown\. A similar situation occurs in three other places: * V6060 The 'params' reference was utilized before it was verified against null\. DomainService\.java\(389\), DomainService\.java\(387\) * V6060 The 'params' reference was utilized before it was verified against null\. DomainService\.java\(372\), DomainService\.java\(369\) * V6060 The 'params' reference was utilized before it was verified against null\. DomainService\.java\(353\), DomainService\.java\(350\) ## Conclusion Today's large companies can't do without usage of cloud services\. A huge number of people use these services\. In this view, even a small error in a service might lead to problems for many people as well as to additional losses, racked up by a company to remedy adverse consequences of this error\. Human flaws should always be taken into account especially since sooner or later everyone makes mistakes, as described in this article\. This fact substantiates usage of all possible tools to improve the code quality\. PVS\-Studio will definitely inform the Huawei company about the results of checking their cloud services so as to Huawei developers could dwell on them, because one\-time usage of static code analysis covered by this articles \([1](https://pvs-studio.com/en/blog/posts/cpp/0594/), [2](https://pvs-studio.com/en/blog/posts/cpp/0639/)\) can't fully demonstrate all its advantages\. You can download the PVS\-Studio analyzer [here](https://pvs-studio.com/en/pvs-studio/download/)\.