Анализ кода CUBA Platform с помощью PVS-Studio

Для Java программистов существуют полезные инструменты, помогающие писать качественный код, например, мощная среда разработки IntelliJ IDEA, бесплатные анализаторы SpotBugs, PMD и другие. Всё это уже используется в разработке проекта CUBA Platform, и в этом обзоре найденных дефектов кода я расскажу, как ещё можно улучшить качество проекта, используя статический анализатор кода PVS-Studio.

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

CUBA Platform — это высокоуровневый Java-фреймворк для быстрого создания корпоративных приложений с полноценным веб-интерфейсом. Платформа абстрагирует разработчика от разнородных технологий, чтобы вы могли сфокусироваться на решении задач бизнеса, в то же время, не лишая возможности работать с ними напрямую. Исходный код взят из репозитория на GitHub.

PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках C, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS. Для удобства Java-программистов мы разработали плагины для Maven, Gradle и IntelliJ IDEA. Проект CUBA Platform легко проверился с помощью плагина для Gradle.

Ошибки в условиях

Предупреждение 1

V6007 Expression 'StringUtils.isNotEmpty(«handleTabKey»)' is always true. SourceCodeEditorLoader.java(60)

@Override
public void loadComponent() {
  ....
  String handleTabKey = element.attributeValue("handleTabKey");
  if (StringUtils.isNotEmpty("handleTabKey")) {
    resultComponent.setHandleTabKey(Boolean.parseBoolean(handleTabKey));
  }
  ....
}

После извлечения значения атрибута из некого элемента не выполняется проверка этого значения. Вместо этого в функцию isNotEmpty передаётся константная строка, а надо было передать переменную handleTabKey.

Есть ещё одна аналогичная ошибка в файле AbstractTableLoader.java:

• V6007 Expression 'StringUtils.isNotEmpty(«editable»)' is always true. AbstractTableLoader.java(596)

Предупреждение 2

V6007 Expression 'previousMenuItemFlatIndex >= 0' is always true. CubaSideMenuWidget.java(328)

protected MenuItemWidget findNextMenuItem(MenuItemWidget currentItem) {
  List<MenuTreeNode> menuTree = buildVisibleTree(this);
  List<MenuItemWidget> menuItemWidgets = menuTreeToList(menuTree);

  int menuItemFlatIndex = menuItemWidgets.indexOf(currentItem);
  int previousMenuItemFlatIndex = menuItemFlatIndex + 1;
  if (previousMenuItemFlatIndex >= 0) {
      return menuItemWidgets.get(previousMenuItemFlatIndex);
  }
  return null;
}

Функция indexOf может вернуть значение -1, если в списке не будет найден элемент. Затем к индексу прибавляется единица, скрывая таким образом ситуацию, когда нужный элемент отсутствует. Другой потенциальной проблемой может стать тот факт, что переменная previousMenuItemFlatIndex будет всегда больше или равна нулю. Если, например, список menuItemWidgets будет пустым, то становится возможным выход за границу массива.

Предупреждение 3

V6009 The 'delete' function could receive the '-1' value while non-negative value is expected. Inspect argument: 1. AbstractCollectionDatasource.java(556)

protected DataLoadContextQuery createDataQuery(....) {
  ....
  StringBuilder orderBy = new StringBuilder();
  ....
  if (orderBy.length() > 0) {
      orderBy.delete(orderBy.length() - 2, orderBy.length());
      orderBy.insert(0, " order by ");
  }
  ....
}

В буфере символов orderBy удаляют последние 2 символа, если их общее количество больше нуля, т.е. строка содержит один символ или больше. Но стартовую позицию удаления символов задали со смещением на 2 символа. Таким образом, если вдруг orderBy будет состоять из 1 символа, попытка удаления приведёт к исключению StringIndexOutOfBoundsException.

Предупреждение 4

V6013 Objects 'masterCollection' and 'entities' are compared by reference. Possibly an equality comparison was intended. CollectionPropertyContainerImpl.java(81)

