# Проверка JavaScript\-движка Rhino, или как встретились единорог с носорогом Имеется большое разнообразие языков программирования, но больше всего наши пользователи хотят, чтобы анализатор PVS\-Studio поддержал один из самых популярных языков – Javascript\. Одним из проектов, который можно использовать для написания статического анализатора JavaScript, является движок Rhino\. В данной статье мы проведём анализ исходного кода Rhino при помощи PVS\-Studio\. ![0990_RhinoEngine_ru/image1.png](https://import.viva64.com/docx/blog/0990_RhinoEngine_ru/image1.png) ## Предыстория Анализатор PVS\-Studio, постепенно развиваясь, получал поддержку новых языков программирования\. Первым дополнительным языком \(к изначальным С и С\+\+\) стал C\#, затем появился и Java\. Поэтому пользователи анализатора часто интересуются: не появится ли в будущем в PVS\-Studio нового языка, а если появится, то какой? Я один из тех разработчиков, которые в свободное время иногда изучают что\-то связанное с работой\. Одной из таких тем для меня является написание статического анализатора для языка JavaScript\. Однако началось всё с другого языка, а именно, Kotlin\. Однажды в свободное время я решил поизучать какой\-нибудь новый язык\. Так как я знаком с Java и C\#, то выбор пал на Kotlin, который, по моему мнению, сочетает в себе черты обоих этих языков\. В процессе изучения я узнал, что имеется возможность транспиляции Kotlin не только в Java, но и в JavaScript\. > Транспиляция – это преобразование программы, при которой используется исходный код программы, написанной на одном языке программирования в качестве исходных данных, и производится эквивалентный исходный код на другом языке программирования\\\. Эта возможность очень заинтересовала меня\. В результате я полез на GitHub изучать исходники транспилятора Kotlin в JavaScript\. Как оказалось, его разработчики не стали создавать свой парсер и классы для [AST \(абстрактного синтактического дерева\)](https://pvs-studio.ru/ru/blog/terms/0004/), а переиспользовали код из движка Rhino\. Так что следующим проектом для изучения оказался именно он\. Движок Rhino имеет несколько интересных особенностей: * Написан на Java; * Первая версия написана в 1997 году \(25 лет назад\), когда в Java даже не было дженериков \(они появились только в Java 5\.0 в 2004 году\); * Движок, кроме режима работы в качестве интерпретатора, может работать и в режиме компилятора\. "Как же возможно компилировать JavaScript?" – спросите вы\. Всё просто: код, написанный на JavaScript, преобразуется в Java байт код; * Работа Rhino в режиме компилятора часто оказывается производительнее C\-реализации JavaScript; * Rhino не имеет по умолчанию интеграции с браузерами; * Одно из применений Rhino – написание бизнес\-логики приложений на более простом языке JavaScript вместо Java\. Эта особенность позволяет писать бизнес\-логику даже специалистам, слабо разбирающимся в программировании\. Параллельно с изучением исходного кода [Rhino](https://github.com/mozilla/rhino) я скачал его исходники и запустил анализ проекта при помощи [плагина PVS\-Studio для IntelliJ IDEA](https://plugins.jetbrains.com/plugin/12263-pvs-studio-for-idea-and-android-studio)\. В результате анализа набралось немало предупреждений для проекта 25\-летней давности \(список по убыванию степени достоверности срабатываний\): * High: 79 * Medium: 158 * Low: 208 При беглом просмотре предупреждений мне показалось, что срабатывания некоторых диагностик оказались [ложными](https://pvs-studio.ru/ru/blog/terms/6461/)\. Вот наиболее яркие из них: * Все срабатывания диагностики [V6022](https://pvs-studio.ru/ru/docs/warnings/v6022/), по моему мнению, оказались ложными\. Связано это с изменением API некоторых методов и конструкторов\. [V6022](https://pvs-studio.ru/ru/docs/warnings/v6022/) ищет аргументы, которые не используются внутри тела методов или конструкторов\. То есть она срабатывает, когда аргументам ничего не присваивается, у них не вызываются никакие свойства/методы, аргументы не передаются ни в один метод/конструктор, не присваиваются никаким свойствам/полям; * Примерно четверть срабатываний правила [V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) \(эта диагностика ищет места, где может произойти доступ по нулевой ссылке\) низкого уровня достоверности \(Low\)\. Связано это с тем, что метод _XMLList\.getXML_ потенциально может вернуть _null_, если переданный индекс окажется отрицательным\. Однако этого никогда не произойдёт, потому что при вызове этого метода в качестве первого аргумента всегда передаётся массив из поля _\_annos_, а внутри метода _XMLList\.getXML_ вызывается метод _XMLList\.length,_ который так же связан с полем _\_annos_\. Поэтому метод никогда не вернёт отрицательное значение: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 't2Docked' to the left and to the right of the '&&' operator\. SwingGui\.java\(2718\), SwingGui\.java\(2718\) ```cpp 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_ используется дважды: ```cpp if (leftDocked && t2Docked && // <= rightDocked && t2Docked) { // <= // no change return; } ``` Учитывая, что в методе _check_ пристыкованность компонентов проверяется для левой и правой сторон у меня сложилось впечатление, что изначально у объекта _clistener_ было ещё одно поле _t1Docked_, которое со временем удалили\. В таком случае условие в _if_ выглядело более логичным, хотя и имело бы ошибку, которая произошла, скорее всего, из\-за опечатки, о которой бы также предупредил анализатор\. После исправления этой опечатки условие в _if_ стало бы таким: ```cpp leftDocked && t1Docked && rightDocked && t2Docked ``` Для исправления текущего кода я посоветовал бы удалить из условия одно из использований _t2Docked_\.Например, так: ```cpp t2Docked && leftDocked && rightDocked ``` ## Копипаст, невыполнимые условия и недостижимый код [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'power < 2' is always false\. UintMap\.java\(39\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'power < 2' is always false\. ObjToIntMap\.java\(102\) ```cpp 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_ идентичен, отличие только в имени аргумента\. В конце кода обоих конструкторов производится проверка: ```cpp if (check && power < 2) Kit.codeBug(); ``` Если данная проверка оказывается истинной, то вызывается метод _Kit\.codeBug,_ который просто кидает исключение: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression '\!BUG\_369394\_IS\_VALID' is always true\. XmlProcessor\.java\(288\) ```cpp class XmlProcessor implements Serializable { .... private void addTextNodesToRemoveAndTrim(List 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](https://bugzilla.mozilla.org/show_bug.cgi?id=369394)\. Учитывая, что переменной _BUG\_369394\_IS\_VALID_ присваивается значение _false_, то, скорее всего, этот баг уже не актуален\. Если это так, то неплохо бы убрать ветвление, зависящее от этой переменной\. Если же этот баг ещё актуален, тогда непонятно, почему значение у данной переменной _false_, а не _true_\. ## Когда pretty не такой уж pretty [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'i < indentLevel' is always false\. XmlProcessor\.java\(454\) ```cpp 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](https://www.ecma-international.org/wp-content/uploads/ECMA-357_2nd_edition_december_2005.pdf) \(страница 41\)\. Из документации следует, что в метод должен быть передан ещё один опциональный аргумент как раз с именем _indentLevel_\. Появление данной ошибки я могу объяснить либо тем, что изначально этот аргумент был, но затем его удалили, либо разработчик оказался невнимателен и забыл доработать этот метод таким образом, чтобы алгоритм соответствовал документации\. ## Потенциальное разыменование нулевой ссылки [V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'generatorState' in function 'freezeGenerator'\. Interpreter\.java\(1244\), Interpreter\.java\(3188\) ```cpp 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_ при клонировании: ```cpp CallFrame cloneFrozen() { .... copy.frozen = false; // <= return copy; } ``` **Второе**\. Вызов метода _freezeGenerator_ произойдёт только если _frame\.frozen_ будет ложно: ```cpp .... if (!frame.frozen) { return freezeGenerator( cx, frame, stackTop, generatorState, // <= op == Icode_YIELD_STAR); } .... ``` **Третье**\. Выше вызова этого метода имеется проверка на _null_ переменной _generatorState_: ```cpp if (generatorState == null && frame.frozen) Kit.codeBug(); ``` Если выражение в _if_ окажется истинным, то произойдёт исключение, потому что метод _Kit\.codeBug_ без всяких проверок выбрасывает исключение: ```cpp 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_: ```cpp if (throwable != null) { // <= .... } else { if (generatorState == null && frame.frozen) Kit.codeBug(); // <= } ``` **Пятое\.** Если посмотреть ещё выше, то обнаружится, что изначально значение переменной _generatorState_ равно _null_, а изменяется оно только если _throwable_ оказывается не равен _null _и является экземпляром класса _GeneratorState_: ```cpp .... 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_: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. Parser\.java\(3138\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6021/) The value is assigned to the 'localMax' variable but is not used\. NativeRegExp\.java\(568\) ```cpp 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_ результат выражения: ```cpp (char) (src[index++] & 0x1F) ``` сохраняется в переменную _localMax_\. Однако записанное значение не используется до записи нового значения '\\\\' в _localMax_\. После выхода из _switch_ значение _localMax_ используется в _if_ условии\. Поэтому вполне возможна ситуация, что одна из ветвей _if\.\.\.else_ не будет выполнятся тогда, когда это необходимо\. В этом же классе имеется метод _processCharSetImpl_, где используется почти полностью идентичный по структуре _switch_\. Однако в нём в ветви для символа 'c' никакой ошибки нет, потому что тело _else_ обёрнуто в фигурные скобки: ```cpp .... 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_: ```cpp case 'c': if ((index < end) && isControlLetter(src[index])) localMax = (char) (src[index++] & 0x1F); else { // <= --index; localMax = '\\'; } // <= break; .... ``` ## Забытое неиспользуемое значение [V6021](https://pvs-studio.ru/ru/docs/warnings/v6021/) The value is assigned to the 'nameString' variable but is not used\. QName\.java\(293\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6021/) обнаружила, что в переменную _nameString_ сохраняется результат выражения: ```cpp ((QName) name).localName() ``` Однако далее в обоих ветвлениях _if\.\.\.else_ значение _nameString_ перезаписывается прежде, чем используется: ```cpp if (name == Undefined.instance) { nameString = ""; } else { nameString = ScriptRuntime.toString(name); } ``` Скорее всего, данная ошибка произошла из\-за невнимательности программиста или в результате рефакторинга\. Предполагаю, что между сохранением значения в _nameString_ в начале метода и изменением его в _if\.\.\.else_ оно должно было быть использовано\. Также возможно, что в _if\.\.\.else_ забыли добавить ещё одну ветвь, где использовалось \(или как минимум не менялось\) значение _nameString_\. ## Потенциальная ошибка и зависимые аргументы [V6025](https://pvs-studio.ru/ru/docs/warnings/v6025/) Index 'sig1\.length \- 1' is out of bounds\. NativeJavaMethod\.java\(461\) [V6025](https://pvs-studio.ru/ru/docs/warnings/v6025/) Index 'sig2\.length \- 1' is out of bounds\. NativeJavaMethod\.java\(462\) ```cpp 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_ пустой: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6025/) Possibly index 'last \- 1' is out of bounds\. SuperBlock\.java\(50\) ```cpp 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_: ```cpp while (last >= 1 // <= && locals[last] == TypeInfo.TOP && !TypeInfo.isTwoWords(locals[last - 1])) { last--; } ``` Либо вместо оператора \>\= \(больше или равно\) использовать оператор \> \(больше\): ```cpp while (last > 0 // <= && locals[last] == TypeInfo.TOP && !TypeInfo.isTwoWords(locals[last - 1])) { last--; } ``` ## Взаимоуничтожающиеся операнды [V6028](https://pvs-studio.ru/ru/docs/warnings/v6028/) Identical expression 'end' to the left and to the right of compound assignment\. FastDtoaBuilder\.java\(74\) ```cpp 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_\. Если расписать более подробно операции в нём и упростить код, то станет понятнее, о чём предупреждает анализатор: ```cpp end = end + point – end => end = end – end + point => end = + point => end = point ``` Как видите, всё что в действительности произойдёт, так это присваивание переменной _end_ значения переменной _point_\. Скорее всего, ошибки в данном случае нет, а данное выражение осталось после рефакторинга\. Однако даже в таком случае я бы посоветовал заменить использование оператора _\+\=_ на обычное присваивание значения _point_ переменной _end_\. ## Закомментированный код [V6032](https://pvs-studio.ru/ru/docs/warnings/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\) ```cpp public class Node implements Iterable { .... 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](https://pvs-studio.ru/ru/docs/warnings/v6032/) обнаружены методы, выполняющие одно и то же\. Происходит это потому, что оба метода должны были быть доработаны в будущем\. Об этом свидетельствуют комментарии и закомментированные куски кода внутри этих функций\. Таким образом, анализатор помог обнаружить методы с закомментированным кодом, которые необходимо доработать\. Без подсказки анализатора программист мог бы и забыть доработать эти методы в будущем, ведь тут даже нет TODO\-комментария\. ## Как сложно жить Java\-программистам без директив компилятора [V6048](https://pvs-studio.ru/ru/docs/warnings/v6048/) This expression can be simplified\. Operand 'lParenIndex' in the operation equals 0\. ClassFileWriter\.java\(2658\) ```cpp 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_ просто единицей: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6019/)\)\. При разработке, для проверки значения _type_, поле _DEBUG_ необходимо было бы выставить в _true_, а при релизе – в _false_: ```cpp // 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](https://pvs-studio.ru/ru/docs/warnings/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\) ```cpp 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_: ```cpp 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 в виде таблицы, то получится такой результат: ![0990_RhinoEngine_ru/image2.png](https://import.viva64.com/docx/blog/0990_RhinoEngine_ru/image2.png) Если же вызов _scope\.getParentScope\(\)_ вернёт не null, то получится такой результат: ![0990_RhinoEngine_ru/image3.png](https://import.viva64.com/docx/blog/0990_RhinoEngine_ru/image3.png) Не могу утверждать на 100%, что повторный вызов метода _result\.setParentScope\(result\)_ является ошибкой, но выглядит это очень странно\. Тут нужно уже разработчикам изучить этот код и исправить его, если это необходимо\. ## Полезные ложно\-позитивные срабатывания [V6106](https://pvs-studio.ru/ru/docs/warnings/v6106/) Casting expression to short type before implicitly casting it to other type may be excessive or incorrect\. ClassFileWriter\.java\(796\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6106/) Casting expression to short type before implicitly casting it to other type may be excessive or incorrect\. ClassFileWriter\.java\(847\) ```cpp 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_, так это раскомментировать эту строку: ```cpp 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 и вы хотите опробовать его на своём проекте, то просто [скачайте дистрибутив](https://pvs-studio.ru/ru/pvs-studio/try-free/) и пользуйтесь анализатором свободно в течение пробного периода\. P\.S\. В данный момент я продолжаю исследование проектов написанных на Java, которые обрабатывают JavaScript код, так что ждите ещё несколько статей про анализ проектов\. В качестве альтернативы JavaScript движкам я также рассматриваю [TypeScript Compiler API](https://github.com/microsoft/TypeScript/wiki/Using-the-Compiler-API) в качестве инструмента для написания статического анализатора для JavaScript\. Связано это с тем, что TypeScript является надмножеством языка JavaScript\. Также мне интересно будет узнать в комментариях к статье, какие диагностики для JavaScript вы хотели бы увидеть\.