Если хотите посмотреть, что нашёл статический анализатор PVS-Studio в исходном коде Intellij платформы, используемой OpenIDE, то добро пожаловать в статью.

О чём будет статья?

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

Мы — компания PVS-Studio, разрабатываем свой одноимённый статический анализатор. Если кратко, то это программа, которая ищет ошибки в исходном коде других программ. И в таких статьях-проверках мы берём open source проект и показываем, какие интересные ошибки нашёл там наш анализатор, тем самым знакомим вас с методологией статического анализа и демонстрируем, что умеет находить наш продукт. И если появляется возможность, через призму срабатывания рассказываем о каком-либо интересном аспекте языка, на котором проект написан.

Здесь и сейчас мы рассмотрим, что, по мнению автора, интересного нашёл PVS-Studio при анализе Intellij платформы, которую используют OpenIDE.

К слову, так пользоваться анализатором неправильно

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

Представим OpenIDE

OpenIDE — это бесплатная лицензионно чистая IDE на базе IntelliJ IDEA Community Edition с открытым исходным кодом.

Разработкой OpenIDE занимается группа компаний:

• Axiom JDK: вендоры российской среды разработки и исполнения Java;

• "Группа Астра": разработчики системного и прикладного ПО. Их вы можете знать по отечественной Astra Linux;

• "Хоулмонт": разработчики корпоративного ПО для среднего и крупного сегментов бизнеса. Их вы можете знать по Amplicode — плагину для работы со Spring-проектами.

Исходный код OpenIDE располагается на платформе GitFlic. Клонировали и собрали ветку 252 —можно приступать к анализу. Последний коммит на момент проверки: ea0f8. Что же мы нашли?

Вперёд, к срабатываниям

Обсудим clinit

Здесь мы поговорим про инициализацию статических полей.

Анализатор указывает на вот такой код:

public final class UsageType {
  ....
  public static final UsageType 
    DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED = new UsageType(
      UsageViewBundle.messagePointer(
        "usage.type.delegate.to.another.instance.method.parameters.changed"
      )
    );

  private static final Logger LOG = Logger.getInstance(UsageType.class);

  public UsageType(@NotNull Supplier<....> nameComputable) {
    myNameComputable = nameComputable;
    if (ApplicationManager.getApplication().isUnitTestMode()) {
      String usageTypeString = myNameComputable.get();
      if (usageTypeString.indexOf('{') != -1) {
        LOG.error(....);
      }
    }
  }
  ....
}

Ссылка на файл: UsageType.java

Можете попробовать найти ошибку самостоятельно, а можете сразу перейти к объяснению.

DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED использует для своей инициализации конструктор public UsageType(@NotNull Supplier<....> nameComputable).

В этом конструкторе происходит использование логгера:

LOG.error(....);

Но обращение к логгеру в этот момент приведёт к NPE. Почему? Обратимся к JLS:

1. Пункт 12.3.2 говорит нам о том, что ещё до инициализации класса, на этапе его подготовки, статические поля принимают значения по умолчанию.

2. Пункт 12.4.2 сообщает, что инициализация статических полей происходит в порядке их объявления.

Т.е. на этапе инициализации DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED LOG будет иметь значение null, поскольку его инициализация происходит после:

public static final UsageType 
  DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED = ....;

private static final Logger LOG = Logger.getInstance(UsageType.class);

Исправить проблему с NPE донельзя просто: логгер необходимо инициализировать раньше, чем поле, в котором он используется, т.е. просто разместить поле с ним выше.

Вот что на это говорит анализатор PVS-Studio: V6050 Class initialization cycle is present. Initialization of 'DELEGATE_TO_ANOTHER_INSTANCE_PARAMETERS_CHANGED' appears before the initialization of 'LOG'. UsageType.java 46.

В проекте был ещё один похожий случай, но с небольшим отличием:

public class LanguageIndentStrategy extends LanguageExtension<IndentStrategy> {
  ....
  public static final LanguageIndentStrategy 
    INSTANCE = new LanguageIndentStrategy();

  private static final DefaultIndentStrategy 
    DEFAULT_INDENT_STRATEGY = new DefaultIndentStrategy();

  public LanguageIndentStrategy() {
    super(EP_NAME, DEFAULT_INDENT_STRATEGY);
  }
  ....
}

Ссылка на файл: LanguageIndentStrategy.java

V6050 Class initialization cycle is present. Initialization of 'INSTANCE' appears before the initialization of 'DEFAULT_INDENT_STRATEGY'. LanguageIndentStrategy.java 13

Для инициализации поля INSTANCE используется конструктор LanguageIndentStrategy. В нём уже в родительский конструктор через super передаётся к этому моменту ещё неинициализированное поле DEFAULT_INDENT_STRATEGY.

Посмотрим, что с переданной ссылкой происходит в конструкторе родителя LanguageExtension:

