# АНБ, Ghidra и единороги На этот раз взгляд команды PVS\-Studio привлекла Ghidra \- большой и злой фреймворк для ревёрс\-инжиниринга, с помощью которого можно анализировать различные бинарные файлы и делать с ними всякие страшные вещи\. Наиболее интересно в нём даже не то, что он бесплатен для использования или отлично расширяется плагинами, а то, что написали его в АНБ и выложили исходники на GitHub для всех желающих\. С одной стороны, кажется, что у АНБ\-то точно достаточно ресурсов для поддержания кодовой базы в чистоте\. А с другой, не очень знакомые с ней новые контрибьюторы могли за прошедшее время случайно добавить незамеченных багов\. Поэтому, вооружившись статическим анализом, мы решили поискать слабые места в этом проекте\. ![0738_Ghidra_and_unicorns_ru/image1.png](https://import.viva64.com/docx/blog/0738_Ghidra_and_unicorns_ru/image1.png) ## Прелюдия Всего статический анализатор PVS\-Studio выдал 651 high, 904 medium и 909 low предупреждений в Java\-части проекта Ghidra \([релиз 9\.1\.2, коммит 687ce7f](https://github.com/NationalSecurityAgency/ghidra/releases/tag/Ghidra_9.1.2_build)\)\. Среди них около половины high и medium срабатываний были вызваны диагностикой "[V6022](https://pvs-studio.ru/ru/docs/warnings/v6022/) Parameter is not used inside method's body", которые обычно появляются после рефакторинга, когда какой\-то параметр оказался больше не нужен или некий функционал был временно отключен комментариями\. Беглый взгляд по этим предупреждениям \(очень уж их много, чтобы просматривать каждое в роли стороннего наблюдателя\) в данном проекте не выявил ничего явно подозрительного\. Вероятно, для данного проекта допустимо временно отключить эту диагностику в настройках анализатора, чтобы на неё не отвлекаться\. На практике часто можно встретить опечатки в названии параметра сеттера или конструктора и, вообще говоря, ей не следует пренебрегать\. Уверен, большинство читателей хотя бы однажды встречали подобный неприятный паттерн: ```cpp public class A { private String value; public A(String val) { // V6022 this.value = value; } public int hashCode() { return value.hashCode(); // NullPointerException } } ``` Более половины low\-предупреждений произвела диагностика "[V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'variable'" \- например, часто используется значение _File\.getParentFile\(\)_ без проверки на _null_\. В случае, если объект файла, на котором был вызван этот метод, был сконструирован без абсолютного пути, вернётся _null_ и отсутствие проверки может уронить приложение\. По традиции будем разбирать только предупреждения уровней high и medium, поскольку основная часть реальных ошибок содержится именно в них\. При работе с отчётами анализатора мы всегда рекомендуем разбирать предупреждения в порядке убывания их достоверности\. Далее рассмотрим несколько фрагментов, указанных анализатором, которые показались мне подозрительными или интересными\. Кодовая база у проекта оказалась внушительных размеров, и вручную такие места найти практически невозможно\. ## Фрагмент 1: сломанная валидация ```cpp private boolean parseDataTypeTextEntry() throws InvalidDataTypeException { ... try { newDataType = parser.parse(selectionField.getText(), getDataTypeRootForCurrentText()); } catch (CancelledException e) { return false; } if (newDataType != null) { if (maxSize >= 0 && newDataType.getLength() > newDataType.getLength()) { // <= throw new InvalidDataTypeException("data-type larger than " + maxSize + " bytes"); } selectionField.setSelectedValue(newDataType); return true; } return false; } ``` Предупреждение PVS\-Studio: [V6001](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 'newDataType\.getLength\(\)' to the left and to the right of the '\>' operator\. DataTypeSelectionEditor\.java:366 Данный класс предоставляет графический компонент для выбора типа данных с поддержкой автодополнения\. Разработчик, пользующийся этим компонентом, может задать максимально допустимый размер выбираемого типа данных \(через поле _maxSize_\) или сделать его неограниченным, задав отрицательное значение\. Предполагалось, что при валидации введённых данных превышение лимита бросает исключение, которое затем отлавливается выше по стеку вызовов и пользователю показывается соответствующее сообщение\. Похоже, что автора компонента отвлекли прямо во время написания этой проверки, или, может, он задумался над смыслом жизни, но в итоге валидация просто не выполняется, так как число никогда не может быть больше самого себя и, соответственно, получаем игнорирование этого условия\. А это значит, что данный компонент может предоставить невалидные данные\. Другая похожая ошибка нашлась в ещё двух классах: _GuidUtil_ и _NewGuid_\. ```cpp public class GuidUtil { ... public static GuidInfo parseLine(...) { ... long[] data = new long[4]; ... if (isOK(data)) { if (!hasVersion) { return new GuidInfo(guidString, name, guidType); } return new VersionedGuidInfo(guidString, version, name, guidType); } return null; } ... private static boolean isOK(long[] data) { for (int i = 0; i < data.length; i++) { if ((data[i] != 0) || (data[i] != 0xFFFFFFFFL)) { // <= return true; } } return false; } ... } ``` Предупреждение PVS\-Studio: [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'data\[i\] \!\= 0xFFFFFFFFL' is always true\. GuidUtil\.java:200 В цикле _for_ метода _isOK_ проверяется, что одно и то же значение не равно одновременно двум разным числам\. Если это так, то GUID сразу же признаётся валидным\. То есть, GUID будет признан невалидным только в случае, когда массив _data_ пуст, а это не происходит никогда, поскольку значение соответствующей переменной присваивается только один раз \- в начале метода _parseLine_\. Тело метода _isOK_ в обоих классах полностью совпадает, что наталкивает на мысль об очередной копипасте некорректного кода\. Что именно автор хотел проверить, я не уверен, но могу предположить, что этот метод следует исправить следующим образом: ```cpp private static boolean isOK(long[] data) { for (int i = 0; i < data.length; i++) { if ((data[i] == 0) || (data[i] == 0xFFFFFFFFL)) { return false; } } return true; } ``` ## Фрагмент 2: прячем исключения ```cpp public void putByte(long offsetInMemBlock, byte b) throws MemoryAccessException, IOException { long offsetInSubBlock = offsetInMemBlock - subBlockOffset; try { if (ioPending) { new MemoryAccessException("Cyclic Access"); // <= } ioPending = true; doPutByte(mappedAddress.addNoWrap(offsetInSubBlock / 8), (int) (offsetInSubBlock % 8), b); } catch (AddressOverflowException e) { new MemoryAccessException("No memory at address"); // <= } finally { ioPending = false; } } ``` Предупреждение PVS\-Studio: [V6006](https://pvs-studio.ru/ru/docs/warnings/v6006/) The object was created but it is not being used\. The 'throw' keyword could be missing: 'new MemoryAccessException\("Cyclic Access"\)'\. BitMappedSubMemoryBlock\.java:99 Сами по себе объекты исключений, как известно, ничего не делают \(или, по крайней мере, не должны ничего делать\)\. Практически всегда их новые экземпляры выбрасываются через _throw_, в отдельных редких случаях \- передаются куда\-либо или, может быть, помещаются в коллекцию\. Класс, содержащий данный метод, представляет из себя обёртку над блоком памяти, позволяющую считывать и записывать данные\. Здесь из\-за того, что исключения не выбрасываются, может нарушиться накладываемое ограничение доступа к текущему блоку памяти с помощью флага _ioPending_ и, кроме того, игнорируется _AddressOverflowException_\. Таким образом данные могут быть молча испорчены, а вместо явного указания на случившуюся ошибку в конкретном месте разработчик получит странные артефакты, которые придётся анализировать отладчиком\. Всего таких потерявшихся исключений нашлось восемь: * BitMappedSubMemoryBlock\.java: строки 77, 99, 106, 122 * ByteMappedSubMemoryBlock\.java: строки 52, 73, 92, 114 Характерно, что в этих же файлах есть крайне похожие методы, в которых _throw_ присутствует\. Скорее всего, изначально был написан один метод аналогично приведённому фрагменту, после чего его несколько раз скопипастили, каким\-то образом обнаружили ошибку и исправили её в тех местах, о которых смогли вспомнить\. ## Фрагмент 3: минное поле ```cpp private void processSelection(OptionsTreeNode selectedNode) { if (selectedNode == null) { setViewPanel(defaultPanel, selectedNode); // <= return; } ... } private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) { ... setHelpLocation(component, selectedNode); ... } private void setHelpLocation(JComponent component, OptionsTreeNode node) { Options options = node.getOptions(); ... } ``` Предупреждение PVS\-Studio: [V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Null dereference of 'selectedNode' in function 'setViewPanel'\. OptionsPanel\.java:266 Анализатор немного соврал \- на данный момент вызов метода _processSelection_ не приводит к _NullPointerException_, так как этот метод вызывается всего два раза, причём перед его вызовом _selectedNode_ явно проверяется на _null_\. Так делать не следует, поскольку другой разработчик может увидеть, что метод явно обрабатывает случай _selectedNode \=\= null_, и решить, что это валидное значение, что потом выльется в падение приложения\. Особо опасны такие неожиданности как раз в открытых проектах, так как в них участвуют люди, которые не досконально знают кодовую базу\. Вообще, должен сказать, что весь метод _processSelection_ выглядит достаточно странно\. Вполне вероятно, что это ошибка копипасты, так как в этом же методе далее ещё дважды встречается if\-блок с таким же телом, хотя и с другими условиями\. Однако, к этому моменту _selectedNode_ уже не будет _null_, и цепочка вызовов _setViewPanel\-setHelpLocation_ не приведёт к _NullPointerException_\. ## Фрагмент 4: автодополнение во зло ```cpp public static final int[] UNSUPPORTED_OPCODES_LIST = { ... }; public static final Set UNSUPPORTED_OPCODES = new HashSet<>(); static { for (int opcode : UNSUPPORTED_OPCODES) { UNSUPPORTED_OPCODES.add(opcode); } } ``` Предупреждение PVS\-Studio: [V6053](https://pvs-studio.ru/ru/docs/warnings/v6053/) The collection is modified while the iteration is in progress\. ConcurrentModificationException may occur\. DWARFExpressionOpCodes\.java:205 В данном случае анализатор опять немного наврал \- исключение не будет выброшено, поскольку коллекция _UNSUPPORTED\_OPCODES_ всегда пуста и цикл просто не выполнится\. К тому же, так совпало, что коллекция \- множество, и добавление уже присутствующего элемента её не изменит\. Скорее всего, автор вписал в _for\-each_ название коллекции через автодополнение и не заметил, что было предложено не то поле\. Модификация коллекции при итерировании по ней невозможна, но при удачных обстоятельствах, как и в данном случае, приложение может не упасть\. Здесь эта опечатка имеет косвенное влияние: автомат, разбирающий DWARF\-файлы, полагается на эту коллекцию для остановки анализа при нахождении неподдерживаемых опкодов\. Начиная с Java 9 для коллекций\-констант стоит использовать методы\-фабрики стандартной библиотеки: например, _Set\.of\(T\.\.\. elements\)_ не только сильно удобнее, но и сразу делает созданную коллекцию иммутабельной, что повышает надёжность кода\. ## Фрагмент 5: найдётся всё ```cpp public void setValueAt(Object aValue, int row, int column) { ... int index = indexOf(newName); if (index >= 0) { // <= Window window = tool.getActiveWindow(); Msg.showInfo(getClass(), window, "Duplicate Name", "Name already exists: " + newName); return; } ExternalPath path = paths.get(row); // <= ... } private int indexOf(String name) { for (int i = 0; i < paths.size(); i++) { ExternalPath path = paths.get(i); if (path.getName().equals(name)) { return i; } } return 0; } ``` Предупреждения PVS\-Studio: * [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'index \>\= 0' is always true\. ExternalNamesTableModel\.java:105 * [V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. ExternalNamesTableModel\.java:109 Автор задумался и в методе _indexOf_ вместо "индекса" \-1 для ненайденного значения возвращает 0 \- индекс первого элемента коллекции _paths_\. Даже если коллекция пустая\. Или, может быть, сгенерировал метод, но забыл поменять возвращаемое значение по умолчанию\. В итоге метод _setValueAt_ отбросит любое значение, переданное ему, и отобразит пользователю ошибку "Имя уже существует", даже если нет ни одного существующего имени\. Кстати, _indexOf_ больше нигде не используется, да и его значение нужно только для того, чтобы определить, существует ли искомый элемент\. Возможно, следовало вместо отдельного метода написать _for\-each_ прямо в _setValueAt_ и сделать _return_ на совпадающем элементе вместо игр с индексами\. Примечание: воспроизвести предположенную ошибку у меня не удалось\. Вероятно, метод _setValueAt_ больше не используется или вызывается только при определённых условиях\. ## Фрагмент 6: соблюдайте тишину ```cpp final static Map DELIMITER_NAME_MAP = new HashMap<>(20); // Any non-alphanumeric char can be used as a delimiter. static { DELIMITER_NAME_MAP.put(' ', "Space"); DELIMITER_NAME_MAP.put('~', "Tilde"); DELIMITER_NAME_MAP.put('`', "Back quote"); DELIMITER_NAME_MAP.put('@', "Exclamation point"); DELIMITER_NAME_MAP.put('@', "At sign"); DELIMITER_NAME_MAP.put('#', "Pound sign"); DELIMITER_NAME_MAP.put('$', "Dollar sign"); DELIMITER_NAME_MAP.put('%', "Percent sign"); ... } ``` Предупреждение PVS\-Studio: [V6033](https://pvs-studio.ru/ru/docs/warnings/v6033/) An item with the same key '@' has already been added\. FilterOptions\.java:45 Ghidra поддерживает фильтрацию данных в различном контексте: например, можно отфильтровать список файлов проекта по названию\. Кроме того, реализована фильтрация сразу по нескольким ключевым словам: '\.java,\.c' при включенном режиме 'OR' выведет все файлы, в названии которых есть либо '\.java', либо '\.c'\. Подразумевается, что в качестве разделителя слов можно использовать любой специальный символ \(конкретный разделитель выбирается в настройках фильтра\), но в реальности восклицательный знак оказался недоступен\. В подобных простынях инициализации очень просто опечататься, так как они часто пишутся с помощью копипасты, а при просмотре такого кода взгляд быстро замыливается\. А если опечатка оказалась не на двух соседних строках, то вручную её почти наверняка никто не увидит\. ## Фрагмент 7: остаток от деления всегда 0 ```cpp void setFactorys(FieldFactory[] fieldFactorys, DataFormatModel dataModel, int margin) { factorys = new FieldFactory[fieldFactorys.length]; int x = margin; int defaultGroupSizeSpace = 1; for (int i = 0; i < factorys.length; i++) { factorys[i] = fieldFactorys[i]; factorys[i].setStartX(x); x += factorys[i].getWidth(); // add in space between groups if (((i + 1) % defaultGroupSizeSpace) == 0) { // <= x += margin * dataModel.getUnitDelimiterSize(); } } width = x - margin * dataModel.getUnitDelimiterSize() + margin; layoutChanged(); } ``` Предупреждения PVS\-Studio: * [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression '\(\(i \+ 1\) % defaultGroupSizeSpace\) \=\= 0' is always true\. ByteViewerLayoutModel\.java:66 * [V6048](https://pvs-studio.ru/ru/docs/warnings/v6048/) This expression can be simplified\. Operand 'defaultGroupSizeSpace' in the operation equals 1\. ByteViewerLayoutModel\.java:66 Просмотрщик байтов в шестнадцатеричном режиме поддерживает выбор размера отображаемых групп: например, можно настроить вывод данных в формате 'ffff ffff' или 'ff ff ff ff'\. За расположение этих групп в пользовательском интерфейсе отвечает метод _setFactorys_\. Несмотря на то, что кастомизация и отображение работают правильно, цикл в этом методе выглядит крайне подозрительно: остаток от деления на единицу всегда равен нулю, а значит и координата _x_ будет увеличиваться на каждой итерации\. Подозрений добавляет и наличие свойства _groupSize_ у параметра _dataModel_\. Оставшийся мусор после рефакторинга? Или, может быть, потерялись вычисления переменной _defaultGroupSizeSpace_? В любом случае, попытка замены её значения на _dataModel\.getGroupSize\(\)_ сломала вёрстку, и однозначный ответ может дать, пожалуй, только автор этого кода\. ## Фрагмент 8: сломанная валидация, часть 2 ```cpp private String parseArrayDimensions(String datatype, List arrayDimensions) { String dataTypeName = datatype; boolean zeroLengthArray = false; while (dataTypeName.endsWith("]")) { if (zeroLengthArray) { // <= return null; // only last dimension may be 0 } int rBracketPos = dataTypeName.lastIndexOf(']'); int lBracketPos = dataTypeName.lastIndexOf('['); if (lBracketPos < 0) { return null; } int dimension; try { dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1, rBracketPos)); if (dimension < 0) { return null; // invalid dimension } } catch (NumberFormatException e) { return null; } dataTypeName = dataTypeName.substring(0, lBracketPos).trim(); arrayDimensions.add(dimension); } return dataTypeName; } ``` Предупреждение PVS\-Studio: [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'zeroLengthArray' is always false\. PdbDataTypeParser\.java:278 Данный метод парсит размерность многомерного массива и возвращает либо оставшийся после разбора текст, либо _null_ для невалидных данных\. Комментарий возле одной из проверок валидности гласит, что нулевым может быть только последний считанный размер\. Разбор идёт справа налево, поэтому подразумевается, что '\[0\]\[1\]\[2\]' \- валидный входной текст, а '\[2\]\[1\]\[0\]' \- нет\. Но, беда: проверку, что очередной размер равен нулю, в цикл никто не добавил, и парсер без лишних вопросов съедает невалидные данные\. Вероятно, следует исправить try\-блок следующим образом: ```cpp try { dimension = Integer.parseInt(dataTypeName.substring(lBracketPos + 1, rBracketPos)); if (dimension < 0) { return null; // invalid dimension } else if (dimension == 0) { zeroLengthArray = true; } } catch (NumberFormatException e) { return null; } ``` Естественно, существует возможность, что данный критерий валидности со временем оказался ненужным, или комментарий автора несёт в себе другой смысл и нужно проверять первую прочитанную размерность\. В любом случае, валидация данных \- критическая часть любого приложения, к которой надо относиться со всей ответственностью\. Ошибки в ней могут привести как к достаточно безобидным падениям приложения, так и дырам в безопасности, утечке данных, их порче или потере \(например, если пропустить SQL\-инъекцию при валидации запроса\)\. ## Немного об остальных предупреждениях Читатель может заметить, что предупреждений было выдано много, а рассмотрено мало\. Не очень аккуратно настроенный cloc насчитал в проекте около 1\.25 млн\. строк Java\-кода \(не пустых и не комментариев\)\. Дело в том, что почти все предупреждения крайне однотипные: здесь забыли проверить на _null_, там не удалили неиспользуемый легаси\-код\. Утомлять читателя перечислением одного и того же не очень хочется, а часть подобных случаев я упоминал в начале статьи\. Ещё, как пример, можно отметить полсотни предупреждений "[V6009](https://pvs-studio.ru/ru/docs/warnings/v6009/) Function receives an odd argument" в контексте неаккуратного использования метода _substring_ \(CParserUtils\.java:280, ComplexName\.java:48 и прочие\) для получения остатка строки после какого\-либо разделителя\. Разработчики часто надеются, что этот разделитель в строке будет присутствовать и забывают, что в ином случае _indexOf_ вернёт \-1, который является некорректным значением для _substring_\. Естественно, если данные были провалидированы или получены не "извне", то вероятность падения приложения значительно уменьшается\. Однако, в общем случае это потенциально опасные места, от которых мы хотим помочь избавиться\. ## Заключение В общем и целом, Ghidra порадовала качеством кода \- никаких особенных кошмаров в глаза не бросается\. Код неплохо отформатирован и имеет вполне консистентый стиль: переменным, методам и всему прочему в большинстве случаев даны понятные названия, встречаются разъяснительные комментарии, присутствует огромное количество тестов\. Не обошлось, естественно, и без проблем, среди которых: * Мёртвый код, который, скорее всего, остался после многочисленных рефакторингов; * Множество javadoc\-ов безнадёжно устарели и, например, указывают на несуществующие параметры; * Отсутствует возможность удобной разработки [при использовании IntelliJ IDEA](https://github.com/NationalSecurityAgency/ghidra/issues/450); * Модульная система, построенная вокруг рефлексии, делает навигацию по проекту и поиск зависимостей между компонентами в разы сложнее\. Пожалуйста, не пренебрегайте инструментами для разработчиков\. Статический анализ, как и ремни безопасности, не панацея, но он поможет предотвратить часть катастроф ещё до релиза\. А забагованным софтом пользоваться не нравится никому\. Про другие проверенные проекты можно почитать в [нашем блоге](https://pvs-studio.ru/ru/blog/posts/)\. А ещё у нас есть [триальная лицензия](https://pvs-studio.ru/ru/pvs-studio/download/) и различные [варианты](https://pvs-studio.ru/ru/blog/posts/0614/) использования анализатора без необходимости за него платить\.