На этот раз взгляд команды PVS-Studio привлекла Ghidra — большой и злой фреймворк для реверс-инжиниринга, с помощью которого можно анализировать различные бинарные файлы и делать с ними всякие страшные вещи. Наиболее интересно в нём даже не то, что он бесплатен для использования или отлично расширяется плагинами, а то, что написали его в АНБ и выложили исходники на GitHub для всех желающих. С одной стороны, кажется, что у АНБ-то точно достаточно ресурсов для поддержания кодовой базы в чистоте. А с другой, не очень знакомые с ней новые контрибьюторы могли за прошедшее время случайно добавить незамеченных багов. Поэтому, вооружившись статическим анализом, мы решили поискать слабые места в этом проекте.
Прелюдия
Всего статический анализатор PVS-Studio выдал 651 high, 904 medium и 909 low предупреждений в Java-части проекта Ghidra (релиз 9.1.2, коммит 687ce7f). Среди них около половины high и medium срабатываний были вызваны диагностикой "V6022 Parameter is not used inside method's body", которые обычно появляются после рефакторинга, когда какой-то параметр оказался больше не нужен или некий функционал был временно отключен комментариями. Беглый взгляд по этим предупреждениям (очень уж их много, чтобы просматривать каждое в роли стороннего наблюдателя) в данном проекте не выявил ничего явно подозрительного. Вероятно, для данного проекта допустимо временно отключить эту диагностику в настройках анализатора, чтобы на неё не отвлекаться. На практике часто можно встретить опечатки в названии параметра сеттера или конструктора и, вообще говоря, ей не следует пренебрегать. Уверен, большинство читателей хотя бы однажды встречали подобный неприятный паттерн:
public class A {
private String value;
public A(String val) { // V6022
this.value = value;
}
public int hashCode() {
return value.hashCode(); // NullPointerException
}
}Более половины low-предупреждений произвела диагностика "V6008 Potential null dereference of 'variable'" — например, часто используется значение File.getParentFile() без проверки на null. В случае, если объект файла, на котором был вызван этот метод, был сконструирован без абсолютного пути, вернётся null и отсутствие проверки может уронить приложение.
По традиции будем разбирать только предупреждения уровней high и medium, поскольку основная часть реальных ошибок содержится именно в них. При работе с отчётами анализатора мы всегда рекомендуем разбирать предупреждения в порядке убывания их достоверности.
Далее рассмотрим несколько фрагментов, указанных анализатором, которые показались мне подозрительными или интересными. Кодовая база у проекта оказалась внушительных размеров, и вручную такие места найти практически невозможно.
Фрагмент 1: сломанная валидация
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 There are identical sub-expressions 'newDataType.getLength()' to the left and to the right of the '>' operator. DataTypeSelectionEditor.java:366
Данный класс предоставляет графический компонент для выбора типа данных с поддержкой автодополнения. Разработчик, пользующийся этим компонентом, может задать максимально допустимый размер выбираемого типа данных (через поле maxSize) или сделать его неограниченным, задав отрицательное значение. Предполагалось, что при валидации введённых данных превышение лимита бросает исключение, которое затем отлавливается выше по стеку вызовов и пользователю показывается соответствующее сообщение.
Похоже, что автора компонента отвлекли прямо во время написания этой проверки, или, может, он задумался над смыслом жизни, но в итоге валидация просто не выполняется, так как число никогда не может быть больше самого себя и, соответственно, получаем игнорирование этого условия. А это значит, что данный компонент может предоставить невалидные данные.
Другая похожая ошибка нашлась в ещё двух классах: GuidUtil и NewGuid.
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 Expression 'data[i] != 0xFFFFFFFFL' is always true. GuidUtil.java:200
В цикле for метода isOK проверяется, что одно и то же значение не равно одновременно двум разным числам. Если это так, то GUID сразу же признаётся валидным. То есть, GUID будет признан невалидным только в случае, когда массив data пуст, а это не происходит никогда, поскольку значение соответствующей переменной присваивается только один раз — в начале метода parseLine.
Тело метода isOK в обоих классах полностью совпадает, что наталкивает на мысль об очередной копипасте некорректного кода. Что именно автор хотел проверить, я не уверен, но могу предположить, что этот метод следует исправить следующим образом:
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: прячем исключения
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 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: минное поле
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 Null dereference of 'selectedNode' in function 'setViewPanel'. OptionsPanel.java:266
Анализатор немного соврал — на данный момент вызов метода processSelection не приводит к NullPointerException, так как этот метод вызывается всего два раза, причём перед его вызовом selectedNode явно проверяется на null. Так делать не следует, поскольку другой разработчик может увидеть, что метод явно обрабатывает случай selectedNode == null, и решить, что это валидное значение, что потом выльется в падение приложения. Особо опасны такие неожиданности как раз в открытых проектах, так как в них участвуют люди, которые не досконально знают кодовую базу.
Вообще, должен сказать, что весь метод processSelection выглядит достаточно странно. Вполне вероятно, что это ошибка копипасты, так как в этом же методе далее ещё дважды встречается if-блок с таким же телом, хотя и с другими условиями. Однако, к этому моменту selectedNode уже не будет null, и цепочка вызовов setViewPanel-setHelpLocation не приведёт к NullPointerException.
Фрагмент 4: автодополнение во зло
public static final int[] UNSUPPORTED_OPCODES_LIST = { ... };
public static final Set<Integer> UNSUPPORTED_OPCODES = new HashSet<>();
static {
for (int opcode : UNSUPPORTED_OPCODES) {
UNSUPPORTED_OPCODES.add(opcode);
}
}Предупреждение PVS-Studio: 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: найдётся всё
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 Expression 'index >= 0' is always true. ExternalNamesTableModel.java:105
- 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: соблюдайте тишину
final static Map<Character, String> 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 An item with the same key '@' has already been added. FilterOptions.java:45
Ghidra поддерживает фильтрацию данных в различном контексте: например, можно отфильтровать список файлов проекта по названию. Кроме того, реализована фильтрация сразу по нескольким ключевым словам: '.java,.c' при включенном режиме 'OR' выведет все файлы, в названии которых есть либо '.java', либо '.c'. Подразумевается, что в качестве разделителя слов можно использовать любой специальный символ (конкретный разделитель выбирается в настройках фильтра), но в реальности восклицательный знак оказался недоступен.
В подобных простынях инициализации очень просто опечататься, так как они часто пишутся с помощью копипасты, а при просмотре такого кода взгляд быстро замыливается. А если опечатка оказалась не на двух соседних строках, то вручную её почти наверняка никто не увидит.
Фрагмент 7: остаток от деления всегда 0
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 Expression '((i + 1) % defaultGroupSizeSpace) == 0' is always true. ByteViewerLayoutModel.java:66
- 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
private String parseArrayDimensions(String datatype,
List<Integer> 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 Expression 'zeroLengthArray' is always false. PdbDataTypeParser.java:278
Данный метод парсит размерность многомерного массива и возвращает либо оставшийся после разбора текст, либо null для невалидных данных. Комментарий возле одной из проверок валидности гласит, что нулевым может быть только последний считанный размер. Разбор идёт справа налево, поэтому подразумевается, что '[0][1][2]' — валидный входной текст, а '[2][1][0]' — нет.
Но, беда: проверку, что очередной размер равен нулю, в цикл никто не добавил, и парсер без лишних вопросов съедает невалидные данные. Вероятно, следует исправить try-блок следующим образом:
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 Function receives an odd argument" в контексте неаккуратного использования метода substring (CParserUtils.java:280, ComplexName.java:48 и прочие) для получения остатка строки после какого-либо разделителя. Разработчики часто надеются, что этот разделитель в строке будет присутствовать и забывают, что в ином случае indexOf вернёт -1, который является некорректным значением для substring. Естественно, если данные были провалидированы или получены не «извне», то вероятность падения приложения значительно уменьшается. Однако, в общем случае это потенциально опасные места, от которых мы хотим помочь избавиться.
Заключение
В общем и целом, Ghidra порадовала качеством кода — никаких особенных кошмаров в глаза не бросается. Код неплохо отформатирован и имеет вполне консистентый стиль: переменным, методам и всему прочему в большинстве случаев даны понятные названия, встречаются разъяснительные комментарии, присутствует огромное количество тестов.
Не обошлось, естественно, и без проблем, среди которых:
- Мёртвый код, который, скорее всего, остался после многочисленных рефакторингов;
- Множество javadoc-ов безнадёжно устарели и, например, указывают на несуществующие параметры;
- Отсутствует возможность удобной разработки при использовании IntelliJ IDEA;
- Модульная система, построенная вокруг рефлексии, делает навигацию по проекту и поиск зависимостей между компонентами в разы сложнее.
Пожалуйста, не пренебрегайте инструментами для разработчиков. Статический анализ, как и ремни безопасности, не панацея, но он поможет предотвратить часть катастроф ещё до релиза. А забагованным софтом пользоваться не нравится никому.
Про другие проверенные проекты можно почитать в нашем блоге. А ещё у нас есть триальная лицензия и различные варианты использования анализатора без необходимости за него платить.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lazeba. NSA, Ghidra, and Unicorns.