Clever Geek Handbook

Huawei Cloud: it is cloudy in PVS-Studio today





Picture 2






In this century, everyone has heard of cloud services. Many companies have mastered this market segment and created their cloud services in various directions. Our team has also recently been interested in these services in terms of integrating the PVS-Studio code analyzer with them. We think our regular readers already guess what type of project we will check this time. The choice fell on the code of cloud services from Huawei.



Introduction



If you are following the PVS-Studio team, you probably noticed that recently we have been very interested in cloud technologies. We have already published several articles related to this topic:





And so, when I was picking up another interesting project for the upcoming article, I received a job offer from Huawei in the mail. When collecting information about this company, it turned out that they have their own cloud services, but the main thing is that the source code of these services is freely available on GitHub. This is what became the main reason for choosing this company for this article. As one Chinese sage said: “Accidents are not accidental.”



Now a little about our analyzer. PVS-Studio is a static code analyzer that identifies errors and vulnerabilities in the source code of programs written in C, C ++, C # and Java. The analyzer works on the platforms Windows, Linux and macOS. In addition to plugins for such classic development environments as Visual Studio or IntelliJ IDEA, the analyzer has the ability to integrate with SonarQube and Jenkins:





Project analysis



In the process of finding information for the article, I found out that Huawei has a developer center where you can get information, guides, and the source code for their cloud services. A variety of programming languages ​​were used to create these services, but languages ​​such as Go, Java, and Python turned out to be the most popular.



Since I specialize in Java, the projects were chosen accordingly. Sources of the projects analyzed in the article can be obtained from the huaweicloud repository on GitHub.



To analyze the projects, I needed to perform only a few actions:





After analyzing the projects, we managed to single out only three that I would like to pay attention to. This is due to the fact that the size of the remaining Java projects was too small.



Project analysis results (number of warnings and number of files):





There are few warnings, so in general we can say about the good quality of the code. Moreover, not all triggers are valid errors. This is due to the fact that the analyzer sometimes does not have enough information to distinguish the correct code from the incorrect one. Therefore, the analyzer diagnostics are improved every day with the help of information received from users. See also the article “ How and why static analyzers fight false positives ”.



In the process of analyzing projects, I selected the most interesting warnings, which I will discuss in this article.



Field initialization order



V6050 Class initialization cycle is present. Initialization of 'INSTANCE' appears before the initialization of 'LOG'. UntrustedSSL.java (32), UntrustedSSL.java (59), UntrustedSSL.java (33) 



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 any exception occurs in the constructor of the UntrustedSSL class, information about this exception is logged in the catch block using the LOG logger. However, due to the initialization order of static fields, the LOG at the time of initializing the INSTANCE field is not yet initialized. Therefore, when logging exception information in the constructor, a NullPointerException will be thrown. This exception is the cause of another ExceptionInInitializerError exception that is thrown if an exception occurred while initializing the static field. All that is needed to solve the described problem is to place the LOG initialization before the INSTANCE initialization.



Invisible typo