@Override
public void setItems(@Nullable Collection<E> entities) {
  super.setItems(entities);
  Entity masterItem = master.getItemOrNull();
  if (masterItem != null) {
    MetaProperty masterProperty = getMasterProperty();
    Collection masterCollection = masterItem.getValue(masterProperty.getName());
    if (masterCollection != entities) {
      updateMasterCollection(masterProperty, masterCollection, entities);
    }
  }
}

В функции updateMasterCollection значения из entities копируются в masterCollection. Перед этим коллекции сравнили по ссылке, но, возможно, их планировали сравнивать по значению.

Предупреждение 5

V6013 Objects 'value' and 'oldValue' are compared by reference. Possibly an equality comparison was intended. WebOptionsList.java(278)

protected boolean isCollectionValuesChanged(Collection<I> value,
                                            Collection<I> oldValue) {
  return value != oldValue;
}

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

Избыточные условия

Предупреждение 1

V6007 Expression 'mask.charAt(i + offset) != placeHolder' is always true. DatePickerDocument.java(238)

private String calculateFormattedString(int offset, String text) .... {
  ....
  if ((mask.charAt(i + offset) == placeHolder)) {         // <=
    ....
  } else if ((mask.charAt(i + offset) != placeHolder) &&  // <=
             (Character.isDigit(text.charAt(i)))) {
    ....
  }
  ....
}

Во втором сравнении проверяется выражение, которое противоположно первому. Вторую проверку можно удалить, что упростит код.

V6007 Expression 'connector == null' is always false. HTML5Support.java(169)

private boolean validate(NativeEvent event) {
  ....
  while (connector == null) {
    widget = widget.getParent();
    connector = Util.findConnectorFor(widget);
  }

  if (this.connector == connector) {
      return true;
  } else if (connector == null) {             // <=
      return false;
  } else if (connector.getWidget() instanceof VDDHasDropHandler) {
      return false;
  }
  return true;
}

После завершения цикла while, значение переменной connector не будет равняться null, следовательно, избыточную проверку можно удалить.

Ещё одно подозрительное место, на которое стоит обратить внимание разработчикам:

• V6007 Expression 'StringUtils.isBlank(strValue)' is always true. Param.java(818)

Недостижимый код в тестах

V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(283)

private void throwException() {
  throw new RuntimeException(TEST_EXCEPTION_MSG);
}

@Test
public void testSuspendRollback() {
  Transaction tx = cont.persistence().createTransaction();
  try {
    ....
    Transaction tx1 = cont.persistence().createTransaction();
    try {
      EntityManager em1 = cont.persistence().getEntityManager();
      assertTrue(em != em1);
      Server server1 = em1.find(Server.class, server.getId());
      assertNull(server1);
      throwException();        // <=
      tx1.commit();            // <=
    } catch (Exception e) {
      //
    } finally {
      tx1.end();
    }

    tx.commit();
  } finally {
    tx.end();
  }
}

Функция throwException бросает исключение, из-за которого не выполняется вызов функции tx1.commit. Возможно, эти строки стоит поменять местами.

Ещё несколько похожих мест в других тестах:

• V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(218)
• V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(163)
• V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(203)
• V6019 Unreachable code detected. It is possible that an error is present. TransactionTest.java(137)
• V6019 Unreachable code detected. It is possible that an error is present. UpdateDetachedTest.java(153)
• V6019 Unreachable code detected. It is possible that an error is present. EclipseLinkDetachedTest.java(132)
• V6019 Unreachable code detected. It is possible that an error is present. PersistenceTest.java(223)

Подозрительные аргументы функций

Предупреждение 1

V6023 Parameter 'salt' is always rewritten in method body before being used. BCryptEncryptionModule.java(47)

@Override
public String getHash(String content, String salt) {
  salt = BCrypt.gensalt();
  return BCrypt.hashpw(content, salt);
}

