Имеется большое разнообразие языков программирования, но больше всего наши пользователи хотят, чтобы анализатор PVS-Studio поддержал один из самых популярных языков – Javascript. Одним из проектов, который можно использовать для написания статического анализатора JavaScript, является движок Rhino. В данной статье мы проведём анализ исходного кода Rhino при помощи PVS-Studio.

Предыстория

Анализатор PVS-Studio, постепенно развиваясь, получал поддержку новых языков программирования. Первым дополнительным языком (к изначальным С и С++) стал C#, затем появился и Java. Поэтому пользователи анализатора часто интересуются: не появится ли в будущем в PVS-Studio нового языка, а если появится, то какой?

Я один из тех разработчиков, которые в свободное время иногда изучают что-то связанное с работой. Одной из таких тем для меня является написание статического анализатора для языка JavaScript.

Однако началось всё с другого языка, а именно, Kotlin. Однажды в свободное время я решил поизучать какой-нибудь новый язык. Так как я знаком с Java и C#, то выбор пал на Kotlin, который, по моему мнению, сочетает в себе черты обоих этих языков. В процессе изучения я узнал, что имеется возможность транспиляции Kotlin не только в Java, но и в JavaScript.

Транспиляция – это преобразование программы, при которой используется исходный код программы, написанной на одном языке программирования в качестве исходных данных, и производится эквивалентный исходный код на другом языке программирования.

Эта возможность очень заинтересовала меня. В результате я полез на GitHub изучать исходники транспилятора Kotlin в JavaScript. Как оказалось, его разработчики не стали создавать свой парсер и классы для AST (абстрактного синтактического дерева), а переиспользовали код из движка Rhino. Так что следующим проектом для изучения оказался именно он.

Движок Rhino имеет несколько интересных особенностей:

• Написан на Java;

• Первая версия написана в 1997 году (25 лет назад), когда в Java даже не было дженериков (они появились только в Java 5.0 в 2004 году);

• Движок, кроме режима работы в качестве интерпретатора, может работать и в режиме компилятора. "Как же возможно компилировать JavaScript?" – спросите вы. Всё просто: код, написанный на JavaScript, преобразуется в Java байт код;

• Работа Rhino в режиме компилятора часто оказывается производительнее C-реализации JavaScript;

• Rhino не имеет по умолчанию интеграции с браузерами;

• Одно из применений Rhino – написание бизнес-логики приложений на более простом языке JavaScript вместо Java. Эта особенность позволяет писать бизнес-логику даже специалистам, слабо разбирающимся в программировании.

Параллельно с изучением исходного кода Rhino я скачал его исходники и запустил анализ проекта при помощи плагина PVS-Studio для IntelliJ IDEA. В результате анализа набралось немало предупреждений для проекта 25-летней давности (список по убыванию степени достоверности срабатываний):

• High: 79

• Medium: 158

• Low: 208

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

• Все срабатывания диагностики V6022, по моему мнению, оказались ложными. Связано это с изменением API некоторых методов и конструкторов. V6022 ищет аргументы, которые не используются внутри тела методов или конструкторов. То есть она срабатывает, когда аргументам ничего не присваивается, у них не вызываются никакие свойства/методы, аргументы не передаются ни в один метод/конструктор, не присваиваются никаким свойствам/полям;

• Примерно четверть срабатываний правила V6008 (эта диагностика ищет места, где может произойти доступ по нулевой ссылке) низкого уровня достоверности (Low). Связано это с тем, что метод XMLList.getXML потенциально может вернуть null, если переданный индекс окажется отрицательным. Однако этого никогда не произойдёт, потому что при вызове этого метода в качестве первого аргумента всегда передаётся массив из поля _annos, а внутри метода XMLList.getXML вызывается метод XMLList.length, который так же связан с полем _annos. Поэтому метод никогда не вернёт отрицательное значение:

class XMLList extends XMLObjectImpl implements Function {
  ....
  private XmlNode.InternalList _annos;
  ....
  XMLList(XMLLibImpl lib, Scriptable scope, XMLObject prototype) {
    super(lib, scope, prototype);
    _annos = new XmlNode.InternalList();
  }
  ....
  private XML getXML(XmlNode.InternalList _annos, int index) {
    if (index >= 0 && index < length()) {
      return xmlFromNode(_annos.item(index));
    } else {
      return null;
    }
  }
  ....
  @Override
  int length() {
    int result = 0;

    if (_annos != null) {
      result = _annos.length();
    }

    return result;
  }
  ....
  private XML getXmlFromAnnotation(int index) {
    return getXML(_annos, index);               //<=
  }
  ....
}

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

• High: 79 (не изменилось)

• Medium: 47 (было 158)

• Low: 154 (было 208)

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

Забытое поле + опечатка

V6001 There are identical sub-expressions 't2Docked' to the left and to the right of the '&&' operator. SwingGui.java(2718), SwingGui.java(2718)