public class LanguageExtension<T> extends .... {
  private final T defaultImplementation;
  ....
  public LanguageExtension(
    @NonNls String epName, 
    @Nullable T defaultImplementation
  ) {
    super(epName);
    this.defaultImplementation = defaultImplementation;
    ....
  }
  ....
}

Её присваивают полю класса. Если это поле разыменуется до того, как в дочернем классе произойдёт инициализация изначального объекта, на который была передана ссылка, произойдёт NPE.

Напрямую это поле в классе нигде не разыменовывается:

protected T getDefaultImplementation() {
  return defaultImplementation;
}
....
protected T findForLanguage(@NotNull Language language) {
  for (Language l = language; l != null; l = l.getBaseLanguage()) {
    List<T> extensions = forKey(l);
    if (!extensions.isEmpty()) {
      return extensions.get(0);
    }
  }
  return defaultImplementation;
}

Вроде как проблемы и не возникнет? Вопрос сложный, особенно если учитывать возможность многопоточного контекста. Если параллельно созданию объекта произойдёт обращение к полю defaultImplementation и его разыменование, NPE всё же быть.

Чтобы не приходилось гадать "будет/не будет" и не просчитывать разные сценарии использования, лучше, как и в самом первом примере, в изначальном классе поменять местами статические поля, чтобы изменился порядок инициализации, и ссылка в родительский класс передавалась на уже инициализированный объект:

public class LanguageIndentStrategy extends LanguageExtension<IndentStrategy> {
  ....
  private static final DefaultIndentStrategy 
    DEFAULT_INDENT_STRATEGY = new DefaultIndentStrategy();
    
  public static final LanguageIndentStrategy 
    INSTANCE = new LanguageIndentStrategy();

  public LanguageIndentStrategy() {
    super(EP_NAME, DEFAULT_INDENT_STRATEGY);
  }
  ....
}

И тогда всё точно будет в порядке :)

Некорректная проверка на equals

Здесь предлагаю перейти сразу к коду:

@Override
public boolean equals(Object obj) {
  if (obj instanceof Filter) {
    ....
    return    myPattern.pattern().equals(filter.myPattern)
           && myPattern.flags() == filter.myPattern.flags();
  }
  return false;
}

Ссылка на файл: Filter.java

Срабатывание PVS-Studio: V6058 The 'equals' function compares objects of incompatible types: String, Pattern. Filter.java 112

К сожалению, ни один Filter не пройдёт equals проверку, поскольку myPattern — это объект типа Pattern, а метод pattern() возвращает его строковое представление.

myPattern.pattern().equals(filter.myPattern)

Т.е. мы методу equals класса String передаём объект типа Pattern. Такая проверка будет всегда возвращать false.

Исправление будет выглядеть просто:

return myPattern.pattern().equals(filter.myPattern.pattern()) && ....;

Про неатомарные модификации

Итак, сначала код:

public final class AppendableStorageBackedByResizableMappedFile<Data> .... {
  ....
  private volatile @Nullable AppendMemoryBuffer appendBuffer;
  ....
  //requires storage.lockWrite()
  private void flushAppendBuffer() throws IOException {
    if (AppendMemoryBuffer.hasChanges(appendBuffer)) {
      int bufferPosition = appendBuffer.getBufferPosition();
      storage.put(
        fileLength, 
        appendBuffer.getAppendBuffer(), 
        0, 
        bufferPosition
      );
      fileLength += bufferPosition;
      appendBuffer = appendBuffer.rewind(fileLength); // <=
    }
  }
  ....
  @Override
  public int append(Data value) throws IOException {
    ....
    if (size > APPEND_BUFFER_SIZE) {
      ....
      if (appendBuffer != null) {
        appendBuffer = appendBuffer.rewind(fileLength); // <=
      }
    }
    ....
  }
  ....
}

Ссылка на файл: AppendableStorageBackedByResizableMappedFile.java

Срабатывания анализатора PVS-Studio:

• V6074 Non-atomic modification of volatile variable. Inspect 'appendBuffer'. AppendableStorageBackedByResizableMappedFile.java 58

• V6074 Non-atomic modification of volatile variable. Inspect 'appendBuffer'. AppendableStorageBackedByResizableMappedFile.java 162

Анализатор указывает на потенциальную проблему многопоточности. К слову, здесь сочту корректным упомянуть статью своего коллеги, в которой эта тема раскрыта шире. Здесь же пройдёмся кратко.

Что делает ключевое слово volatile:

• обеспечивает видимость актуального значения поля для всех потоков, работающих с ним;

• устанавливает отношение happens-before для всех операций, производимых с полем;

• гарантирует атомарность операций записи и чтения переменной.

