Huawei Cloud: в PVS-Studio сегодня облачно

В нынешний век каждый уже слышал об облачных сервисах. Многие компании освоили этот сегмент рынка и создали свои облачные сервисы самых различных направлений. Наша команда также в последнее время интересуется этими сервисами с точки зрения интеграции с ними анализатора кода PVS-Studio. Думаем, наши постоянные читатели уже догадываются, проект какого типа мы проверим в этот раз. Выбор пал на код облачных сервисов компании Huawei.

Введение

Если вы следите за командой PVS-Studio, то, наверняка, заметили, что в последнее время нас сильно заинтересовали облачные технологии. Мы уже опубликовали несколько статей, связанных с данной темой:

• PVS-Studio идёт в облака: Azure DevOps
• PVS-Studio идёт в облака: Travis CI
• PVS-Studio идёт в облака: CircleCI
• PVS-Studio идёт в облака: GitLab CI/CD

И вот когда я подбирал очередной интересный проект для грядущей статьи, мне на почту пришло предложение о работе от компании Huawei. При сборе информации о данной компании выяснилось, что у них есть свои облачные сервисы, но главное то, что исходный код этих сервисов лежит в свободном доступе на GitHub. Именно это стало главной причиной выбора этой компании для данной статьи. Как говорил один китайский мудрец: «Случайности не случайны».

Теперь немного о нашем анализаторе. PVS-Studio — это статический анализатор кода, который выявляет ошибки и уязвимости в исходном коде программ написанных на C, C++, C# и Java. Анализатор работает на платформах Windows, Linux и macOS. Помимо плагинов к таким классическим средам разработки, как Visual Studio или IntelliJ IDEA, анализатор имеет возможность интегрироваться с SonarQube и Jenkins:

• Интеграция результатов анализа PVS-Studio в SonarQube
• Запуск PVS-Studio в Jenkins

Анализ проекта

В процессе поиска информации для статьи я выяснил, что у Huawei имеется developer center, где можно получить информацию, руководства и исходники их облачных сервисов. Для создания этих сервисов использовались разнообразные языки программирования, но самыми популярными оказались такие языки, как Go, Java и Python.

Так как я специализируюсь на Java, то и проекты были выбраны соответствующие. Исходники проектов, анализируемых в статье, можно получить в GitHub репозитории huaweicloud.

Для анализа проектов мне потребовалось совершить лишь несколько действий:

• Получить проекты из репозитория;
• Воспользоваться инструкцией по запуску Java-анализатора и запустить анализ на каждом из проектов.

Проанализировав проекты, получилось выделить лишь три, которым хотелось бы уделить внимание. Это является следствием того, что размер остальных Java проектов оказался слишком мал.

Результаты анализа проектов (количество предупреждений и количество файлов):

• huaweicloud-sdk-java: 31 — High, 2 — Medium и 16 — Low, 2700+ файлов.
• huaweicloud-dis-agent: 7 — High, 6 — Medium и 6 — Low, 100+ файлов.
• huaweicloud-sdk-java-dis: 15 — High, 6 — Medium и 16 — Low, 270+ файлов.

Предупреждений немного, так что в целом можно сказать о хорошем качестве кода. Тем более, что не все срабатывания являются действительными ошибками. Связанно это с тем, что анализатору иногда не хватает информации, чтобы отличить корректный код от ошибочного. Поэтому диагностики анализатора совершенствуются каждый день при помощи информации, полученной от пользователей. См. также статью "Как и почему статические анализаторы борются с ложными срабатываниями".

В процессе анализа проектов я отобрал самые интересные предупреждения, о которых и расскажу в данной статье.

Порядок инициализации полей

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);           // <=
    }
  }
}

При возникновении любого исключения в конструкторе класса UntrustedSSL, информация об этом исключении логируется в блоке catch при помощи логгера LOG. Однако из-за порядка инициализации статических полей, LOG в момент инициализации поля INSTANCE еще не инициализирован. Поэтому при логировании информации об исключении в конструкторе произойдет исключение NullPointerException. Это исключение является причиной еще одного исключения ExceptionInInitializerError, которое выбрасывается, если произошло исключение при инициализации статического поля. Все что необходимо для решения описанной проблемы — поместить инициализацию LOG перед инициализацией INSTANCE.

Незаметная опечатка

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

Оба метода устанавливают поле metricSchema, но названия методов различаются на один символ 's'. Программист именовал аргументы этих методов в соответствии с названием метода. В результате в строке, на которую указывает анализатор, поле metricSchema присваивается самому себе, а аргумент метода metricsSchema не используется.

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

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

Здесь представлена банальная ошибка, связанная с невнимательностью, из-за которой происходит присваивание аргумента suspend самому себе. В результате полю suspend не будет присвоено значение пришедшего аргумента, как подразумевалось. Правильно:

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

Предопределенность условий

Как это часто бывает, правило V6007 вырывается вперед по количеству предупреждений.

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

В этом методе аргументы проверяются на равенство null методом checkNotNull:

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