class ContextWindow extends JPanel implements ActionListener {
  ....
  public ContextWindow(final SwingGui debugGui) {
    ....
    ComponentListener clistener =
      new ComponentListener() {
          boolean t2Docked = true;

          void check(Component comp) {
              ....
              if (leftDocked && t2Docked && rightDocked && t2Docked) { // <=
                  // no change
                  return;
              }
              
              t2Docked = rightDocked;
              
              // Further t2Docked is not used
              ....
          }
          ....      
    };
    ....
  }
  ....
}

В конструкторе ContextWindow создается локальный объект типа интерфейса ComponentListener, который в каждом из методов, реализующих этот интерфейс, вызывает метод check. Внутри него проверяется пристыкован ли компонент к левой или правой стороне какого-то окна, а также изменяется расположение разделителей окна в JSplitPane.

В этом методе анализатор обнаружил потенциальную ошибку, которая заключается в том, что поле t2Docked объекта clistener (да-да, это поле объекта, а не переменная, так как объявлена она в блоке инициализации локального объекта) в условии if используется дважды:

if (leftDocked && 
    t2Docked &&     // <=
    rightDocked && 
    t2Docked) {     // <=
  // no change
  return;
}

Учитывая, что в методе check пристыкованность компонентов проверяется для левой и правой сторон у меня сложилось впечатление, что изначально у объекта clistener было ещё одно поле t1Docked, которое со временем удалили. В таком случае условие в if выглядело более логичным, хотя и имело бы ошибку, которая произошла, скорее всего, из-за опечатки, о которой бы также предупредил анализатор. После исправления этой опечатки условие в if стало бы таким:

leftDocked && t1Docked && rightDocked && t2Docked

Для исправления текущего кода я посоветовал бы удалить из условия одно из использований t2Docked.Например, так:

t2Docked && leftDocked && rightDocked

Копипаст, невыполнимые условия и недостижимый код

V6007 Expression 'power < 2' is always false. UintMap.java(39)

public class UintMap implements Serializable {
  ....
  // If true, enables consitency checks
  private static final boolean check = false;
  ....
  public UintMap(int initialCapacity) {
    if (initialCapacity < 0) Kit.codeBug();
    // Table grow when number of stored keys >= 3/4 of max capacity
    int minimalCapacity = initialCapacity * 4 / 3;
    int i;
    for (i = 2; (1 << i) < minimalCapacity; ++i) {}
    power = i;
    if (check && power < 2) Kit.codeBug();      // <=
  }
  ....
}

V6007 Expression 'power < 2' is always false. ObjToIntMap.java(102)

public class ObjToIntMap implements Serializable {
  ....
  // If true, enables consitency checks
  private static final boolean check = false;
  ....
  public ObjToIntMap(int initialCapacity) {
    if (initialCapacity < 0) Kit.codeBug();
    // Table grow when number of stored keys >= 3/4 of max capacity
    int minimalCapacity = initialCapacity * 4 / 3;
    int i;
    for (i = 2; (1 << i) < minimalCapacity; ++i) {}
    power = i;
    if (check && power < 2) Kit.codeBug();      // <=
  }
  ....
}

Код конструкторов классов UintMap и ObjToIntMap идентичен, отличие только в имени аргумента. В конце кода обоих конструкторов производится проверка:

if (check && power < 2) Kit.codeBug();

Если данная проверка оказывается истинной, то вызывается метод Kit.codeBug, который просто кидает исключение:

public static RuntimeException codeBug() throws RuntimeException {
  RuntimeException ex = new IllegalStateException("FAILED ASSERTION");
  // Print stack trace ASAP
  ex.printStackTrace(System.err);
  throw ex;
}

Во-первых, эта проверка зависит от значения финального приватного поля check. Так как это поле по умолчанию равно false, то выполнение никогда не дойдёт до выражения power < 2. Это поле, скорее всего, использовалось программистом для тестирования данного класса, поэтому оно всё ещё может принять значение true, когда класс будет снова тестироваться разработчиком.

Во-вторых, анализатор указал на то, что даже если выполнение дойдёт до этого выражения, то оно всегда будет ложно, потому что к моменту выполнения этой проверки значение поля power будет не меньше 2. Связано это с тем, что в power сохраняется значение локальной переменной i, которая так же является счётчиком в цикле for. Инициализируется эта переменная в цикле значением 2, а далее инкрементируется на каждой итерации цикла. Максимальное число, до которого может дорасти значение i – это 31, потому что 1 << 31 = Integer.MIN_VALUE (-2147483648). Если же произойдёт переполнение переменной minimalCapacity, то не будет выполнено ни одной итерации, а значит переменная i останется равной 2.

Из-за этого метод Kit.codeBug никогда не будет вызван, а значит анализатор помог обнаружить недостижимый код. Я не знаю как исправить данный код, так как не очень понимаю, зачем здесь эта проверка. Поэтому исправлением данной ошибки должен заняться тот программист, который писал этот код.

Память о баге, пронесённая через года

V6007 Expression '!BUG_369394_IS_VALID' is always true. XmlProcessor.java(288)