V6005 The variable 'this.metricSchema' is assigned to itself. OpenTSDBSchema.java (72) 



 public class OpenTSDBSchema { @JsonProperty("metric") private List<SchemaField> metricSchema; .... public void setMetricsSchema(List<SchemaField> metricsSchema) { this.metricSchema = metricSchema; // <= } public void setMetricSchema(List<SchemaField> metricSchema) { this.metricSchema = metricSchema; } .... }


Both methods set the metricSchema field, but method names differ by one 's' character. The programmer named the arguments of these methods according to the name of the method. As a result, in the line pointed to by the analyzer, the metricSchema field is assigned to itself, and the argument of the metricsSchema method is not used.



V6005 The variable 'suspend' is assigned to itself. SuspendTransferTaskRequest.java (77) 



 public class SuspendTransferTaskRequest { .... private boolean suspend; .... public void setSuspend(boolean suspend) { suspend = suspend; } .... }


Here is a banal error related to inattention, due to which the assignment of the argument suspend to itself occurs. As a result, the value of the argument arrived will not be assigned to the suspend field, as implied. Correctly: 



 public void setSuspend(boolean suspend) { this.suspend = suspend; }


Predetermined conditions



As often happens, the V6007 rule breaks ahead in terms of the number of warnings.



V6007 Expression 'firewallPolicyId == null' is always false. FirewallPolicyServiceImpl.java (125) 



 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, the arguments are checked for null by the checkNotNull method: 



 @CanIgnoreReturnValue public static <T> T checkNotNull(T reference) { if (reference == null) { throw new NullPointerException(); } else { return reference; } }


After checking the argument with the checkNotNull method, you can be 100% sure that the argument passed to this method is not null . Since both arguments to the removeFirewallRuleFromPolicy method are checked by the checkNotNull method, it makes no sense to check the arguments for null values again. However, an expression is passed to the checkState method as the first argument, where the firewallPolicyId and firewallRuleId arguments are re-checked for null .



A similar warning is issued for firewallRuleId :





V6007 Expression 'filteringParams! = Null' is always true. NetworkPolicyServiceImpl.java (60) 



 private Invocation<NetworkServicePolicies> buildInvocation(Map<String, String> filteringParams) { .... if (filteringParams == null) { return servicePoliciesInvocation; } if (filteringParams != null) { // <= .... } return servicePoliciesInvocation; }


In this method, if the filteringParams argument is null , the method exits and returns a value. Therefore, the test pointed to by the analyzer will always be true, which means that this test does not make sense.



Similar occurs in 13 classes:





Null reference



V6008 Potential null dereference of 'm.blockDeviceMapping'. NovaServerCreate.java (390) 



 @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 will not occur if the blockDevice argument is null . This field is initialized only in this method, so when the add method is called, the m.blockDeviceMapping field will throw a NullPointerException .



V6008 Potential null dereference of 'FileId.get (path)' in function '<init>'. TrackedFile.java (140), TrackedFile.java (115) 



 public TrackedFile(FileFlow<?> flow, Path path) throws IOException { this(flow, path, FileId.get(path), ....); }


The result of the static method FileId.get (path) is passed to the constructor of the TrackedFile class as the third argument. But this method can return null : 



 public static FileId get(Path file) throws IOException { if (!Files.exists(file)) { return null; } .... }


In the constructor called through this , the id argument does not change until it is first used: 



 public TrackedFile(...., ...., FileId id, ....) throws IOException { .... FileId newId = FileId.get(path); if (!id.equals(newId)) { .... } }


Therefore, if null is passed to the method as the third argument, an exception will occur.



A similar situation occurs in one more case:





V6008 Potential null dereference of 'dataTmpFile'. CacheManager.java (91) 



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


And again NPE. A number of checks in the conditional statement allow the dataTmpFile object to be null for further dereferencing. I think there are two typos here, and the check should actually be like this: 



 if (dataTmpFile != null && !dataTmpFile.exists())


Substrings and negative numbers



V6009 The 'substring' function could receive the '-1' value while non-negative value is expected. Inspect argument: 2. RemoveVersionProjectIdFromURL.java (37) 



 @Override public String apply(String url) { String urlRmovePojectId = url.substring(0, url.lastIndexOf("/")); return urlRmovePojectId.substring(0, urlRmovePojectId.lastIndexOf("/")); }


It is understood that the URL is passed to this method as a string, which is not validated in any way. After this string is trimmed several times using the lastIndexOf method. If lastIndexOf does not find a match in the string, it will return -1. This will throw a StringIndexOutOfBoundsException , because the arguments to the substring method must be non-negative numbers. For the method to work correctly, you must add validation of the input argument or verify that the results of the lastIndexOf methods are not negative numbers.



Here are some other places like this:





Forgotten result



V6010 The return value of function 'concat' is required to be utilized. AKSK.java (278) 



 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, it was not taken into account that calling the concat method will not change the host string due to the immutability of objects of type String . For the method to work correctly, you must assign the result of the concat method to the host variable in the if block. Correctly: 



 if (port > -1) { host = host.concat(":" + Integer.toString(port)); }


Unused variables



V6021 Variable 'url' is not used. TriggerV2Service.java (95) 



 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 is not used after initialization. Most likely, the url variable should be passed to the uri method as the second argument instead of functionUrn , since the variable functionUrn is involved in the initialization of the url variable.



Argument not used in constructor



V6022 Parameter 'returnType' is not used inside constructor body. HttpRequest.java (68) 



 public class HttpReQuest<R> { .... Class<R> returnType; .... public HttpRequest(...., Class<R> returnType) // <= { this.endpoint = endpoint; this.path = path; this.method = method; this.entity = entity; } .... public Class<R> getReturnType() { return returnType; } .... }


In this constructor, they forgot to use the returnType argument and assign its value to the returnType field. Therefore, when calling the getReturnType method, the object created by this constructor will return the default value of null , although it was most likely intended to get the object that was previously passed to the constructor.



Same functionality



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) 



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


The presence of two identical methods is not an error, 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 perform opposite actions. In fact, both methods do the same thing - they create and return a ServiceAction object. Most likely, the disable method was created by copying the code of the enable method, but forgot to change the method body.



Forgot to check the main thing



V6060 The 'params' reference was utilized before it was verified against null. DomainService.java (49), DomainService.java (46) 



 public Domains list(Map<String, String> params) { Preconditions.checkNotNull(params.get("page_size"), ....); Preconditions.checkNotNull(params.get("page_number"), ....); Invocation<Domains> domainInvocation = get(Domains.class, uri("/domains")); if (params != null) { // <= .... } return domainInvocation.execute(this.buildExecutionOptions(Domains.class)); }


In this method, we decided to check the contents of a structure of type Map for the null inequality. To do this, the params argument calls the get method twice, the result of which is passed to the checkNotNull method. Everything seems logical, but no matter how! In if , the params argument is checked for null . After which it can be assumed that the input argument may be null , but before this check, the get method was called on params twice already. If null is passed as an argument to this method, then the first time the get method is called, an exception will be thrown.



A similar situation occurs in three more places:





Conclusion



Modern large companies nowadays simply can not do without the use of cloud services. A huge number of people use these services, so even the slightest mistake in the service can lead to problems for many people, as well as to additional costs for the company to correct the consequences of this error. In developing it is always necessary to take into account the human factor, since any person sooner or later makes mistakes, and this article is an example of this. That is why you should use all possible tools to improve the quality of the code.



PVS-Studio will certainly inform Huawei about the results of checking their cloud services so that the developers of this company can study them in more detail.



The one-time use of static code analysis demonstrated in the article cannot show all its advantages. More information on how to use static analysis correctly can be found here and here . You can download the PVS-Studio analyzer here .











If you want to share this article with an English-speaking audience, then please use the link to the translation: Valery Komarov. Huawei Cloud: It's Cloudy in PVS-Studio Today .


More articles:


All Articles