В криптографии salt — строка данных, которая передаётся хеш-функции вместе с паролем. Главным образом используется для защиты от перебора по словарю и атак с использованием радужных таблиц, а также сокрытия одинаковых паролей. Подробнее: Соль (криптография).

В этой функции строка перетирается сразу при входе в функцию. Возможно, игнорирование переданного значения является потенциальной уязвимостью.

Предупреждение 2

Для рассматриваемой функции анализатор выдаёт сразу два предупреждения:

• V6023 Parameter 'offsetWidth' is always rewritten in method body before being used. CubaSuggestionFieldWidget.java(433)
• V6023 Parameter 'offsetHeight' is always rewritten in method body before being used. CubaSuggestionFieldWidget.java(433)

@Override
public void setPosition(int offsetWidth, int offsetHeight) {
  offsetHeight = getOffsetHeight();
  ....
  if (offsetHeight + getPopupTop() > ....)) {
    ....
  }
  ....
  offsetWidth = containerFirstChild.getOffsetWidth();
  if (offsetWidth + getPopupLeft() > ....)) {
      ....
  } else {
      left = getPopupLeft();
  }
  setPopupPosition(left, top);
}

Занятный код. Функция принимает всего две переменных: offsetWidth и offsetHeight, обе перезаписываются перед использованием.

Предупреждение 3

V6022 Parameter 'shortcut' is not used inside constructor body. DeclarativeTrackingAction.java(47)

public DeclarativeTrackingAction(String id, String caption, String description,
                                 String icon, String enable, String visible,
                                 String methodName, @Nullable String shortcut,
                                 ActionsHolder holder) {
  super(id);
  this.caption = caption;
  this.description = description;
  this.icon = icon;

  setEnabled(enable == null || Boolean.parseBoolean(enable));
  setVisible(visible == null || Boolean.parseBoolean(visible));

  this.methodName = methodName;
  checkActionsHolder(holder);
}

Значение параметра shortcut не используется в функции. Возможно, интерфейс функции устарел или это предупреждение не является ошибкой.

Ещё несколько подобных мест:

• V6022 Parameter 'type' is not used inside constructor body. QueryNode.java(36)
• V6022 Parameter 'text2' is not used inside constructor body. MarkerAddition.java(22)
• V6022 Parameter 'selection' is not used inside constructor body. AceEditor.java(114)
• V6022 Parameter 'options' is not used inside constructor body. EntitySerialization.java(379)

Разные функции с одинаковым кодом

Предупреждение 1

V6032 It is odd that the body of method 'firstItemId' is fully equivalent to the body of another method 'lastItemId'. ContainerTableItems.java(213), ContainerTableItems.java(219)

@Override
public Object firstItemId() {
  List<E> items = container.getItems();
  return items.isEmpty() ? null : items.get(0).getId();
}

@Override
public Object lastItemId() {
  List<E> items = container.getItems();
  return items.isEmpty() ? null : items.get(0).getId();
}

Функции firstItemId и lastItemId имеют одинаковую реализацию. Скорее всего, в последней необходимо было получать элемент не с индексом 0, а вычислять индекс последнего элемента.

Предупреждение 2

V6032 It is odd that the body of method is fully equivalent to the body of another method. SearchComboBoxPainter.java(495), SearchComboBoxPainter.java(501)

private void paintBackgroundDisabledAndEditable(Graphics2D g) {
  rect = decodeRect1();
  g.setPaint(color53);
  g.fill(rect);
}

private void paintBackgroundEnabledAndEditable(Graphics2D g) {
  rect = decodeRect1();
  g.setPaint(color53);
  g.fill(rect);
}

Ещё две функции с подозрительно одинаковой реализацией. Рискну предположить, что в одной из них нужно было использовать другой цвет, отличный от color53.

Обращение по нулевой ссылке

Предупреждение 1

V6060 The 'descriptionPopup' reference was utilized before it was verified against null. SuggestPopup.java(252), SuggestPopup.java(251)