class XmlProcessor implements Serializable {
  ....
  private void addTextNodesToRemoveAndTrim(List<Node> toRemove, Node node) {
    if (node instanceof Text) {
      Text text = (Text) node;
      boolean BUG_369394_IS_VALID = false;
      if (!BUG_369394_IS_VALID) {                 // <=
        text.setData(text.getData().trim());
      } else {
        if (text.getData().trim().length() == 0) {
          text.setData("");
        }
      }
      if (text.getData().length() == 0) {
        toRemove.add(node);
      }
    }
    ....
  }
  ....
}

Предупреждения анализатора не только помогают обнаружить ошибки и уязвимости в вашем коде. Иногда они также позволяют не забыть удалить ненужный код. Программист создал в методе addTextNodesToRemoveAndTrim ветвление, зависящее от локальной булевой переменной BUG_369394_IS_VALID, которая инициализируется значением false и не изменяется по ходу метода. Анализатор же это заметил и выдал предупреждение.

Эта переменная, как я понял, оказалась необходима для фикса бага с номером 369394 с сайта Bugzilla. Учитывая, что переменной BUG_369394_IS_VALID присваивается значение false, то, скорее всего, этот баг уже не актуален. Если это так, то неплохо бы убрать ветвление, зависящее от этой переменной. Если же этот баг ещё актуален, тогда непонятно, почему значение у данной переменной false, а не true.

Когда pretty не такой уж pretty

V6007 Expression 'i < indentLevel' is always false. XmlProcessor.java(454)

class XmlProcessor implements Serializable {
  ....
  private boolean prettyPrint;
  ....
  private int prettyIndent;  
  ....
  final String ecmaToXmlString(Node node) {
    // See ECMA 357 Section 10.2.1
    StringBuilder s = new StringBuilder();
    int indentLevel = 0;
    if (prettyPrint) {
      for (int i = 0; i < indentLevel; i++) { // <=
        s.append(' ');
      }
    }
    ....
  }
  ....
}

Анализатор заметил, что выражение i < indentLevel всегда ложно, потому что на первом же цикле переменные i и indentLevel будут равны 0, а значит, ни одной итерации цикла for не произойдёт. В результате при записи информации в StringBuilder не будут добавлены отступы в виде пробелов, если выбран режим prettyPrint. Это, скорее всего, ухудшит читабельность текста, записанного в StringBuilder.

Если судить по комментарию в начале метода, то алгоритм этого метода должен соответствовать алгоритму из документации ECMA, описанном в разделе 10.2.1 (страница 41). Из документации следует, что в метод должен быть передан ещё один опциональный аргумент как раз с именем indentLevel. Появление данной ошибки я могу объяснить либо тем, что изначально этот аргумент был, но затем его удалили, либо разработчик оказался невнимателен и забыл доработать этот метод таким образом, чтобы алгоритм соответствовал документации.

Потенциальное разыменование нулевой ссылки

V6008 Potential null dereference of 'generatorState' in function 'freezeGenerator'. Interpreter.java(1244), Interpreter.java(3188)

public final class Interpreter extends Icode implements Evaluator {
  ....
  private static Object interpretLoop(Context cx, 
                                      CallFrame frame, 
                                      Object throwable) {
  
    GeneratorState generatorState = null;
    if (throwable != null) {
      if (throwable instanceof GeneratorState) {
        generatorState = (GeneratorState) throwable;
        ....
      } else if (!(throwable instanceof ContinuationJump)) {
        Kit.codeBug();
      }
    }
    ....
    StateLoop:
    for (; ; ) {
      ....
      if (throwable != null) {
        ....
      } else {
        if (generatorState == null && frame.frozen) Kit.codeBug();
      }
    
      for (; ; ) {
        ....
        int op = iCode[frame.pc++];
        jumplessRun:
        {
          switch (op) {
            ....
            case ....:
              {
                if (!frame.frozen) {
                  return freezeGenerator(...., 
                                         generatorState, // <=
                                         ....);
                }                  
                ....
              }
            // many more cases
            ....
          }
        }
        ....
      }               
      ....
    }
    ....
  }
  ....
}

Анализатор сообщает, что внутри метода freezeGenerator может произойти разыменование generatorState, когда он будет равен null. Учитывая, что метод interpretLoop растянулся почти на 1500 строк, это было бы сложно (или почти невозможно) понять без анализатора. Я считаю, что, скорее всего, анализатор оказался прав. Доказательство начнём от места вызова метода freezeGenerator.

Первое. Поле frame.frozen выставляется в false по умолчанию у всех объектов CallFrame при клонировании:

CallFrame cloneFrozen() {
  ....
  copy.frozen = false;        // <=
  return copy;
}

Второе. Вызов метода freezeGenerator произойдёт только если frame.frozen будет ложно:

  ....
  if (!frame.frozen) {
    return freezeGenerator(
      cx,
      frame,
      stackTop,
      generatorState,                // <=
      op == Icode_YIELD_STAR);
  }
  ....

Третье. Выше вызова этого метода имеется проверка на null переменной generatorState:

if (generatorState == null && frame.frozen) Kit.codeBug();

Если выражение в if окажется истинным, то произойдёт исключение, потому что метод Kit.codeBug без всяких проверок выбрасывает исключение:

public static RuntimeException codeBug() throws RuntimeException {
  RuntimeException ex = new IllegalStateException("FAILED ASSERTION");
  // Print stack trace ASAP
  ex.printStackTrace(System.err);
  throw ex;
}

Четвёртое. Производиться эта проверка будет только если аргумент throwable метода interpretLoop оказывается равен null:

  if (throwable != null) { // <=
    ....
  } else {
    if (generatorState == null && frame.frozen) Kit.codeBug(); // <=
  }

Пятое. Если посмотреть ещё выше, то обнаружится, что изначально значение переменной generatorState равно null, а изменяется оно только если throwable оказывается не равен null и является экземпляром класса GeneratorState:

  ....
  GeneratorState generatorState = null; // <=
  if (throwable != null) {
    if (throwable instanceof GeneratorState) {
      generatorState = (GeneratorState) throwable; // <=

      // reestablish this call frame
      enterFrame(cx, frame, ScriptRuntime.emptyArgs, true);
      throwable = null;
    } else if (!(throwable instanceof ContinuationJump)) {
      // It should be continuation
      Kit.codeBug();
    }
  }
  ....

В результате. Учитывая всё вышесказанное, получается, что если throwable окажется равен null или не GeneratorState, то и generatorState будет равен null. А если уж и frame.frozen будет ложно, то при вызове метода freezeGenerator произойдёт исключение NullPointerException при обращении к полю operation:

private static Object freezeGenerator(...., 
                                      GeneratorState generatorState, 
                                      ....) {
  if (generatorState.operation == NativeGenerator.GENERATOR_CLOSE){     // <=
      // Error: no yields when generator is closing
      throw ScriptRuntime.typeErrorById("msg.yield.closing");
  }
  ....
}

Недостижимая обработка ошибки

V6019 Unreachable code detected. It is possible that an error is present. Parser.java(3138)

public class Token {
  ....
  public static final int ERROR = -1, .... 
  ....
}

public class Parser {

  static final int CLEAR_TI_MASK = 0xFFFF, ....;    
  private AstNode primaryExpr() throws IOException {
    int ttFlagged = peekFlaggedToken();
    int tt = ttFlagged & CLEAR_TI_MASK; 

    switch (tt) {
      ....
      case Token.ERROR:
            consumeToken();                                               // <=
            // the scanner or one of its subroutines reported the error.
            break;
      ....
    }
    ....
  }
  ....
}

Метод peekFlaggedToken возвращает значение int, которое затем изменяется при помощи побитовой операции "&" (и). Причем правый операнд этого выражения – статическая неизменяемая константа 0xFFFF (65535). Поэтому результатом побитового "и" всегда будет число >= 0. Однако далее по коду результат побитового "и" используется в switch, где одно из ветвлений выполняется, когда значение выражения в switch равно Token.ERROR, а именно -1. Эта ветвь никогда не будет выполнена, о чём и предупреждает анализатор.

Копипаст и потерянные фигурные скобки

V6021 The value is assigned to the 'localMax' variable but is not used. NativeRegExp.java(568)

public class NativeRegExp extends IdScriptableObject {
  ....
  private static boolean calculateBitmapSize(....) {
    ....                             
    switch (src[index]) {
      case '\\':
        ....
        switch (c) {
          ....
          case 'c':
            if ((index < end) && isControlLetter(src[index]))
              localMax = (char) (src[index++] & 0x1F);         // <=
            else --index;
            localMax = '\\';
            break;
          ....
        }
      ....
    }
    // use localMax
    ....
  }
  ....
}

В ветке if результат выражения:

(char) (src[index++] & 0x1F)

сохраняется в переменную localMax. Однако записанное значение не используется до записи нового значения '\\' в localMax. После выхода из switch значение localMax используется в if условии. Поэтому вполне возможна ситуация, что одна из ветвей if...else не будет выполнятся тогда, когда это необходимо. В этом же классе имеется метод processCharSetImpl, где используется почти полностью идентичный по структуре switch. Однако в нём в ветви для символа 'c' никакой ошибки нет, потому что тело else обёрнуто в фигурные скобки:

....
case 'c':
  if ((src < end) && isControlLetter(gData.regexp.source[src]))
    thisCh = (char) (gData.regexp.source[src++] & 0x1F);
  else {
    --src;
    thisCh = '\\';
  }
  break;
....

Соответственно, чтобы исправить эту ошибку, надо обернуть инструкции после else и до break в фигурные скобки в методе NativeRegExp.calculateBitmapSize:

case 'c':
  if ((index < end) && isControlLetter(src[index]))
    localMax = (char) (src[index++] & 0x1F);         
  else { // <=
    --index;
    localMax = '\\';
  }      // <=
  break;
....

Забытое неиспользуемое значение

