В настоящее время все знают об облачных сервисах. Многие компании взломали этот сегмент рынка и создали собственные облачные сервисы различного назначения. В последнее время наша команда также заинтересовалась этими сервисами в плане интеграции в них анализатора кода 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:
Анализ проекта
Когда я проводил исследование для статьи, я обнаружил, что у Huawei есть центр разработчиков с доступной информацией, руководствами и источниками их облачных сервисов. Для создания этих сервисов использовались самые разные языки программирования, но наиболее распространенными были такие языки, как Go, Java и Python.
Поскольку я специализируюсь на Java, проекты подбирались в соответствии с моими знаниями и навыками. Получить проанализированные в статье исходники проекта можно в репозитории GitHub huaweicloud.
Для анализа проектов мне нужно было сделать всего несколько вещей:
- Получить проекты из репозитория;
- Воспользуйтесь инструкцией запуска 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 Переменная приостановить присваивается самой себе. 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 Выражение firewallPolicyId == null всегда ложно. 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 не имеет смысла. Однако выражение, в котором аргументы firewallPolicyId и firewallRuleId повторно проверяются на null, передается в качестве первого аргумента в checkState метод.
Аналогичное предупреждение выдается и для firewallRuleId:
- V6007 Выражение «firewallRuleId == null» всегда ложно. FirewallPolicyServiceImpl.java(125)
V6007 Выражение filteringParams != null всегда верно. 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 Выражение ‘filteringParams != null’ всегда истинно. PolicyRuleServiceImpl.java(58)
- V6007 Выражение ‘filteringParams != null’ всегда истинно. GroupServiceImpl.java(58)
- V6007 Выражение ‘filteringParams != null’ всегда истинно. ExternalSegmentServiceImpl.java(57)
- V6007 Выражение ‘filteringParams != null’ всегда истинно. L3policyServiceImpl.java(57)
- V6007 Выражение ‘filteringParams != null’ всегда истинно. 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;
}В этом методе инициализация поля ссылки m.blockDeviceMapping не произойдет, если аргумент blockDevice равен null. Это поле инициализируется только в этом методе, поэтому при вызове метода add из поля m.blockDeviceMapping произойдет NullPointerException.
V6008 Возможное нулевое разыменование FileId.get(path) в функции ‹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 Потенциальное нулевое разыменование «буфера». ПубликацияQueue.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 ;
}
}
....
}снова НПЭ. Ряд проверок в условном операторе разрешает нулевой объект dataTmpFile для дальнейшего разыменования. Я думаю, что здесь две опечатки, и чек должен выглядеть так:
if (dataTmpFile != null && !dataTmpFile.exists())Подстроки и отрицательные числа
V6009 Функция подстрока может получить значение -1, в то время как ожидается неотрицательное значение. Проверить аргумент: 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 Функция «подстрока» может получить значение «-1», в то время как ожидается неотрицательное значение. Проверить аргумент: 2. RemoveProjectIdFromURL.java(37)
- V6009 Функция «подстрока» может получить значение «-1», в то время как ожидается неотрицательное значение. Проверить аргумент: 2. RemoveVersionProjectIdFromURL.java(38)
Забытый результат
V6010 Необходимо использовать возвращаемое значение функции concat. АКСК.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 Переменная 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 Ссылка на параметры использовалась до того, как она была проверена на соответствие нулю. 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. Для этого дважды вызывается метод get из аргумента params . Результат метода get передается методу checkNotNull. Вроде бы все логично, но это не так! Аргумент params проверяется на null в if. После этого ожидается, что входной аргумент может быть null, но перед этой проверкой метод get уже дважды вызывался из params. Если null передается в качестве аргумента этому методу, при первом вызове метода get будет создано исключение.
Аналогичная ситуация происходит еще в трех местах:
- V6060 Ссылка на «параметры» использовалась до того, как она была проверена на нуль. DomainService.java(389), DomainService.java(387)
- V6060 Ссылка на «параметры» использовалась до того, как она была проверена на нуль. DomainService.java(372), DomainService.java(369)
- V6060 Ссылка на «параметры» использовалась до того, как она была проверена на нуль. DomainService.java(353), DomainService.java(350)
Вывод
Современные крупные компании не могут обойтись без использования облачных сервисов. Этими услугами пользуется огромное количество людей. С этой точки зрения, даже небольшая ошибка в сервисе может привести к проблемам для многих людей, а также к дополнительным убыткам, которые несет компания для устранения неблагоприятных последствий этой ошибки. Человеческие недостатки всегда следует учитывать, тем более, что рано или поздно все совершают ошибки, как описано в этой статье. Этот факт обосновывает использование всех возможных инструментов для улучшения качества кода.
PVS-Studio обязательно проинформирует компанию Huawei о результатах проверки их облачных сервисов, чтобы разработчики Huawei могли остановиться на них, потому что разовое использование статического анализа кода, описанного в этих статьях (1, 2), может не в полной мере продемонстрировать все свои преимущества. Скачать анализатор PVS-Studio можно здесь.