protected void updateDescriptionPopupPosition() {
  int x = getAbsoluteLeft() + WIDTH;
  int y = getAbsoluteTop();
  descriptionPopup.setPopupPosition(x, y);
  if (descriptionPopup!=null) {
    descriptionPopup.setPopupPosition(x, y);
  }
}

Всего в двух строчках автору удалось написать очень подозрительный код. Сначала у объекта descriptionPopup вызывается метод setPopupPosition, а потом объект сравнивается с null. Скорее всего, первый вызов функции setPopupPosition является лишним и опасным. Похоже на последствия неудачного рефакторинга.

Предупреждения 2

V6060 The 'tableModel' reference was utilized before it was verified against null. DesktopAbstractTable.java(1580), DesktopAbstractTable.java(1564)

protected Column addRuntimeGeneratedColumn(String columnId) {
  // store old cell editors / renderers
  TableCellEditor[] cellEditors =
    new TableCellEditor[tableModel.getColumnCount() + 1];         // <=
  TableCellRenderer[] cellRenderers =
    new TableCellRenderer[tableModel.getColumnCount() + 1];       // <=

  for (int i = 0; i < tableModel.getColumnCount(); i++) {         // <=
      Column tableModelColumn = tableModel.getColumn(i);

      if (tableModel.isGeneratedColumn(tableModelColumn)) {       // <=
          TableColumn tableColumn = getColumn(tableModelColumn);
          cellEditors[i] = tableColumn.getCellEditor();
          cellRenderers[i] = tableColumn.getCellRenderer();
      }
  }

  Column col = new Column(columnId, columnId);
  col.setEditable(false);

  columns.put(col.getId(), col);

  if (tableModel != null) {                                       // <=
      tableModel.addColumn(col);
  }
  ....
}

Похожая ситуация и в этой функции. После многочисленных обращений к объекту tableModel выполняется проверка, равен он null или нет.

Ещё один пример:

• V6060 The 'tableModel' reference was utilized before it was verified against null. DesktopAbstractTable.java(596), DesktopAbstractTable.java(579)

Возможно, логическая ошибка

V6026 This value is already assigned to the 'sortAscending' variable. CubaScrollTableWidget.java(488)

@Override
protected void sortColumn() {
  ....
  if (sortAscending) {
    if (sortClickCounter < 2) {
      // special case for initial revert sorting instead of reset sort order
      if (sortClickCounter == 0) {
        client.updateVariable(paintableId, "sortascending", false, false);
      } else {
        reloadDataFromServer = false;
        sortClickCounter = 0;
        sortColumn = null;
        sortAscending = true;   // <=

        client.updateVariable(paintableId, "resetsortorder", "", true);
      }
    } else {
      client.updateVariable(paintableId, "sortascending", false, false);
    }
  } else {
    if (sortClickCounter < 2) {
      // special case for initial revert sorting instead of reset sort order
      if (sortClickCounter == 0) {
        client.updateVariable(paintableId, "sortascending", true, false);
      } else {
        reloadDataFromServer = false;
        sortClickCounter = 0;
        sortColumn = null;
        sortAscending = true;

        client.updateVariable(paintableId, "resetsortorder", "", true);
      }
    } else {
      reloadDataFromServer = false;
      sortClickCounter = 0;
      sortColumn = null;
      sortAscending = true;

      client.updateVariable(paintableId, "resetsortorder", "", true);
    }
  }
  ....
}

В первом условии переменная sortAscending и так равна true, но ей всё равно присваивают то же самое значение. Возможно, это является ошибкой и хотели присвоить false.

Похожий пример из другого файла:

• V6026 This value is already assigned to the 'sortAscending' variable. CubaTreeTableWidget.java(444)

Странные возвращаемые значения функций

Предупреждение 1

V6037 An unconditional 'return' within a loop. QueryCacheManager.java(128)

public <T> T getSingleResultFromCache(QueryKey queryKey, List<View> views) {
  ....
  for (Object id : queryResult.getResult()) {
    return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....));
  }
  ....
}