V6021 The value is assigned to the 'nameString' variable but is not used. QName.java(293)

final class QName extends IdScriptableObject {
  ....
  QName constructQName(XMLLibImpl lib, 
                       Context cx, 
                       Object namespace, 
                       Object name) {
    String nameString = null;
    if (name instanceof QName) {
      if (namespace == Undefined.instance) {
        return (QName) name;
      } else {
        nameString = ((QName) name).localName();          // <=
      }
    }
    if (name == Undefined.instance) {
      nameString = "";
    } else {
      nameString = ScriptRuntime.toString(name);
    }
    ....
    return newQName(lib, q_uri, q_localName, q_prefix);
  }
  ....
}

Диагностика V6021 обнаружила, что в переменную nameString сохраняется результат выражения:

((QName) name).localName()

Однако далее в обоих ветвлениях if...else значение nameString перезаписывается прежде, чем используется:

if (name == Undefined.instance) {
  nameString = "";
} else {
  nameString = ScriptRuntime.toString(name);
}

Скорее всего, данная ошибка произошла из-за невнимательности программиста или в результате рефакторинга. Предполагаю, что между сохранением значения в nameString в начале метода и изменением его в if...else оно должно было быть использовано. Также возможно, что в if...else забыли добавить ещё одну ветвь, где использовалось (или как минимум не менялось) значение nameString.

Потенциальная ошибка и зависимые аргументы

V6025 Index 'sig1.length - 1' is out of bounds. NativeJavaMethod.java(461)

V6025 Index 'sig2.length - 1' is out of bounds. NativeJavaMethod.java(462)

public class NativeJavaMethod extends BaseFunction {

  private static int preferSignature(Object[] args, 
                                     Class<?>[] sig1, 
                                     boolean vararg1, 
                                     Class<?>[] sig2, 
                                     boolean vararg2) {
    int totalPreference = 0;
    for (int j = 0; j < args.length; j++) {
      Class<?> type1 = vararg1 && 
                       j >= sig1.length ? sig1[sig1.length - 1] // <=
                                        : sig1[j];             
      Class<?> type2 = vararg2 && 
                       j >= sig2.length ? sig2[sig2.length - 1] // <=
                                        : sig2[j];             
      ....
    }
    return totalPreference;
  }
}

Анализатор предупреждает, что доступ по индексу в выражениях sig1[sig1.length - 1] и sig2[sig2.length - 1] может привести к возникновению исключения ArrayIndexOutOfBoundsException. Оба этих выражения и правда могут привести к возникновению этого исключения, если массивы sig1 или sig2 окажутся пустыми, потому что в этом случае будет произведен доступ по индексу -1 (0-1 = -1). Даже проверки тернарных операторов j >= sig1.length и j >= sig2.length не защищают от доступа по индексу -1 на первой итерации цикла for.

Фактически, в текущей кодовой базе данный метод зовётся только в одном месте и аргументы, которые передаются в него при вызове, зависимы друг от друга, а именно, если vararg1 истинно, то и массив sig1 будет иметь минимум один элемент. Аналогичная зависимость имеется и между vararg2 и массивом sig2. Связано это с рефлексией Java, потому что если вызов метода isVararg у объектов java.lang.reflect.Method или java.lang.reflect.Constructor вернёт true, то и вызов метода getParameterTypes вернет массив, в котором имеется хотя бы один элемент (первый аргумент, объявление которого является vararg-ом). Объекты именно этих классов используются в исходном коде как аргументы вызова метода preferSignature. Так что в текущем исходном коде ошибки, о которой предупреждает анализатор, никогда не произойдёт. И всё-таки в будущем остаётся возможность некорректного вызова данного метода, что приведёт к доступу по индексу -1.

Чтобы защититься от некорректного вызова метода preferSignature в будущем, советую сразу вернуть -1, если vararg1 истина и массив sig1 пустой или vararg2 истина и массив sig2 пустой:

private static int preferSignature(Object[] args, 
                                     Class<?>[] sig1, 
                                     boolean vararg1, 
                                     Class<?>[] sig2, 
                                     boolean vararg2) {
    if (vararg1 && sig1.length == 0 || vararg2 && sig2.length == 0){
      return -1;
    }
                                                                    
    int totalPreference = 0;
    for (int j = 0; j < args.length; j++) {
      Class<?> type1 = vararg1 && 
                       j >= sig1.length ? sig1[sig1.length - 1]
                                        : sig1[j];             
      Class<?> type2 = vararg2 && 
                       j >= sig2.length ? sig2[sig2.length - 1]
                                        : sig2[j];             
      ....
    }
    return totalPreference;
  }
}

Небезопасный доступ к элементу массива

V6025 Possibly index 'last - 1' is out of bounds. SuperBlock.java(50)

final class SuperBlock {
  ....
  int[] getTrimmedLocals() {
    int last = locals.length - 1;
    while (last >= 0
        && locals[last] == TypeInfo.TOP
        && !TypeInfo.isTwoWords(locals[last - 1])) {         // <=
      last--;
    }
    ....
  }
  ....
}

