На этот раз внимание команды PVS-Studio привлекла Ghidra — большой плохой реверс-инжиниринговый фреймворк, позволяющий разработчикам анализировать бинарные файлы и делать с ними ужасные вещи. Самый примечательный факт заключается даже не в том, что он бесплатный и легко расширяемый с помощью плагинов, а в том, что он был разработан и загружен на GitHub для публичного доступа АНБ. С одной стороны, вы можете поспорить, что у АНБ достаточно ресурсов для поддержания чистоты своей кодовой базы. С другой стороны, новые участники, которые не очень хорошо знакомы с ним, могут случайно внести ошибки, которые могут остаться незамеченными. Итак, мы решили скормить проект нашему статическому анализатору и посмотреть, нет ли в нем проблем с кодом.
Увертюра
Всего PVS-Studio выдал 651 высокоуровневое, 904 среднеуровневое и 909 низкоуровневое предупреждение на Java-части Ghidra (релиз 9.1.2, коммит 687ce7f). Около половины высокоуровневых и среднеуровневых были предупреждениями «V6022 Параметр не используется внутри тела метода», которые обычно появляются после рефакторинга, когда какой-то параметр становится не нужным или какая-то фича временно закомментирована. Беглый просмотр этих предупреждений (их слишком много, чтобы внимательно изучать каждое в качестве стороннего рецензента) не выявил ничего откровенно подозрительного. Думаю, было бы нормально отключить эту диагностику для проекта в настройках анализатора хотя бы на время, чтобы она не отвлекала разработчиков. На практике, однако, вы не хотите пренебрегать этим, потому что параметры сеттеров или конструкторов часто имеют опечатки в своих именах. Я уверен, что большинство из вас хотя бы раз видели подобный раздражающий паттерн:
public class A {
private String value;
public A(String val) { // V6022
this.value = value;
}
public int hashCode() {
return value.hashCode(); // NullPointerException
}
}Более половины низкоуровневых предупреждений были выданы диагностикой V6008 Potential null разыменование переменной — например, значение, возвращаемое функцией File.getParentFile(), часто используется без предварительная проверка null. Если файловый объект, для которого вызывается метод, был создан без абсолютного пути, метод вернет null, что может привести к сбою приложения, если проверка отсутствует.
Как обычно, я буду обсуждать только предупреждения высокого и среднего уровня, поскольку они обычно выявляют большую часть реальных ошибок, обнаруженных в проекте. Когда вы имеете дело с аналитическими отчетами, мы рекомендуем всегда просматривать предупреждения в порядке убывания их достоверности.
Ниже я поделюсь несколькими фрагментами кода, сообщенными анализатором, которые показались мне подозрительными или интересными. Большой размер кодовой базы Ghidra делает поиск этих дефектов вручную практически невозможным.
Фрагмент 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 Слева и справа от оператора › одинаковые подвыражения newDataType.getLength(). 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 Выражение data[i] != 0xFFFFFFFFFL всегда верно. GuidUtil.java:200
Условие в цикле for метода в порядке проверяет, не равно ли значение двум разным значениям одновременно. Если это правда, 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 Объект создан, но не используется. Ключевое слово throw может отсутствовать: new MemoryAccessException («Циклический доступ)». 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 Нулевое разыменование selectedNode в функции setViewPanel. ПараметрыPanel.java:266
Анализатор не совсем прав насчет этого: вызов метода processSelection в настоящее время не приведет к NullPointerException, так как этот метод вызывается только дважды, с явным нулевая проверка selectedNode перед вызовом. Это нехорошо, потому что другой разработчик может предположить, что, поскольку метод явно обрабатывает случай 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 Коллекция модифицируется в процессе итерации. Может возникнуть исключение ConcurrentModificationException. 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 Выражение index ›= 0 всегда истинно. ВнешниеИменаТаблеМодель.java:105
- V6019 Обнаружен недостижимый код. Возможно, что ошибка присутствует. ВнешниеИменаТаблеМодель.java:109
Что-то отвлекло разработчика, и они случайно реализовали метод indexOf таким образом, что он возвращает 0, т.е. индекс первого элемента коллекции paths, вместо - 1 для несуществующего значения. Это произойдет, даже если коллекция пуста. Или, может быть, они сгенерировали метод, но забыли изменить возвращаемое значение по умолчанию. В любом случае метод setValueAt отклонит любое предложенное значение и покажет сообщение «Имя уже существует», даже если в коллекции нет ни одного имени.
Кстати, метод indexOf больше нигде не используется, и его значение фактически нужно только для того, чтобы определить, существует ли искомый элемент. Вместо того, чтобы писать отдельный метод и возиться с индексами, вероятно, было бы лучше написать цикл for-each прямо в методе setValueAt и возвращать его при встрече с соответствующий элемент.
Примечание: мне не удалось воспроизвести предполагаемую ошибку. Возможно, метод 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 Элемент с таким ключом @ уже добавлен. FilterOptions.java:45
Ghidra поддерживает фильтрацию данных в различных контекстах. Например, вы можете фильтровать файлы проекта по имени. Также поддерживается фильтрация по нескольким ключевым словам одновременно — указание «.java,.c» с включенным режимом «ИЛИ» отобразит все файлы, имена которых содержат либо «.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 ‘((i + 1) % defaultGroupSizeSpace) == 0’ всегда верно. ByteViewerLayoutModel.java:66
- V6048 Это выражение можно упростить. Операнд defaultGroupSizeSpace в операции равен 1. ByteViewerLayoutModel.java:66
Средство просмотра шестнадцатеричных байтов позволяет выбрать длину отображаемых групп: например, вы можете установить формат вывода данных как «ffff ffff» или «ff ff ff ff». Метод setFactorys отвечает за размещение этих групп в пользовательском интерфейсе. Хотя и настройка, и представление здесь в порядке, цикл в этом методе выглядит совсем не так: остаток от деления на единицу всегда равен 0, что означает, что координата x будет увеличиваться при каждом итерация. Тот факт, что параметр dataModel имеет свойство groupSize, делает его еще более подозрительным.
Это остатки рефакторинга? Или, может быть, некоторые вычисления переменной 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 Выражение zeroLengthArray всегда ложно. PdbDataTypeParser.java:278
Этот метод анализирует размеры многомерного массива и возвращает либо оставшийся текст, либо null для недопустимых данных. В комментарии рядом с одной из проверок достоверности говорится, что только последнее прочитанное измерение может быть равно 0. Анализ выполняется справа налево, что означает, что «[0][1][2]» является допустимым входным текстом, а «[2][]». 1][0]' не является.
Но, к сожалению, в цикле нет условия для проверки, равно ли считываемое измерение 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-кода (без учета пустых строк и комментариев). Просто почти все эти предупреждения выглядят очень похоже: некоторые относятся к отсутствующим нулевым проверкам, другие — к устаревшему коду, который больше не нужен, но все еще присутствует. Я не хочу утомлять вас, снова и снова показывая одни и те же типы ошибок, и я упомянул некоторые из них в начале.
Чтобы показать вам последний пример, давайте поговорим о полусотне предупреждений Функция «V6009 получает нечетный аргумент» в контексте небезопасного использования метода substring (CParserUtils.java:280, ComplexName. java:48 и т. д.), чтобы получить часть строки после разделителя. Разработчики часто надеются, что в строке присутствует разделитель, и забывают, что в противном случае indexOf вернет -1, что является неверным значением для substring. Конечно, если данные проверены или не поступают извне, шансов сбоя программы будет гораздо меньше. Но в целом эти дефекты потенциально небезопасны, и мы хотим помочь их устранить.
Вывод
Общее качество Ghidra довольно высокое — откровенных косяков нет, что круто. Код в основном хорошо отформатирован и стиль вполне выдержан: большинство переменных, методов и прочих сущностей имеют понятные названия, некоторые детали проясняются в комментариях, а количество тестов огромно.
Конечно, были некоторые проблемы, в том числе следующие:
- Фрагменты мертвого кода, которые должны быть остатками нескольких сеансов рефакторинга;
- Многие javadocs давно устарели и, например, указывают на несуществующие параметры;
- Удобная разработка с использованием IntelliJ IDEA не поддерживается;
- Модульная система, построенная на рефлексии, значительно усложняет навигацию по проекту и поиск зависимостей компонентов.
Пожалуйста, не пренебрегайте инструментами разработчика. Статический анализ, как и ремни безопасности, не является панацеей, но помогает предотвратить некоторые аварии до того, как их выпустят. В конце концов, никому не нравится использовать программное обеспечение, кишащее ошибками, не так ли?
Проверьте наш блог на наличие других сообщений о проектах, которые мы проверили. Также мы предлагаем пробную лицензию и несколько вариантов использования анализатора без оплаты.