Однако несмотря на то, что volatile обеспечивает атомарность отдельных операций чтения и записи, это не распространяется на составные операции (к примеру инкремент, декремент или +=). Такие операции состоят из нескольких шагов: чтение > вычисление > запись, и потому не являются атомарными.

Поэтому если два и более потока одновременно производят операцию инкремента над одним volatile-полем, результат может оказаться непредсказуемым. Заметил это не я один. Тимлид, увидев срабатывание, обратил моё внимание на комментарий к этому классу, оставленный одним из коммитеров:

Особо наблюдательные читатели могли заметить комментарий над первым методом в приведённом фрагменте кода:

//requires storage.lockWrite()
private void flushAppendBuffer() throws IOException {

Не только наш анализатор оказался внимательным.

Подобная проблема в проекте не единственная. Аналогичные срабатывания:

• V6074 Non-atomic modification of volatile variable. Inspect 'myAnnotationsMap'. JavaMethod.java 89

• V6074 Non-atomic modification of volatile variable. Inspect 'myFlags'. RedBlackTree.java 308

• V6074 Non-atomic modification of volatile variable. Inspect 'logicalSize'. ResizeableMappedFile.java 128

• V6074 Non-atomic modification of volatile variable. Inspect 'myMoreIndex'. RunAnythingGroup.java 85

• V6074 Non-atomic modification of volatile variable. Inspect 'myTitleIndex'. RunAnythingGroup.java 130

• V6074 Non-atomic modification of volatile variable. Inspect 'myNonCancelableSectionCount'. AbstractProgressIndicatorBase.java 251

• V6074 Non-atomic modification of volatile variable. Inspect 'myNonCancelableSectionCount'. AbstractProgressIndicatorBase.java 256

NPE 1

Снова предлагаю самостоятельно найти ошибку в методе, ответ будет ниже:

public void decorate(
    @NotNull ProjectViewNode node, 
    @NotNull PresentationData data
  ) {
  if (!(node instanceof PsiDirectoryNode psiDirectoryNode)) {
    return;
  }
  PsiDirectory directory = psiDirectoryNode.getValue();
  if (directory == null || !directory.isValid()) {
    return;
  }
  final Project project = directory.getProject();
  if (!isAndroidProject(project)) {
    return;
  }
  // If the build dir is inside the module's content root, 
  // ProjectRootsUtil.isModuleContentRoot will return false. 
  // The reason is that when we set up the project during a sync, 
  // we don't create additional content roots 
  // if the build dir is inside the module.
  final VirtualFile folder = directory.getVirtualFile();
  if (!ProjectRootsUtil.isModuleContentRoot(folder, project)) {
    return;
  }
  Object parentValue = psiDirectoryNode.getParent().getValue();
  if (!(parentValue instanceof Module)) {
    return;
  }
  Module module = ProjectRootManager.getInstance(project)
                                    .getFileIndex()
                                    .getModuleForFile(folder);
  if (module == null && !module.isDisposed()) {
    return;
  }
  GradleAndroidModel gradleModel = GradleAndroidModel.get(module);
  IdeAndroidProject 
      androidProject = gradleModel != null ? gradleModel.getAndroidProject() 
                                                         : null;
  if (androidProject == null) {
    return;
  }
  File buildFolderPath = androidProject.getBuildFolder();
  File folderPath = VfsUtilCore.virtualToIoFile(folder);
  if (FileUtil.filesEqual(buildFolderPath, folderPath)) {
    data.clearText();
    data.addText(folder.getName(), SimpleTextAttributes.REGULAR_ATTRIBUTES);
  }
}

Снова сократим код и оставим только необходимый фрагмент:

public void decorate(....) {
  ....
  Module module = ProjectRootManager.getInstance(project)
                                    .getFileIndex()
                                    .getModuleForFile(folder);
  if (module == null && !module.isDisposed()) {
    return;
  }
  ....
}

Файл: BuildNodeDecorator.java

Обратите внимание на условие. Думаю, про необходимость заменить && на || и говорить не стоит. Разве что упомяну, что с такими вещами мы часто сталкиваемся при проверке open source проектов, и не только в контексте NPE: Ошибка настолько проста, что программисты её не замечают.

Срабатывание PVS-Studio: V6008 Null dereference of 'module'. BuildNodeDecorator.java 78

NPE 2

Довольно часто при анализе проектов мы сталкиваемся со случаями, когда проверки на null происходят несвоевременно. Приведу вам один из таких:

@NotNull
JBIterable<InspectionTreeNode> traverseFrom(....) {
  return JBIterable.generate(
      node, 
      n -> getParent(n)
    ).filter(
        n -> getParent(n) != null
    ).flatMap(n1 -> {
      InspectionTreeNode p = getParent(n1);
      int idx = getIndexOfChild(p, n1);
      if (idx < 0) return JBIterable.empty();
      assert p != null; 
      InspectionTreeNode.Children children = p.myChildren;
      ....
    });
}

Ссылка на файл: InspectionTreeModel.java

Сообщение PVS-Studio: V6060 The 'p' reference was utilized before it was verified against null. InspectionTreeModel.java 91

Мы получаем переменную p. Метод, который её возвращает, можешь вернуть null. Это учитывается перед тем, как происходит обращение к его полю:

assert p != null;
InspectionTreeNode.Children children = p.myChildren;

Но не учитывается ранее:

int idx = getIndexOfChild(p, n1);

И это при условии, что p в getIndexOfChild разыменовывается:

@Override
public int getIndexOfChild(Object object, Object child) {
  return ((InspectionTreeNode)object).getIndex((TreeNode)child);
}

Так что несмотря проверку, от разыменования null мы не спасёмся, если таковой вернётся.

NPE 3

Ну и напоследок также предлагаю вам самостоятельно попробовать найти ошибку:

@NotNull
public CompletableFuture<BufferedImage> renderDrawable(
  @NotNull ResourceValue drawableResourceValue
) {
  ....
  return runAsyncRenderAction(() -> myLayoutLib.renderDrawable(params))
    .thenCompose(result -> {
      if (result != null && result.isSuccess()) {
        Object data = result.getData();
        if (!(data instanceof BufferedImage)) {
          data = null;
        }
        return CompletableFuture.completedFuture((BufferedImage)data);
      }
      else {
        if (result.getStatus() == Result.Status.ERROR_NOT_A_DRAWABLE) {
          LOG.debug(....);
          return CompletableFuture.completedFuture(null);
        }
        Throwable exception = result == null 
                              ? new RuntimeException(
                                   "Rendering failed - null result"
                                ) 
                              : result.getException();
        if (exception == null) {
          String message = result.getErrorMessage();
          exception = new RuntimeException(
            message == null 
              ? "Rendering failed" 
              : "Rendering failed - " + message
          );
        }
        reportException(exception);
        return immediateFailedFuture(exception);
      }
    });
}

Ссылка на файл: RenderTask.java

Теперь оставим только необходимое и подсветим ключевые моменты:

@NotNull
public CompletableFuture<BufferedImage> renderDrawable(
  ....
) {
  ....
  return runAsyncRenderAction(....)
    .thenCompose(result -> {
      if (result != null && result.isSuccess()) {                       // <=
        ....
      }
      else {
        if (result.getStatus() == Result.Status.ERROR_NOT_A_DRAWABLE) { // <=
          ....
        }
        Throwable exception = result == null
                              ? new RuntimeException(
                                    "Rendering failed - null result") 
                              : result.getException();
        if (exception == null) {
          String message = result.getErrorMessage();
          exception = new RuntimeException(
            message == null 
              ? "Rendering failed" 
              : "Rendering failed - " + message
          );
        }
        reportException(exception);
        return immediateFailedFuture(exception);
      }
    });
}

Срабатывание PVS-Studio: V6008 Potential null dereference of 'result'. RenderTask.java 1316

В лямбду параметром приходит некий result. В первом условии у нас result != null && result.isSuccess() . Т.е., если нам придёт result, равный null, мы пойдём в блок else. А там, как вы видите, мы его сразу разыменовываем.

Причём ниже размещён блок кода, в котором ситуация с result == null обрабатывается. Но до него мы, к сожалению, не дойдём.

Завершение

На этом мы завершим демонстрацию, на мой взгляд, самых интересных ошибок, найденных PVS-Studio в Intellij платформе, которую использует OpenIDE.

Отдельно хочется поблагодарить OpenIDE за то, что они сами предложили нам написать такую статью. Нам было только в радость поработать над материалом, ещё и возможность для обоюдного улучшения появилась. Обо всех срабатываниях мы уведомили OpenIDE, и они передали их разработчикам. А мы, обнаружив моменты для улучшения в своём анализаторе, открыли задачи на их доработки. Win-win.

Комментарий от команды OpenIDE:

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

Авторы кода знали о том, что в нём теоретически есть проблема, но не стали её исправлять. Мы тоже не станем делать это без какого-то дополнительного исследования.

Большое спасибо разработчикам PVS-Studio за замечательный инструмент. Мы постараемся пользоваться им чаще и на более регулярной основе :)

К слову, если вы пользуетесь OpenIDE, то PVS-Studio в неё интегрируется. Подробнее об этом можно прочитать здесь. Если вы ещё не пользовались OpenIDE, попробовать можно по этой ссылке.

Если хотите попробовать анализатор PVS-Studio на своём C, C++, C# или Java проекте, то вам прямиком сюда.

P.S. А ещё у нас с OpenIDE есть совместный подкаст. Крайне рекомендую, если ещё не смотрели.

На этом будем с вами прощаться. Если вам было интересно, подписывайтесь на наш блог.

До скорых встреч!