Apache NetBeans — одна из первых IDE для Java, поддержка которой не прекращается на протяжении почти 30 лет. Совсем недавно вышла 21-я версия. Мы решили проверить исходный код такого долгожителя и выбрали наиболее интересные ошибки, которые разберём в этой статье.

Как и что мы проверяем?

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

Интереснее всего использовать анализатор на больших проектах с огромной кодовой базой, которую невозможно полностью изучить с помощью code review. К таким проектам, несомненно, можно отнести open-source IDE Apache NetBeans для языка Java, первый выпуск которой состоялся в далёком 1997 году. А совсем недавно вышла 21-я версия этой IDE.

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

Копируй, но проверяй

Начнём с ошибок copy-paste. Выполняя копирование своего кода, можно забыть поменять всего лишь один символ. В итоге он становится причиной неправильной работы программы.

private void refresh(Node[] newSelection) {
  ....
  if(entry1 != null && entry2 != null && file1 != null && file2 != null) {
    if(entry1.getDateTime().getTime() > entry1.getDateTime().getTime()) {
      refreshRevisionDiffPanel(entry1, entry2, file1, file2);
    } else {
      refreshRevisionDiffPanel(entry2, entry1, file2, file1);
    }
    return;
  }
  ....
}

Обратите внимание на условие второго if, в нем идёт сравнение двух одинаковых объектов между собой:

entry1.getDateTime().getTime() > entry1.getDateTime().getTime().

Анализатор выдаёт следующее предупреждение на этот фрагмент кода:

V6001 There are identical sub-expressions 'entry1.getDateTime().getTime()' to the left and to the right of the '>' operator. HistoryDiffView.java(130)

Вот ещё одна похожая ошибка:

private RevisionInterval getResortedRevisionInterval(SVNRevision revision1, 
                                                     SVNRevision revision2) {
  RevisionInterval ret; 
  if(revision1.equals(SVNRevision.HEAD) && 
     revision1.equals(SVNRevision.HEAD)) {
    ....
  }
  return ret;
}

Указанный здесь метод используется в работе с системой контроля версий SVN и вычисляет интервал между двумя ревизиями: revision1 и revision2. Причём в логическом условии происходит проверка того, что ревизия является одной из самых последних (SVNRevision.HEAD). Вот только разработчик перепутал и вместо сравнения обоих ревизий сравнивает только первую.

Сообщение анализатора PVS-Studio:

V6001 There are identical sub-expressions 'revision1.equals(SVNRevision.HEAD)' to the left and to the right of the '&&' operator. RevertModifications.java(387), RevertModifications.java(387)

Идём дальше.

public boolean paintDragFeedback(....)
{
  ....
  if (x1 >= x2) {
    x1 = contInsets.left;
    x2 = contSize.width - contInsets.right;
    if (x1 >= x2) return true; // container is too small
  }
  if (y1 >= y2) {
    y1 = contInsets.top;
    x2 = contSize.height - contInsets.bottom;                // <=
    if (y1 >= y2) return true; // container is too small
  }
  ....
}

Посмотрите повнимательнее на два блока if: первый обрабатывает координату x, второй — y. Однако во втором блоке забыли поменять x2 на y2. В результате весь блок выполняет свой функционал неверно. Анализатор выдаёт следующее предупреждение:

V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition. BorderLayoutSupport.java(264), BorderLayoutSupport.java(263)

Переходим к следующей ошибке.

public void preferenceChange(PreferenceChangeEvent evt) {
  String k = evt == null ? null : evt.getKey();  
  ....
  try {
    if (k == null || k.equals(FoldUtilitiesImpl.PREF_OVERRIDE_DEFAULTS)) {
     ....
    }
    if (k == null || k.equals(SimpleValueNames.CODE_FOLDING_ENABLE)) {
     ....
    } 
    if (k == null || FoldUtilitiesImpl.PREF_CONTENT_PREVIEW.equals
                              (FoldUtilitiesImpl.PREF_CONTENT_PREVIEW)) {
      ....
    }
    if (k == null || FoldUtilitiesImpl.PREF_CONTENT_SUMMARY.equals
                              (FoldUtilitiesImpl.PREF_CONTENT_SUMMARY)) {
      ....
    } 
    ....
  } 
  ....
}

В этом фрагменте кода в третьем и четвёртом блоках if в условии строки FoldUtilitiesImpl.PREF_CONTENT_PREVIEW и FoldUtilitiesImpl.PREF_CONTENT_SUMMARY сравнивают с самими собой, а не со строкой k. Значит, что действия, которые описаны в этих блоках, будут выполняться всегда, даже когда это не требуется. Сообщения анализатора на эти две строки соответственно:

V6007 Expression 'FoldUtilitiesImpl.PREF_CONTENT_PREVIEW.equals(FoldUtilitiesImpl.PREF_CONTENT_PREVIEW)' is always true. FoldOptionsPanel.java(177)

V6007 Expression 'FoldUtilitiesImpl.PREF_CONTENT_SUMMARY.equals(FoldUtilitiesImpl.PREF_CONTENT_SUMMARY)' is always true. FoldOptionsPanel.java(180)

Переходим к следующей ошибке.

static void createJfxExtension(Project p, FileObject dirFO, 
                               WizardType type) throws IOException {
  FileObject templateFO = FileUtil.getConfigFile("Templates/JFX/jfx-impl.xml"); 
  if (templateFO != null) {
    ....
    if (type == JavaFXProjectWizardIterator.WizardType.SWING) {
      ....
      FileObject swingTemplateFO1 = FileUtil.getConfigFile(....); 
      if (swingTemplateFO1 != null) {
        FileUtil.copyFile(swingTemplateFO1, 
                          templatesFO, "FXSwingTemplate"); 
      }
      FileObjectswingTemplateFO2=FileUtil.getConfigFile(....); 
      if (swingTemplateFO1 != null) {
        FileUtil.copyFile(swingTemplateFO2, 
                          templatesFO, "FXSwingTemplateApplet"); 
      }
      FileObject swingTemplateFO3 = FileUtil.getConfigFile(....); // NOI18N
      if (swingTemplateFO1 != null) {
        FileUtil.copyFile(swingTemplateFO3, 
                          templatesFO,"FXSwingTemplateApplication"); 
      }
    }
    JFXProjectUtils.addExtension(p);
  }
}

А вот в этом примере в методе описаны инструкции для открытия трёх разных файлов конфигурации с их последующим копированием в один. Вроде бы всё правильно: перед каждым копированием происходит проверка, что файл открыт. Но вот проверяют всё время только swingTemplateF01. Ошибка и падение программы могут произойти, если swingTemplateF02 или swingTemplateF03 не удастся открыть.

Сообщение анализатора:

V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition. JFXProjectUtils.java(768), JFXProjectUtils.java(769)

Это ещё не всё.

public Object getElementAt(int index) {
  ....
  int myMinIndex = getExternal (minIndex) + 1; 
  // one after the index of the first non-1 index
  int myMaxIndex = getExternal (maxIndex);

  assert myMaxIndex >= myMaxIndex : "Must be greater"; // NOI18N
  ....
}

Ошибка копипасты может помешать не только работе программы, но и отладке во время разработки. В этой части кода в утверждении написано условие myMaxIndex >= myMaxIndex, хотя должно быть myMaxIndex >= myMinIndex.

Эти индексы используются для заполнения диапазона массива определённым значением. Так как в assert записано неправильное выражение, то не будет заблаговременно обнаружена ошибка в логике программы, из-за которой myMaxIndex меньше myMinIndex. Если бы условие утверждения было записано правильно, то это сократило бы время поиска ошибки и отладки.

Предупреждение PVS-Studio:

V6001 There are identical sub-expressions 'myMaxIndex' to the left and to the right of the '>=' operator. FilteredListModel.java(319)

Переходим к следующему фрагменту.

private void mergeParallelInclusions(List<IncludeDesc> inclusions, 
                                     IncludeDesc original,
                                     boolean preserveOriginal) {
  IncludeDesc best = null; 
  ....
  // 2nd remove incompatible inclusions, move compatible ones to same level
  for (Iterator it=inclusions.iterator(); it.hasNext(); ) {
    IncludeDesc iDesc = (IncludeDesc) it.next();
    if (iDesc != best) {
      if (!compatibleInclusions(iDesc, best, dimension)) {
        it.remove();
      } else if (iDesc.parent == best.parent && 
                 iDesc.neighbor == best.neighbor && 
                (iDesc.neighbor != null || iDesc.index == iDesc.index)) {  // <=
        it.remove(); // same inclusion twice (detect for better robustness)
      }
      ....
    }
    ....
  }
  ....
  if (unifyGaps != null) {
    // unify resizability of the border gaps collected for individual inclusions
    for (LayoutInterval[] gaps : unifyGaps) {
      int preferredFixedSide = 
                         fixedSideGaps[LEADING] >= fixedSideGaps[TRAILING] ? 
                                                   LEADING : TRAILING;
      for (int i=LEADING; i <= TRAILING; i++) {
        if (LayoutInterval.canResize(gaps[i]) && !anyResizingNeighbor[i]
            && (anyResizingNeighbor[i^1] || preferredFixedSide == i)) {
          operations.setIntervalResizing(gaps[i], false);
          if (!LayoutInterval.canResize(gaps[i^1])) {
            operations.setIntervalResizing(gaps[i^i], true);       // <=
          }
          break;
        }
      }
    }
  }
}

