转载

【译】华为云——代码质量“多云”

原文链接

使用静态代码分析看一看华为的源代码。

出于各种原因，有许多企业进入了云市场并建立了他们自己的云服务。最近，我们的团队致力于将PVS-Studio代码分析工具集成到我们的云架构中。我们的忠实读者可能已经猜到我们这次要拿什么项目开刀，没错，就是华为的云服务。

介绍

如果你订阅了PVS-Studio团队的帖子，你可能已经注意到我们最近正在深挖云技术。我们已经发布了好几篇相关的文章：

PVS-Studio in the Clouds: Azure DevOps
PVS-Studio in the Clouds: Travis CI
PVS-Studio in the Clouds: CircleCI
PVS-Studio in the Clouds: GitLab CI/CD

当我正在为这篇文章苦思冥想一个非同寻常的项目的时候，我收到了一封来自于华为的邮件，邮件中给了我一份Offer。查看了一下这个公司的信息之后，我发现他们拥有自己的云服务，最重要的是它们的源代码在GitHub上开源。这就是我在这篇文章中选择华为云的原因。正如一句中国老话说的那样：“相请不如偶遇”。

让我详细介绍一下我们的分析工具。PVS-Studio是一个静态分析工具，用来检查bug，适用于C，C++，C#以及Java。分析工具可以在Windows,Linux以及macOS上工作。除了可以作为常用开发工具例如Visual Studio，IntelliJ IDEA的插件之外，还可以集成到SonarQube以及Jenkins。

项目分析

在我为这篇文章做调研的时候，我发现华为有一个研发中心，提供了可用的信息，手册以及他们云服务的源代码。这些服务由许多种不同的语言编写，其中Go,Java，Python最常见。

由于我司职Java，我就挑选了相应的项目。你可以从华为的GitHub仓库中获取源代码。

要分析这个项目，要做一些准备工作：

从仓库中获取源代码；
使用 Java分析工具指南分析每个项目。

分析这些项目之后，我选择了其中的三个，原因是其他的Java项目太小了，我们着重看一看这三个项目。

分析结果（警告数已经文件数）：

huaweicloud-sdk-java :31——高，2——中，16——低，2700+文件。
huaweicloud-dis-agent :7——高，6——中，6——低，100+文件。
huaweicloud-sdk-java-dis :15——高，6——中，16——低，270+文件。

警告很少，说明代码质量很高。而且并不是所有的警告都指向真正的错误，这更能说明代码质量很高。这是因为有时候分析器缺少足够的信息来区分正确的代码与错误的代码。基于此，我们求助于用户提供的信息日复一日地调整分析器的诊断。欢迎查看这篇文章静态分析器与误报的斗争以及原因。

对于分析的项目，我挑选了最值得关注的警告用于接下来的讨论。

字段初始化顺序

V6050 存在类初始化循环。 INSTANCE 的初始化出现在 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);           // <=
    }
  }
}
复制代码

如果 UntrustedSSL 类的构造函数出现异常，会在 catch 块中使用 LOG 记录异常信息。然而，由于静态字段的初始化书序，在初始化 INSTANCE 字段的时候， LOG 还没有初始化，因此，会导致 NullPointerException ，这个异常是引发另一个异常 ExceptionInInitializerError 的原因，该异常会在静态字段初始化异常时抛出。解决这个问题的办法是将 LOG 的初始化放到 INSTANCE 之前。

不起眼的打字错误

V6005 变量 this.metricSchema 被赋值给自己。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;
  }
  ....
}
复制代码

两个方法都设置 metricSchema 字段，但是方法名一个 s 之差，程序员根据方法名来命名参数名，结果，代码分析工具指出， metricSchema 被赋值给自身，而且参数 metricsSchema 未使用。

V6005 变量 suspend 被赋值给自己。 SuspendTransferTaskRequest.java(77)

public class SuspendTransferTaskRequest 
{
  ....
  private boolean suspend;
  ....
  public void setSuspend(boolean suspend)
  {
    suspend = suspend;                        
  }
  .... 
}
复制代码

这是因为粗心大意引起的错误，结果 suspend 不能按给定的参数赋值。正确的形式是：

public void setSuspend(boolean suspend)
{
  this.suspend = suspend;                        
}
复制代码