Анализатор обнаружил безусловный вызов оператора return на первой же итерации цикла for. Возможно, тут ошибка, или нужно переписать код на использование оператора if.

Предупреждение 2

V6014 It's odd that this method always returns one and the same value. DefaultExceptionHandler.java(40)

@Override
public boolean handle(ErrorEvent event, App app) {
  Throwable t = event.getThrowable();
  if (t instanceof SocketException
      || ExceptionUtils.getRootCause(t) instanceof SocketException) {
    return true;
  }
  if (ExceptionUtils.getThrowableList(t).stream()
      .anyMatch(o -> o.getClass().getName().equals("...."))) {
    return true;
  }
  if (StringUtils.contains(ExceptionUtils.getMessage(t), "....")) {
    return true;
  }
  AppUI ui = AppUI.getCurrent();
  if (ui == null) {
    return true;
  }
  if (t != null) {
    if (app.getConnection().getSession() != null) {
      showDialog(app, t);
    } else {
      showNotification(app, t);
    }
  }
  return true;
}

Эта функция во всех случаях возвращает значение true. Но вот в самой последней строчке напрашивается возврат значения false. Возможно, тут допущена ошибка.

Весь список подозрительных функций с похожим кодом:

• V6014 It's odd that this method always returns one and the same value. ErrorNodesFinder.java(31)
• V6014 It's odd that this method always returns one and the same value. FileDownloadController.java(69)
• V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(73)
• V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(48)
• V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(67)
• V6014 It's odd that this method always returns one and the same value. IdVarSelector.java(46)
• V6014 It's odd that this method always returns one and the same value. JoinVariableNode.java(57)

Предупреждение 3

V6007 Expression 'needReload' is always false. WebAbstractTable.java(2702)

protected boolean handleSpecificVariables(Map<String, Object> variables) {
  boolean needReload = false;

  if (isUsePresentations() && presentations != null) {
    Presentations p = getPresentations();

    if (p.getCurrent() != null && p.isAutoSave(p.getCurrent())
        && needUpdatePresentation(variables)) {
      Element e = p.getSettings(p.getCurrent());
      saveSettings(e);
      p.setSettings(p.getCurrent(), e);
    }
  }
  return needReload;
}

Функция возвращает переменную needReload, значение которой всегда равно false. Скорее всего, в одном из условий забыли добавить код изменения значения переменной.

Предупреждение 4

V6062 Possible infinite recursion inside the 'isFocused' method. GwtAceEditor.java(189), GwtAceEditor.java(190)

public final native void focus() /*-{
  this.focus();
}-*/;

public final boolean isFocused() {
  return this.isFocused();
}

Анализатор обнаружил функцию, которая вызывается рекурсивно без условия остановки рекурсии. В этом файле много функций, которые помечены ключевым словом native и содержат закомментированный код. Скорее всего, файл в данный момент переписывается и вскоре разработчики обратят внимание и на функцию isFocused.

Разные предупреждения

Предупреждение 1

V6002 The switch statement does not cover all values of the 'Operation' enum: ADD. DesktopAbstractTable.java(665)

/**
 * Operation which caused the datasource change.
 */
enum Operation {
    REFRESH,
    CLEAR,
    ADD,
    REMOVE,
    UPDATE
}

@Override
public void setDatasource(final CollectionDatasource datasource) {
  ....
  collectionChangeListener = e -> {
    switch (e.getOperation()) {
      case CLEAR:
      case REFRESH:
        fieldDatasources.clear();
        break;

      case UPDATE:
      case REMOVE:
        for (Object entity : e.getItems()) {
          fieldDatasources.remove(entity);
        }
        break;
    }
  };
  ....
}

В операторе switch не рассмотрено значение ADD. Оно является единственным нерассмотренным, поэтому стоит проверить, ошибка это или нет.

Предупреждение 2

V6021 Variable 'source' is not used. DefaultHorizontalLayoutDropHandler.java(177)