А вот этот метод размером почти в 1500 строк кода. Учитывая такие размеры, становится понятно, почему некоторые вещи не были обнаружены и устранены сразу.

Для начала давайте посмотрим на условия, описанные в первом отмеченном цикле по коллекции. Внутри тела происходит сравнение полученного объекта iDesk с объектом best. В одном из условий сравнения используются все поля объекта. Однако программист ошибся и вместо iDesc.index == best.index написал iDesc.index == iDesc.index.

Теперь посмотрим на фрагменты кода из второго цикла. Обратим внимание на определение индекса элемента массива в строке if (!LayoutInterval.canResize(gaps[i^1])). В качестве индекса в массиве gaps используется результат операции исключающего ИЛИ единицы с индексом i. Однако в следующей строчке в использовании этой бинарной операции есть ошибка: operations.setIntervalResizing(gaps[i^i], true). Здесь исключающее ИЛИ выполняется между одним и тем же элементом: результат такого выражения равен нулю. Логичнее было бы сразу написать ноль, если подразумевалось именно это.

Сообщения анализатора:

V6001 There are identical sub-expressions 'iDesc.index' to the left and to the right of the '==' operator. LayoutFeeder.java(3660)

V6001 There are identical sub-expressions 'i' to the left and to the right of the '^' operator. LayoutFeeder.java(3897)

Когда лень писать методы с нуля

Все предыдущие примеры были связаны с copy-paste отдельных строк кода. В следующих же фрагментах копировали уже целые методы!

public final class EditorMimeTypesImpl 
             implements EditorMimeTypesImplementation {

  private final PropertyChangeSupport listeners;

  @Override
  public void addPropertyChangeListener(@NonNull final 
                                    PropertyChangeListener listener) {
    Parameters.notNull("listener", listener);   //NOI18N
    listeners.addPropertyChangeListener(listener);
  }

  @Override
  public void removePropertyChangeListener(@NonNull final 
                                    PropertyChangeListener listener) {
    Parameters.notNull("listener", listener);   //NOI18N
    listeners.addPropertyChangeListener(listener);
  }
}

Посмотрите на эти два метода. Они взаимодействуют с объектом listeners, внутри которого организована коллекция. Первый метод используется для добавления элемента, а второй для удаления. Но внутри метода removePropertyChangeListener тело полностью повторяет метод addPropertyChangeListener. В таком случае удаление элементов из этой коллекции невозможно в принципе.

Анализатор PVS-Studio выдаёт следующее сообщение:

V6032 It is odd that the body of method 'addPropertyChangeListener' is fully equivalent to the body of another method 'removePropertyChangeListener'. EditorMimeTypesImpl.java(63), EditorMimeTypesImpl.java(69)

И это не единичный случай:

public class MethodParamsTipPaintComponent extends JToolTip {
  protected int getWidth(String s, Font font) {
    if (font == null) return fontMetrics.stringWidth(s);
      return getFontMetrics(font).stringWidth(s);
  }

  protected int getHeight(String s, Font font) {
    if (font == null) return fontMetrics.stringWidth(s);
      return getFontMetrics(font).stringWidth(s);
  }
}

Вот ещё два метода, один из которых решили полностью скопировать. В методе getHeight мы получаем то же значение, что и в методе getWeight.

Сообщение PVS-Studio:

V6032 It is odd that the body of method 'getWidth' is fully equivalent to the body of another method 'getHeight'. MethodParamsTipPaintComponent.java(121), MethodParamsTipPaintComponent.java(126)

Стреляем в ногу нулевой ссылкой

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

Иногда программист обращается к ссылке до того, как проверит её на null. В этом примере возникла именно такая ситуация:

public void setAllData(String key, String[] data) {
  ....
  // remove superfluous data
  if (getEntryCount() > data.length/3) {
    for (int j=0; j < getEntryCount(); j++) {
      PropertiesFileEntry pfe = getNthEntry(j);
      PropertiesStructure ps = pfe.getHandler().getStructure();
      if (pfe == null || ps == null) continue;
      ....
    }
  }
}

В этом примере получен объект pfe, вызван его метод, и только потом выполнена проверка на null.

Вот что выдал анализатор:

V6060 The 'pfe' reference was utilized before it was verified against null. BundleStructure.java(423), BundleStructure.java(424)

Переходим к следующей ошибке.

public void run() {
  for (SourceJavadocAttacherImplementation.Definer d : plugins) {
    ....
    List<? extends URL> s = source ?
      d.getSources(root, this) : d.getJavadoc(root, this);
    if (s != null || s.isEmpty()) {
    ....
    }
  }
}

А в этом примере программист неправильно применил логические операции, использовав || вместо &&. Список s будет проверяться на отсутствие в нём элементов только в том случае, если ссылки на список нет. В отличии от предыдущего примера статический анализатор классифицирует ошибку как:

V6008 Null dereference of 's'. SourceJavadocAttacherUtil.java(141).

Вот ещё одна похожая ошибка в логике работы со ссылками:

public void propertyChange(PropertyChangeEvent evt) {
  ....
  synchronized (this) {
    artifacts = null;
    if (listeners == null && listeners.isEmpty()) {
      return;
    }
    ....
  }
}

Список listeners проверяют на отсутствие в нём элементов, если ссылка на listeners нулевая.

Анализатор выдаёт сообщение:

V6008 Null dereference of 'listeners'. MavenArtifactsImplementation.java(613)

Идём далее:

private SourcesModel getModel () {
  SourcesModel tm = model.get ();
  if (tm == null) {
    tm.sourcePath.removePropertyChangeListener (this);
    tm.debugger.getSmartSteppingFilter ().
    removePropertyChangeListener (this);
  }
  return tm;
}

В этом фрагменте кода ошибка нулевой ссылки достигает нового уровня. Разыменование нулевой ссылки будет выполнено тогда, когда мы будем уверены, что ссылка точно нулевая. Причём данная функция используется в 136 местах! Очевидно, что разработчик не планировал реализацию именно такого поведения программы. Если такая ошибка не была замечена сразу, значит ссылка tm практически никогда не бывает нулевой. Однако это не отменяет того факта, что часть запланированного функционала постоянно не выполняется.

Предупреждение PVS-Studio:

V6008 Null dereference of 'tm'. SourcesModel.java(713)

В проекте нашлась ещё одна точно такая же ошибка:

void unsetStepSuspendedByBpIn(JPDAThreadImpl thread) {
  JPDABreakpoint oldBp;
  synchronized (stepBreakpointLock) {
  ....
    if (this.suspendedSteppingThreads == null) {
      this.suspendedSteppingThreads.remove(thread);
      ....
    }
  }
  ....
}

В этом фрагменте удаление ссылки на поток thread происходит из несуществующей коллекции.

Анализатор классифицирует ошибку как:

V6008 Null dereference of 'this.suspendedSteppingThreads'. JPDAThreadImpl.java(2329)

Подобных ошибок, связанных с разыменованием нулевой ссылки, много. Рассмотрим ещё несколько.

public @Override Element getElement() {
  if (view != null) {
    return view.getElement();
  } else {
    return view.getDocument().getDefaultRootElement();
  }
}

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

Сообщение анализатора:

V6008 Null dereference of 'view'. CollapsedView.java(627)

Следующая ошибка интереснее. На null проверить не забыли, но просчитались:

public ModulePathsProblemsProvider(@NonNull final Lookup baseLkp) {
  this.project = baseLkp.lookup(J2SEProject.class);
  if (this.project == null) {
    throw new IllegalArgumentException(String.format(
       "Unsupported project: %s of type: %s",  //NOI18N
        project,
        project.getClass()
       ));
  }
  this.moduleInfoListeners = new HashSet<>();
  this.listeners = new PropertyChangeSupport(this);
}

Разыменование ссылки project происходит при попытке извлечь информацию для выбрасывания исключения о том, что ссылка project нулевая*.* Хотели как лучше, но не в этот раз.

Предупреждение PVS-Studio:

V6008 Null dereference of 'project'. ModulePathsProblemsProvider.java(93)

На этом фрагменты кода, гарантированно приводящие к NullPointerException, не заканчиваются.