В начале метода getTrimmedLocals происходит перебор элементов массива из поля locals при помощи цикла while. Однако в условие цикла while закралась потенциальная ошибка, которая заключается в доступе к элементу массива locals по индексу -1, что приведет к генерации исключения ArrayIndexOutOfBoundsException.

Это произойдёт, если счётчик цикла last окажется равен 0, и одновременно первый элемент массива (индекс 0) будет равен значению TypeInfo.TOP (значение 0). В таком случае последнее выражение из условия цикла while будет выполнено, и перед вызовом метода TypeInfo.isTwoWords будет осуществлен доступ к элементу массива locals по индексу 0 - 1 = -1.

Предполагаю, что исключения ArrayIndexOutOfBoundsException могло ещё ни разу не произойти просто потому, что где-то в коде в массив locals первым элементом записывается всегда значение, не равное TypeInfo.TOP (значение 0). В таком случае, чтобы наверняка защититься от подобного исключения в будущем, лучше заменить индекс в первой проверке на 1 вместо 0 в условии цикла while:

while (last >= 1                          // <=
    && locals[last] == TypeInfo.TOP
    && !TypeInfo.isTwoWords(locals[last - 1])) {         
  last--;
}

Либо вместо оператора >= (больше или равно) использовать оператор > (больше):

while (last > 0                                  // <=
    && locals[last] == TypeInfo.TOP
    && !TypeInfo.isTwoWords(locals[last - 1])) {         
  last--;
}

Взаимоуничтожающиеся операнды

V6028 Identical expression 'end' to the left and to the right of compound assignment. FastDtoaBuilder.java(74)

public class FastDtoaBuilder {
  ....
  private void toFixedFormat(int firstDigit, int decPoint) {
    if (point < end) {
      ....
    } else if (point > end) {
      // large integer, add trailing zeroes
      Arrays.fill(chars, end, point, '0');
      end += point - end;                        /// <=
    }
  }
  ....
}

Анализатору показалась странной последняя строка в блоке else if. Если расписать более подробно операции в нём и упростить код, то станет понятнее, о чём предупреждает анализатор:

end = end + point – end 
=> end = end – end + point 
=> end = + point 
=> end = point

Как видите, всё что в действительности произойдёт, так это присваивание переменной end значения переменной point.

Скорее всего, ошибки в данном случае нет, а данное выражение осталось после рефакторинга. Однако даже в таком случае я бы посоветовал заменить использование оператора += на обычное присваивание значения point переменной end.

Закомментированный код

V6032 It is odd that the body of method 'endCheckSwitch' is fully equivalent to the body of another method 'endCheckTry'. Node.java(681), Node.java(717)

public class Node implements Iterable<Node> {
  ....
  private int endCheckSwitch() {
    int rv = END_UNREACHED;

    // examine the cases
    //         for (n = first.next; n != null; n = n.next)
    //         {
    //             if (n.type == Token.CASE) {
    //                 rv |= ((Jump)n).target.endCheck();
    //             } else
    //                 break;
    //         }

    //         // we don't care how the cases drop into each other
    //         rv &= ~END_DROPS_OFF;

    //         // examine the default
    //         n = ((Jump)this).getDefault();
    //         if (n != null)
    //             rv |= n.endCheck();
    //         else
    //             rv |= END_DROPS_OFF;

    //         // remove the switch block
    //         rv |= getIntProp(CONTROL_BLOCK_PROP, END_UNREACHED);

    return rv;
  }
  ....
  private int endCheckTry() {
    int rv = END_UNREACHED;

    // a TryStatement isn't a jump - needs rewriting

    // check the finally if it exists
    //         n = ((Jump)this).getFinally();
    //         if(n != null) {
    //             rv = n.next.first.endCheck();
    //         } else {
    //             rv = END_DROPS_OFF;
    //         }

    //         // if the finally block always returns, then none of the returns
    //         // in the try or catch blocks matter
    //         if ((rv & END_DROPS_OFF) != 0) {
    //             rv &= ~END_DROPS_OFF;

    //             // examine the try block
    //             rv |= first.endCheck();

    //             // check each catch block
    //             n = ((Jump)this).target;
    //             if (n != null)
    //             {
    //                 // point to the first catch_scope
    //                 for (n = n.next.first; n != null; n = n.next.next)
    //                 {
    //                     // check the block of user code in the catch_scope
    //                     rv |= n.next.first.next.first.endCheck();
    //                 }
    //             }
    //         }

    return rv;
  }
}

Данное срабатывание не является ошибкой, однако, благодаря диагностике V6032 обнаружены методы, выполняющие одно и то же. Происходит это потому, что оба метода должны были быть доработаны в будущем. Об этом свидетельствуют комментарии и закомментированные куски кода внутри этих функций. Таким образом, анализатор помог обнаружить методы с закомментированным кодом, которые необходимо доработать. Без подсказки анализатора программист мог бы и забыть доработать эти методы в будущем, ведь тут даже нет TODO-комментария.