条件预定义

V6007 规则在警告数量上常常有所突破。

V6007 表达式 firewallPolicyId == null 总是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"); 
  .... 
}
复制代码

在这个方法中， checkNotNull 方法检查参数是否为 null ：

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

在checkNotNull方法之后，你可以100%确定参数不会为null，由于 removeFirewallRuleFromPolicy 都经过 checkNotNull 方法检查，之后再检查是不是null没有意义。

一个类似的警告也是由于 firewallRuleId ：

V6007 表达式 firewallRuleId == null 总是false。FirewallPolicyServiceImpl.java(125)。

filteringParams != null 总是true。NetworkPolicyServiceImpl.java(60)

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

在这个方法中，如果参数 filteringParams 是null,方法返回一个值，这就是为什么分析器指出检查结果总是true,这意味着检查没有意义。

还有13个类似的警告：

V6007 表达式'filteringParams != null'总是true. PolicyRuleServiceImpl.java(58)
V6007 表达式'filteringParams != null' 总是true. GroupServiceImpl.java(58)
V6007 表达式'filteringParams != null' 总是true.ExternalSegmentServiceImpl.java(57)
V6007 表达式'filteringParams != null' 总是true. L3policyServiceImpl.java(57)
V6007 表达式n 'filteringParams != null' 总是true. PolicyRuleSetServiceImpl.java(58)
...

空引用

V6008 潜在的空引用 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;
}
复制代码

在这个方法里，如果 blockDevice 为空， m.blockDeviceMapping 就不会初始化，该字段仅在这个方法中被初始化，因此如果调用 m.blockDeviceMapping 的 add 方法就会出现空指针异常。

V6008 潜在的空引用 FileId.get(path) 。 TrackedFile.java(140), TrackedFile.java(115)

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

TrackedFile的构造函数取静态方法 FileId.get(path) 的结果最为第三个参数，但是该方法可能会返回null：

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

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

可见，如果传入null作为第三个参数，会引起异常。

还有一个类似的地方： V6008 潜在的空引用 buffer . PublishingQueue.java(518)

V6008 潜在的空引用 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 ;
    }
  }
  ....
}
复制代码

我认为这里有两处打字错误，正确的应该是：

if (dataTmpFile != null && !dataTmpFile.exists())
复制代码

Substrings与负数

V6009 substring函数需要非负数作为参数，但是可能接收到"-1"，请检查第二个参数。RemoveVersionProjectIdFromURL.java(37)

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

假设方法获得一个URL作为字符串，没有做任何验证。之后，使用 lastIndexOf 方法将字符串切开很多次，如果 lastIndexOf 没有找到匹配的字符串，会返回"-1"。这会导致StringIndexOutOfBoundsException，因为substring只接受非负数。正确的操作，需要加一个参数验证，或者检查lastIndexOf的返回值。

还有几处片段有同样的问题

V6009 substring函数需要非负数作为参数，但是可能接收到"-1"，请检查第二个参数。RemoveProjectIdFromURL.java(37)
V6009 substring函数需要非负数作为参数，但是可能接收到"-1"，请检查第二个参数。 RemoveVersionProjectIdFromURL.java(38)

被遗忘的结果

V6021 变量url未使用。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();
}
复制代码

在这个方法里，参数url在初始化后没有使用。很可能url是uri方法的第二个参数，而不是functionUrn，因为functionUrn参与了url的初始化。

参数在构造函数未使用

V6022 参数returnType在构造函数未使用。 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;
  }
  ....
}
复制代码

在构造函数中，程序员忘记使用returnType参数，并给returnType字段赋值。这就是为什么调用getReturnType默认返回null。

相同的函数

V6032 很奇怪enable与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);
  }
  ....
}
复制代码

有两个完全一样的方法并不是一个错误，但是两个方法起一样的作用至少看起来会很奇怪。看一看上面方法的名字，它们应该是执行相反的操作，但事实上它们却完全相同，并返回ServiceAction对象，很可能disable函数是复制的enable的代码，方法体忘记修改了。

忘记重要的检查

V6060 params在验证是否为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));
}
复制代码

params在if里被检查是否为null，但是在检查之前，就已经调用了它的get方法，两次，如果它是null的话，就会抛出异常。

还有几处类似的错误：