@Override
protected void handleHTML5Drop(DragAndDropEvent event) {
  LayoutBoundTransferable transferable = (LayoutBoundTransferable) event
          .getTransferable();
  HorizontalLayoutTargetDetails details = (HorizontalLayoutTargetDetails) event
          .getTargetDetails();
  AbstractOrderedLayout layout = (AbstractOrderedLayout) details
          .getTarget();
  Component source = event.getTransferable().getSourceComponent(); // <=
  int idx = (details).getOverIndex();

  HorizontalDropLocation loc = (details).getDropLocation();
  if (loc == HorizontalDropLocation.CENTER
          || loc == HorizontalDropLocation.RIGHT) {
      idx++;
  }
  Component comp = resolveComponentFromHTML5Drop(event);
  if (idx >= 0) {
    layout.addComponent(comp, idx);
  } else {
    layout.addComponent(comp);
  }
  if (dropAlignment != null) {
    layout.setComponentAlignment(comp, dropAlignment);
  }
}

В коде объявляется и не используется переменная source. Возможно, как и другую переменную comp этого же типа, source забыли добавить в layout.

Ещё функции с неиспользуемыми переменными:

• V6021 Variable 'source' is not used. DefaultHorizontalLayoutDropHandler.java(175)
• V6021 The value is assigned to the 'r' variable but is not used. ExcelExporter.java(262)
• V6021 Variable 'over' is not used. DefaultCssLayoutDropHandler.java(49)
• V6021 Variable 'transferable' is not used. DefaultHorizontalLayoutDropHandler.java(171)
• V6021 Variable 'transferable' is not used. DefaultHorizontalLayoutDropHandler.java(169)
• V6021 Variable 'beanLocator' is not used. ScreenEventMixin.java(28)

Предупреждение 3

V6054 Classes should not be compared by their name. MessageTools.java(283)

public boolean hasPropertyCaption(MetaProperty property) {
  Class<?> declaringClass = property.getDeclaringClass();
  if (declaringClass == null)
    return false;

  String caption = getPropertyCaption(property);
  int i = caption.indexOf('.');
  if (i > 0 && declaringClass.getSimpleName().equals(caption.substring(0, i)))
    return false;
  else
    return true;
}

Анализатор обнаружил ситуацию, когда сравнение классов осуществляется по имени. Такое сравнение является некорректным, т.к., согласно спецификации, JVM классы имеют уникальное имя только внутри пакета. Это может стать причиной некорректного сравнения и выполнения не того кода, который планировался.

Отзыв разработчиков CUBA Platform

Безусловно, в любом крупном проекте бывают баги. Именно поэтому мы с радостью согласились на предложение команды PVS-Studio проверить наш проект. В репозиторий CUBA включены форки некоторых сторонних OSS библиотек под лицензией Apache 2 и, кажется, нам нужно уделить этому коду больше внимания, анализатор нашёл довольно много проблем в этих исходниках. Сейчас мы используем SpotBugs в качестве основного анализатора, и он не находит некоторые существенные проблемы, найденные PVS-Studio. Самое время пойти и написать дополнительные проверки самим. Большое спасибо команде PVS-Studio за проделанную работу.

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

Ещё команда PVS-Studio не может пройти мимо фразы «самое время пойти и написать дополнительные проверки самим» и не оставить эту картинку :)

Заключение

PVS-Studio будет отличным дополнением для вашего проекта к уже существующим инструментам улучшения качества кода. Особенно, если в штате десятки, сотни и тысячи сотрудников. PVS-Studio предназначен не только для нахождения ошибок, но и для их исправления. Причём речь не об автоматическом редактировании кода, а о надёжном контроле качества кода. В большой компании невозможно представить ситуацию, когда абсолютно все разработчики будут самостоятельно проверять свой код различными инструментами, поэтому таким компаниям больше подходят инструменты типа PVS-Studio, где предусмотрен контроль качества кода на всех этапах разработки, не только у рядового программиста.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Analyzing the Code of CUBA Platform with PVS-Studio