{{announcement.body}}
{{announcement.title}}

Huawei Cloud: It's Cloudy in PVS-Studio Today

DZone 's Guide to

Huawei Cloud: It's Cloudy in PVS-Studio Today

Take a look at some of the errors found in the Huawei source code with static code analysis.

· Cloud Zone ·
Free Resource

phone on computer

An analysis of Huawei's source code.

Image title

Many companies have broken into the cloud market segment and created their own cloud services of various purposes. Recently our team has also been interested in integrating the PVS-Studio code analyzer into our own cloud infrastructure. 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.

You may also enjoy:  Using PVS-Studio to Get Beginners Familiar With Code Analysis Tools 

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:

Right when I was looking for an unusual project for the upcoming article, I got an email with a job offer from Huawei. 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.

Project Analysis

When I was doing some research for the article, I found out that Huawei had a developer center 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 prevalent.

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.

To analyze projects, I needed only a few things to do:

  • Get projects from the repository;
  • Use start-up Java analyzer instructions 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):

There were few warnings, which tells us about the 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."

As for analyzing the project, I picked over only the most hotshot warnings, which I'll talk about in this article.

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

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

Conditions predetermination

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

V6007 The 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, 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 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 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 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 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 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 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 constructor of the TrackedFile  class receives the result of the static  FileId.get(path) method as a third argument. But this method can returnnull:

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


In the constructor, called via this, the id the argument doesn't change until its first use:

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 a similar case:

  • V6008 Potential null dereference of buffer. PublishingQueue.java(518)

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


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:

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

Substrings and Negative Numbers

V6009 The substring function could receive the "-1" value whilea 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("/"));
}


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 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, 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:

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


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 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, 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 quite. 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 the use 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 the 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 the usage of all possible tools to improve code quality.

Further Reading 


Why You Probably Don't Use Static Analysis (and How to Change That) 


Setting Up Static Code Analysis for Java

Topics:
java ,huawei ,cloud services ,pvs-studio ,static code analysis

Published at DZone with permission of Valery Komarov . See the original article here.

Opinions expressed by DZone contributors are their own.

{{ parent.title || parent.header.title}}

{{ parent.tldr }}

{{ parent.urlSource.name }}