Как сложно жить Java-программистам без директив компилятора

V6048 This expression can be simplified. Operand 'lParenIndex' in the operation equals 0. ClassFileWriter.java(2658)

public class ClassFileWriter {
  ....
  private int[] createInitialLocals() {
    int[] initialLocals = new int[itsMaxLocals];
    ....
    // No error checking should be necessary, sizeOfParameters does this
    String type = itsCurrentMethod.getType();
    int lParenIndex = type.indexOf('(');
    int rParenIndex = type.indexOf(')');
    if (lParenIndex != 0 || rParenIndex < 0) {
      throw new IllegalArgumentException("bad method type");
    }
    int start = lParenIndex + 1;                                  // <=
    StringBuilder paramType = new StringBuilder();
    while (start < rParenIndex) {
      switch (type.charAt(start)) {
      ....
      }
      ....
    }
    return initialLocals;  
  }
  ....
}

Строка, возвращаемая методом itsCurrentMethod.getType, должна быть валидной, а именно, начинаться с открывающейся скобки "(" и иметь закрывающую скобку ")". Это следует из комментария перед вызовом этого метода. Однако кто-то решил перестраховаться и добавил if с проверками значений переменных lParenIndex и rParenIndex, и генерацией исключения, если одна из этих проверок окажется истинной. В результате, после if переменная lParenIndex теперь всегда имеет значение 0. Эту особенность и обнаружил анализатор.

Изучив подробно все места создания объектов типа ClassFileMethod, которые сохраняются в поле itsCurrentMethod, я пришел к выводу, что условие lParenIndex != 0 || rParenIndex < 0 никогда не будет выполнено. Связано это с тем, что во всех случаях строка, которая сохраняется в поле itsCurrentMethod .type, начинается с символа открывающейся скобки '(' и всегда содержит один символ закрывающей скобки ')'.

Для упрощения понимания кода, я бы инициализировал переменную start просто единицей:

private int[] createInitialLocals() {
  int[] initialLocals = new int[itsMaxLocals];
  ....
  // No error checking should be necessary, sizeOfParameters does this
  String type = itsCurrentMethod.getType();
  int lParenIndex = type.indexOf('(');
  int rParenIndex = type.indexOf(')');
  if (lParenIndex != 0 || rParenIndex < 0) {
      throw new IllegalArgumentException("bad method type");
  }
  int start = 1; // <=
  StringBuilder paramType = new StringBuilder();
  while (start < rParenIndex) {
    switch (type.charAt(start)) {
    ....
    }
    ....
  }
  return initialLocals;  
}

Также, как вариант, можно добавить статическое приватное финальное булево поле DEBUG, и в комментарии над этим полем описать, для чего оно нужно. Тем более, в некоторых классах разработчики уже так делали, из-за чего анализатор PVS-Studio предупреждал о недостижимом коде (V6019). При разработке, для проверки значения type, поле DEBUG необходимо было бы выставить в true, а при релизе – в false:

// set the true value if you need to check 
// currentMethodType in the createInitialLocals method
private static final boolean DEBUG = false;
....
private int[] createInitialLocals() {
  int[] initialLocals = new int[itsMaxLocals];
  ....
  // No error checking should be necessary, sizeOfParameters does this
  String type = itsCurrentMethod.getType();
  int lParenIndex = type.indexOf('(');
  int rParenIndex = type.indexOf(')');
  
  if (DEBUG &&                             // <=
     (lParenIndex != 0 || rParenIndex < 0)
  ) {
    throw new IllegalArgumentException("bad method type");
  }

  int start = 1; 
  StringBuilder paramType = new StringBuilder();
  while (start < rParenIndex) {
    switch (type.charAt(start)) {
    ....
    }
    ....
  }
  return initialLocals;  
}

Вызывающий объект и аргумент в одном лице

V6100 An object is used as an argument to its own method. Consider checking the first actual argument of the 'setParentScope' method. Scope.java(123)

public class Scope extends Jump {
  ....
  public static Scope splitScope(Scope scope) {
    Scope result = new Scope(scope.getType());
    result.symbolTable = scope.symbolTable;
    scope.symbolTable = null;
    result.parent = scope.parent;
    result.setParentScope(scope.getParentScope());
    result.setParentScope(result);                   // <= 
    scope.parent = result;
    result.top = scope.top;
    return result;
  }
  ....
}

В методе splitScope анализатору показалось странным, что при вызове result.setParentScope(result) методу setParentScope в качестве аргумента передают тот же самый объект result. Это странно тем, что внутри тела метода экземпляра класса всегда и так имеется доступ к текущему экземпляру через this. Причём странности на этом не заканчиваются: главная заключается в том, что перед вызовом, на который указал анализатор, происходит вызов того же самого метода у того же самого объекта, однако, в этом случае аргументом является результат выражения scope.getParentScope(). Если бы метод setParentScope был обычным методом-сеттером для поля parentScope, то повторный вызов метода setParentScope просто перезаписывал бы предыдущее значение, что 100% было бы ошибкой. Однако всё не так просто. Давайте подробнее изучим метод setParentScope:

public void setParentScope(Scope parentScope) {
  this.parentScope = parentScope;
  this.top = parentScope == null ? (ScriptNode) this 
                                 : parentScope.top;
}

Этот метод, кроме сохранения переданного аргумента в поле parentScope, также имеет и побочный эффект. Заключается он в том, что если аргумент оказывается равен null, то в поле top сохраняется ссылка на текущий экземпляр объекта – this. Если же аргумент не равен null, то в поле top сохраняется ссылка на объект из поля top переданного аргумента.

Если отобразить эффект от вызовов методов setParentScope в методе splitScope когда вызов scope.getParentScope() вернёт null в виде таблицы, то получится такой результат:

Если же вызов scope.getParentScope() вернёт не null, то получится такой результат:

Не могу утверждать на 100%, что повторный вызов метода result.setParentScope(result) является ошибкой, но выглядит это очень странно. Тут нужно уже разработчикам изучить этот код и исправить его, если это необходимо.

Полезные ложно-позитивные срабатывания

V6106 Casting expression to short type before implicitly casting it to other type may be excessive or incorrect. ClassFileWriter.java(796)

public class ClassFileWriter {
  ....
  public void addInvoke(int theOpCode, 
                        String className, 
                        String methodName, 
                        String methodType) {
    if (DEBUGCODE) {
        System.out.println("Add "
                        + bytecodeStr(theOpCode)
                        + ", "
                        + className
                        + ", "
                        + methodName
                        + ", "
                        + methodType);
    }
    int parameterInfo = sizeOfParameters(methodType);
    int parameterCount = parameterInfo >>> 16;  // <= not commented
    int stackDiff = (short) parameterInfo;      // <= analyzer warning

    ....
  }
  ....
}

V6106 Casting expression to short type before implicitly casting it to other type may be excessive or incorrect. ClassFileWriter.java(847)

public class ClassFileWriter {
  ....
  public void addInvokeDynamic(String methodName, 
                               String methodType, 
                               MHandle bsm, 
                               Object... bsmArgs) {
    if (DEBUGCODE) {
      System.out.println("Add invokedynamic, " +
                         methodName +
                         ", " +
                         methodType);
    }
    // JDK 1.7 major class file version is required for invokedynamic
    if (MajorVersion < 51) {
        throw new RuntimeException(
          "Please build and run with JDK 1.7 for invokedynamic support");
    }

    int parameterInfo = sizeOfParameters(methodType);
    // int parameterCount = parameterInfo >>> 16;  // <= commented
    int stackDiff = (short) parameterInfo;         // <= analyzer warning
    ....
  }
  ....
}

Наконец, последняя пара предупреждений в нашем исследовании. Оба предупреждения, по моему мнению, являются ложными. Однако, просматривая их, я обнаружил настоящую ошибку. Заключается она в том, что код в начале методов addInvoke и addInvokeDynamic очень похож, а значит, скорее всего, один из этих методов создан при помощи копипаста другого. Однако в начале метода addInvokeDynamic закомментирована строка с побитовым сдвигом переменной parameterInfo, а в методе addInvoke идентичная строка не закомментирована. Скорее всего, в методе addInvokeDynamic забыли убрать комментарий в этой строке. Из-за этого, как мне кажется, поведение метода addInvokeDynamic будет некорректным, что может привести к ещё большим ошибкам при работе программы, которые потенциально могут быть не замечены.

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

Всё, что нужно, чтобы в будущем никто не столкнулся с некорректным поведением программы при вызове публичного метода addInvokeDynamic, так это раскомментировать эту строку:

public class ClassFileWriter {
  ....
  public void addInvokeDynamic(....) {
    ....
    
    int parameterInfo = sizeOfParameters(methodType);
    int parameterCount = parameterInfo >>> 16;    // <= comment removed
    int stackDiff = (short) parameterInfo;          
    ....
  }
  ....
}

Заключение

Как видите, даже в проекте, которому уже 25 лет, и который используется во многих других проектах и утилитах, всё ещё могут таиться ошибки. Чем дольше живёт ошибка, тем больше ресурсов потребуется на её исправление, поэтому лучше всего если она будет обнаружена ещё при написании кода, что позволит в разы сократить негативный эффект от неё. Помощником в этом нелегком деле является регулярное использование инструментов анализа кода (лучше не одного, а сразу нескольких) таких как, например, PVS-Studio.

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

P.S. В данный момент я продолжаю исследование проектов написанных на Java, которые обрабатывают JavaScript код, так что ждите ещё несколько статей про анализ проектов. В качестве альтернативы JavaScript движкам я также рассматриваю TypeScript Compiler API в качестве инструмента для написания статического анализатора для JavaScript. Связано это с тем, что TypeScript является надмножеством языка JavaScript. Также мне интересно будет узнать в комментариях к статье, какие диагностики для JavaScript вы хотели бы увидеть.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valery Komarov. The check of the Rhino JavaScript engine or how the unicorn met the rhino.