public static void main(String args[]) throws Exception {
  ....
  Bridge lex = null; //new XMLSyntaxTokenManager(input);
  int i = 25; //token count
  int id;
  int toks = 0;
  long time = System.currentTimeMillis();
  while (i/*--*/>0) {

    lex.next();
    ....
  }
}

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

Анализатор негодует и выдаёт предупреждение:

V6008 Null dereference of 'lex'. XMLSyntaxTokenManager.java(216)

Ошибка наоборот

Теперь перейдём к более необычным для этого проекта ошибкам.

private void changeIgnoreStatus (File f) throws IOException {
  File parent = f;
  boolean isDirectory = f.isDirectory() && (! Files.isSymbolicLink(f.toPath()));
  StringBuilder sb = new StringBuilder('/');
  if (isDirectory) {
    sb.append('/');
  }
  boolean cont = true;
  while (cont) {
    sb.insert(0, parent.getName()).insert(0, '/');
    parent = parent.getParentFile();
    String path = sb.toString();
    if (parent.equals(getRepository().getWorkTree())) {
      if (addStatement(new File(parent, Constants.DOT_GIT_IGNORE), 
          path, isDirectory, false) &&
          handleAdditionalIgnores(path, isDirectory)) {
        addStatement(new File(parent, Constants.DOT_GIT_IGNORE),
                              path, isDirectory, true);
      }
      cont = false;
    } else {
      cont = addStatement(new File(parent, Constants.DOT_GIT_IGNORE), 
                                   path, isDirectory, false);
   }
}

В конструктор StringBuilder передаётся символ '/'. Кажется, что здесь ничего такого нет: просто создают экземпляр StringBuilder и сразу добавляют в него символ. Однако Java интерпретирует этот символ как число, и в результате sb вместо получения начального символа в виде слеша резервирует место в памяти под 47 символов: число 47 соответствует символу '/' в таблице ASCII.

А теперь самое интересное. В этом коде происходит генерация абсолютного пути до файла f. Если в конструкторе sb слеш был бы указан как строка, то он стал бы начальным символом. Далее путь дописывается в начало этой строки, тем самым оставляя слеш в конце. Исходя из этого алгоритма, мы бы получали следующие пути:

"/folder1/folder2/f/" для файла;
"/folder1/folder2/f//" для папки.

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

V6009 Buffer capacity is set to '47' using a char value. Most likely, the '/' symbol was supposed to be placed in the buffer. IgnoreUnignoreCommand.java(107)

Опасный код: падение неизбежно

Предлагаю посмотреть на "неудержимый" код, который просто невозможно остановить:

private class BrowserOutlineView extends OutlineView {
  ....
  public void startEditingAtPath(TreePath path) {
    startEditingAtPath(path);
  }
  ....
}

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

Предупреждение анализатора:

V6062 Possible infinite recursion inside the 'startEditingAtPath' method. BrowserPanel.java(165), BrowserPanel.java(166)

Кроме того, подобная рекурсия встречается не один раз в исходном коде:

public class SchemaReferenceProxy extends SchemaReference 
                                  implements AXIComponentProxy {
  ....
  private SchemaReference getShared() {
    return (SchemaReference)getShared();
  }
  ....
}

Изучив исходный код внимательнее, можно обнаружить, что функция getShared вызывается во всех методах класса SchemaReferenceProxy. А сам класс используется при работе с XML-файлами. Так что, если вы активно работаете в Apache NetBeans с XML-файлами, и у вас вдруг упадёт программа, то вы знаете, по какой причине это может происходить.

Анализатор выдаёт то же предупреждение:

V6062 Possible infinite recursion inside the 'getShared' method. SchemaReferenceProxy.java(40), SchemaReferenceProxy.java(41)

Подведём итог

Мы рассмотрели 21 ошибку в исходном коде Apache NetBeans. И хотя на этом мы решили остановиться, многие подозрительные моменты ещё остались без должного внимания. Были и просто странности. Например, много где использовалось тройное отрицание, например, как здесь: !!!it.hasNext(). Зачем так делать в Java, понять не получилось :)

Нужно понимать, что Apache NetBeans — это проект с огромным исходным кодом, в котором найти и исправить все ошибки вручную просто невозможно. Вот почему в разработках такого уровня использование различных инструментов, как, например, статический анализатор, становится необходимостью.

Если вам интересны проверки других IDE, то предлагаем вам ознакомиться с соответствующими статьями:

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Daniil Liakhov. 21 bugs in 21st version of Apache NetBeans.

21 ошибка в 21-й версии Apache NetBeans