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.
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:
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 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.
To analyze projects, I needed only a few things to do:
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 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 of analyzing the project I picked over only the most hotshot warnings, which I'll talk about in this article.
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.
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:
public void setSuspend(boolean suspend)
{
this.suspend = suspend;
}As often happens, the V6007 rule breaks ahead in terms of warnings quantity.
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 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 '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:
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 return null:
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:
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 '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())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("/"));
}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:
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));
}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.
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.
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.
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 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:
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, 2) can't fully demonstrate all its advantages. You can download the PVS-Studio analyzer here.