После проверки аргумента методом checkNotNull, на 100% можно быть уверенным в том, что аргумент, переданный в этот метод, не равен null. Так как оба аргумента метода removeFirewallRuleFromPolicy проверяются методом checkNotNull, то не имеет смысла в дальнейшем проверять аргументы на равенство null еще раз. Однако в метод checkState в качестве первого аргумента передается выражение, где аргументы firewallPolicyId и firewallRuleId повторно проверяются на равенство null.

Аналогичное предупреждение выдается и для firewallRuleId:

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

В данном методе, если аргумент filteringParams равняется null, то происходит выход из метода с возвращением значения. Поэтому проверка, на которую указывает анализатор, всегда будет истинной, а значит эта проверка не имеет смысла.

Подобное встречается еще в 13 классах:

• 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)
• и так далее...

Нулевая ссылка

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

В данном методе инициализация ссылочного поля m.blockDeviceMapping не произойдет, если аргумент blockDevice будет равен null. Это поле инициализируется только в данном методе, поэтому при вызове метода add у поля m.blockDeviceMapping произойдет исключение 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), ....);
}

В качестве третьего аргумента конструктору класса TrackedFile передается результат статического метода FileId.get(path). Но этот метод может вернуть null:

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

В конструкторе, вызываемом через this, аргумент id не изменяется до его первого использования:

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

Следовательно, если в метод в качестве третьего аргумента будет передан null, то произойдет исключение.

Подобная ситуация происходит еще в одном случае:

• 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. Ряд проверок в условном операторе вполне допускает нулевой объект dataTmpFile для дальнейшего разыменования. Я думаю, здесь две опечатки, и проверка, на самом деле, должна быть такой:

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("/"));
}

Подразумевается, что в данный метод передается URL в виде строки, которая никак не валидируется. После эту строку обрезают несколько раз при помощи метода lastIndexOf. Если метод lastIndexOf не найдет совпадения в строке, то вернет -1. Это приведет к исключению StringIndexOutOfBoundsException, так как аргументы метода substring должны быть неотрицательными числами. Для корректной работы метода необходимо добавить валидацию входного аргумента или проверить, что результаты методов lastIndexOf не являются отрицательными числами.

Вот еще места, где встречается подобное:

• 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)

Забытый результат

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

При написании этого кода не было учтено, что вызов метода concat не изменит строку host ввиду неизменяемости объектов типа String. Для корректной работы метода необходимо присвоить результат метода concat переменной host в блоке if. Правильно:

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

В данном методе переменная url не используется после инициализации. Скорее всего, в метод uri вторым аргументом должна была передаваться переменная url вместо functionUrn, так как переменная functionUrn участвует в инициализации переменной url.

Аргумент, не используемый в конструкторе

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

В этом конструкторе забыли использовать аргумент returnType и присвоить его значение полю returnType. Поэтому при вызове метода getReturnType у объекта, созданного данным конструктором, будет возвращено значение по умолчанию – null, хотя, скорее всего, подразумевалось получить объект, ранее переданный в конструктор.

Одинаковая функциональность

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

Наличие двух одинаковых методов не является ошибкой, но то, что два метода выполняют одно и то же действие – это, как минимум, странно. Взглянув на названия вышеуказанных методов можно предположить, что они выполняют противоположные действия. На самом деле, оба метода выполняют одно и то же – создают и возвращают объект ServiceAction. Скорее всего, метод disable создали копированием кода метода enable, но при этом забыли изменить тело метода.

Забыли проверить главное

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

В этом методе решили проверить содержимое структуры типа Map на неравенство null. Для этого у аргумента params два раза вызывают метод get, результат которого передается в метод checkNotNull. Все кажется логичным, но как бы не так! В if происходит проверка аргумента params на неравенство null. После чего можно предположить, что входной аргумент может быть равен null, но до этой проверки у params уже два раза был вызван метод get. Если в качестве аргумента в данный метод будет передан null, то при первом же вызове метода get будет выброшено исключение.

Аналогичная ситуация встречается еще в трех местах:

• 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)

Заключение

Современные крупные компании в наше время просто не могут обойтись без использования облачных сервисов. Этими сервисами пользуется огромное количество человек, поэтому даже малейшая ошибка в сервисе может привести к проблемам у многих людей, а также к дополнительным расходам компании для исправления последствий этой ошибки. При разработке всегда надо учитывать человеческий фактор, так как любой человек рано или поздно совершает ошибки, и данная статья тому пример. Именно поэтому следует использовать все возможные инструменты для повышения качества кода.

PVS-Studio обязательно сообщит компании Huawei о результатах проверки их облачных сервисов, чтобы разработчики этой компании могли изучить их более подробно.

Разовое использование статического анализа кода, продемонстрированное в статье, не может показать все его преимущества. Более подробную информацию о том, как правильно использовать статический анализ, можно посмотреть здесь и здесь. Скачать анализатор PVS-Studio можно тут.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valery Komarov. Huawei Cloud: It's Cloudy in PVS-Studio Today.