# Как уронить Minecraft своим модом
Разработка модификаций для игры Minecraft — очень интересное и приятное хобби\. В этой статье мы посмотрим на ошибки в модификациях для нашей любимой игры на примере проекта Custom NPC\+\. Воспроизведём их в игре и уроним Minecraft\.
## Введение
Все, кто играл в Minecraft, знают про огромное комьюнити мододелов для этой величайшей игры\. Кстати, автор статьи тоже заядлый моддер\. Каких только воплощений этой игры я не видел:
* До жути индустриальную песочницу, где можно построить свой огромный завод;
* Шутер с крутыми анимациями оружия и проработанным геймплеем;
* RPG на уровне AAA проектов от всем известных игровых студий\.
Одна из ключевых механик при создании своего RPG\-проекта на основе Minecraft — это возможность добавления неигровых персонажей \(НИП\-ов\), с которыми игрок сможет взаимодействовать\. Проверяемый нами проект — как раз один из самых популярных модов, который может такое реализовать\.
Приведу описание от автора проекта:
CustomNPC\+ — это Minecraft мод, позволяющий добавлять пользовательских NPC в ваш мир\. Он разработан для креативных игроков, которые хотят сделать свои миры Minecraft более уникальными\.
Хочу подметить, что CustomNPC\+ — это форк оригинала \(CustomNPC\), который добавляет контент из более новых версий мода в старую версию для Minecraft 1\.7\.10\. Но помимо бэкпортинга, он также включает много собственных изменений\. Если вы хотите ознакомиться с полным списком, можно обратиться к [официальной страничке](https://www.curseforge.com/minecraft/mc-mods/customnpc-plus) модификации на CurseForge\.
Функциональные возможности мода:
* Позволяет создавать неигровых персонажей с любым скином, предметом в руках или любой моделью, которая существует в игре \(даже если они добавлены другим модом\)\.
* Присутствует удобная система создания заданий с разными условиями выполнения\.
* Полностью рабочая диалоговая система между НИП\-ом и игроком\.
* Система фракций, позволяющая регулировать отношения между игроком и НИП\-ами фракций\.
* У каждого НИП\-а может быть своя роль \(Почтальон, Торговец, Транспортёр и т\.д\.\)\.
* Помимо роли существует работа \(Раздатчик предметов, Целитель, Страж и т\.д\.\)\.
В общем мод добавляет множество механик для игры, предоставляя возможности для создания собственных серверов или карт для других игроков\.
Я сам использую этот мод для нескольких своих проектов и пройти мимо просто не мог\. Я проверил его на предмет ошибок статическим анализатором кода PVS\-Studio и представлю вам статью с самыми интересными срабатываниями, которые я нашел в ходе составления pull request'а для авторов мода\.
## Проверяем проект
Важная \\\(почти\\\) информация
* Для проверки проекта мы будем использовать статический анализатор PVS\-Studio, разработчиком которого автор статьи и является\.
* Во время чтения вы встретите примеры кода\. Большинство из них сокращено, чтобы не перегружать читателя\. Пометкой сокращённого кода является многоточие "\.\.\.\."\.
* На момент проверки проекта последней ревизией был коммит [179c7b6](https://github.com/KAMKEEL/CustomNPC-Plus/commit/179c7b6fb4b08e27d0e58d5f9696fbe141cb5d10), её мы и будем проверять статическим анализатором\.
* Все исходники, которые проверялись, и все суждения, основанные на других исходных файлах, снабжены постоянной ссылкой, по которой их можно найти\.
* В статье приведены только те ошибки, что показались интересными именно автору \(да, вкусовщина\)\. Если кто\-то хочет посмотреть и на остальные, то всегда можно скачать анализатор и проверить проект самостоятельно\.
### Локализировали, локализировали да не вылокализировали
Неотъемлемый атрибут любой игры — это её локализация\. В Minecraft способом локализации "из коробки" являются \.lang файлы в ресурсах игры\. Алгоритм до жути прост: вы записываете текст на нужном языке в \.lang файл, а после в коде получаете его по индивидуальному имени\. Может ли статический анализ найти ошибку при локализации? Я вам отвечу "да"\. И вот наглядный пример:
Файл [GuiMailbox\.java\(76\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/client/gui/player/GuiMailbox.java#L76)
```cpp
private String getTimePast() {
....
if(selected.timePast > 3600000){
int hours = (int) (selected.timePast / 3600000);
if(hours == 1)
return hours + " " +
StatCollector.translateToLocal("mailbox.hour");
else
return hours + " " +
StatCollector.translateToLocal("mailbox.hours");
}
int minutes = (int) (selected.timePast / 60000);
if(minutes == 1)
return minutes + " " +
StatCollector.translateToLocal("mailbox.minutes");
else
return minutes + " " +
StatCollector.translateToLocal("mailbox.minutes");
}
```
В моде есть отдельная профессия "Почтальон", а также блок почтового ящика\. Когда игрок открывает почтовый ящик, в меню получения письма должно отображаться время его отправления\. Программист реализовал правильный перевод с выбором hour/hours, а вот с minute/minutes он допустил ошибку, из\-за чего перевод будет неверным\. Пример из игры, как быть не должно:
Учитывая, что перевод для `mailbox.minute` уже присутствует в ресурсах игры, отправка коммита заняла больше времени, чем правка в код\.
Срабатывание анализатора:
[V6004](https://pvs-studio.ru/ru/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. GuiMailbox\.java 76
### Как потерять блок
Рассматривая срабатывания дальше, я обратил внимание на целочисленное деление в профессии лекаря\. Мне показалось странным, что в методе `expand`, который принимает значения с точностью `double`, мы записываем значение с точностью `int`\.
Пояснение про метод expand
Метод `expand(double x, double y, double z) ` из класса `AxisAlignedBB` расширяет размер [коллизии](https://ru.wikipedia.org/wiki/%D0%9E%D0%B1%D0%BD%D0%B0%D1%80%D1%83%D0%B6%D0%B5%D0%BD%D0%B8%D0%B5_%D1%81%D1%82%D0%BE%D0%BB%D0%BA%D0%BD%D0%BE%D0%B2%D0%B5%D0%BD%D0%B8%D0%B9)\. Параметры x, y, z отвечают за объем и направление, по которому мы должны увеличить размер коллизии\.
_AxisAlignedBB_ \(Axis Aligned Bounding Box\) — класс, представление коллизии в рамках Minecraft\. Используется повсеместно как для получения сущностей из определённого пространства, так и для описания самой коллизии этих сущностей\. Хранит в себе координаты левого нижнего и правого верхнего краёв прямоугольного параллелепипеда\.
Файл [JobHealer\.java\(41\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/roles/JobHealer.java#L41)
```cpp
public boolean aiShouldExecute() {
healTicks++;
if (healTicks < speed * 10)
return false;
for (Object plObj : npc.worldObj.getEntitiesWithinAABB(
EntityLivingBase.class,
npc.boundingBox.expand(range, range/2, range))
) {
....
}
healTicks = 0;
return !toHeal.isEmpty();
}
```
Срабатывание анализатора:
[V6094](https://pvs-studio.ru/ru/docs/warnings/v6094/) The expression was implicitly cast from 'int' type to 'double' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. JobHealer\.java 41
В этом отрезке кода, с промежутком времени, указанным в поле `speed`, выполняется поиск всех сущностей в прямоугольнике с радиусом `range` вокруг НИП\-а лекаря\. Но из\-за целочисленного деления при расчете координаты Y мы теряем по половине блока вверх и вниз по этой же координате\. Чтобы наглядно продемонстрировать, как это выглядит, предлагаю вам скриншот из игры:
1. Белым выделена коллизия NPC\-лекаря\.
1. Красным выделена коллизия, реализованная сейчас \(5 / 2\)\.
1. Зелёным выделена коллизия без целочисленного деления \(5 / 2\.0\)\.
Таким образом, для воспроизведения ошибки в переменной `range` должно быть нечётное число\. А если учесть, что в версии этого мода под Minecraft 1\.7\.10 значение переменной `range` по умолчанию 5, ошибка буквально воспроизводится, если не трогать настройки радиуса лечения вокруг НИП\-а\.
Очень важно то, что в оригинальной версии модификации для Minecraft 1\.12\.2 эту ошибку исправили\. К сожалению, я не смогу предоставить прямые доказательства, поэтому давайте представим, что я совершил запрос в космос, который мне вернул фрагмент кода:
```cpp
....
this.npc.world.getEntitiesWithinAABB(
EntityLivingBase.class,
npc.getEntityBoundingBox().expand(range, range / 2.0, range)
);
....
```
То есть баг был исправлен в оригинальной модификации, и тут надо сделать то же самое\.
### Сложности с управляющими символами
В первую очередь перед показом самой ошибки стоит объяснить, что вообще происходит\. Помимо работы с НИП\-ами, модификация позволяет писать небольшие \(а иногда и большие, в зависимости от фантазии автора\) [сценарии](https://www.kodevelopment.nl/minecraft/customnpcs/scripting) на таких языках как ECMAScript \(подмножество JavaScript\), Python, Ruby, Lua\.
Для написания скриптов в игре существует внутренний редактор\. Функционал, конечно же, далёк от современных IDE, статический анализ или автодополнение тяжело достижимы в рамках Майнкрафта\. Но, как минимум, подсветка и вывод компилятора присутствуют :\)
Перейдём к ошибке, связанной с этим меню:
Файл [TextContainer\.java\(35\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/client/gui/util/TextContainer.java#L35)
```cpp
public class TextContainer {
public String text;
....
public TextContainer(String text) {
this.text = text;
text.replaceAll("\\r?\\n|\\r", "\n");
double l = 1.0D;
}
....
}
```
Срабатывание анализатора:
[V6010](https://pvs-studio.ru/ru/docs/warnings/v6010/) The return value of function 'replaceAll' is required to be utilized\. TextContainer\.java 35
Разработчик создал переменную, которая нигде не используется, а `replaceAll` выполняется в холостую\. Но самое важное, что и написанное регулярное выражение кажется мне тоже неверным\. Представляю наборы символов, которые заменит метод `replaceAll`:
* \\\\n
* \\r
* \\r\\n
Такой набор символов очень странный\. Я думаю, что разработчик ожидал замену экранированных символов переноса \(\\r, \\n, \\r\\n\), поэтому первая часть выражения должна быть объединена в скобки:
```cpp
(\\r)?\\n|\\r
```
В таком случае наше регулярное выражение будет проверять набор экранированных символов переноса:
* \\n
* \\r
* \\r\\n
Символы переноса
Минутное [изучение поисковых систем](https://stackoverflow.com/a/15433225) привело меня к пониманию, что:
* \\r\= CR \(возврат каретки\) — Используется как символ новой строки в macOS\.
* \\n\= LF \(перевод строки\) — Используется как символ новой строки в Unix/macOS\.
* \\r\\n\= CRLF — Используется как символ новой строки в Windows\.
Исходя из этого, я предлагаю исправление:
```cpp
public TextContainer(String text){
this.text = text.replaceAll("(\\r)?\\n|\\r", "\n");
}
```
Тема разных схем переноса строки меня зацепила, и я провёл небольшой пентестинг, не связанный с ошибкой выше\. Что если ввести символы возврата каретки, сохранив строку с ними в буфер обмена?
В первую очередь я решил проверить, как поведёт себя IntelliJ IDEA, и при вставке символов возврата каретки там добавились новые строки\. После удачи с IDEA я запустил мод и попробовал вставить символы в интегрированную в Майнкрафт среду разработки\. И всё упало\)
Представляю тот самый злополучный набор символов: "\\r\\r\\r\\r\\r\\r\\r test"
Честно говоря, как конкретно нужно исправить или сделать правильное поведение независимо от используемых символов переноса строки — вопрос открытый\. Наверное, на текущий момент стоит поправить перестраховку от экранированных символов в `replaceAll` и учесть, что редактор имеет технический долг\.
### Бонусная программа для Майнкрафт мода
Представляю код, на который поругался анализатор:
Файл [ScriptDBCPlayer\.java\(289\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L289)
```cpp
....
int startIndex = -1;
boolean number = false;
try {
startIndex = Integer.parseInt(bonusID);
number = true;
} catch (Exception var34) {
number = false;
}
for (startIndex = 0; startIndex < bonuses.length; ++startIndex) {
....
if (number && startIndex == startIndex || // <=
!number && bonusValues[startIndex][0].equals(bonusID)
) {
noNBTText = bonusValues[startIndex][0] + ";" + bonusValueString;
bonuses[startIndex] = "";
bonuses[startIndex] = noNBTText;
....
break;
}
}
....
```
Срабатывания анализатора:
* [V6001](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 'startIndex' to the left and to the right of the '\=\=' operator\. ScriptDBCPlayer\.java 289
* [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression '\!number' is always true\. ScriptDBCPlayer\.java 289
* [V6033](https://pvs-studio.ru/ru/docs/warnings/v6033/) An item with the same key 'startIndex' has already been changed\. ScriptDBCPlayer\.java 293
Сравнение переменной самой с собой — это уже очень странно\. Но чтобы понять проблему, с которой мы сейчас столкнулись, придётся узнать дополнительную информацию:
* Аббревиатура DBC в названии класса означает, что он тесно связан с поддержкой мода [JinGames Dragon Block C](https://jindbc.fandom.com/wiki/Getting_Started)\.
* Именование Script относит его к API и означает, что классом пользуются в механизме сценариев\. Благо из\-за прошлой ошибки мы уже знаем, что это\.
* Код представлен в общем методе, конкретно приведённая часть отвечает за выставление бонусов к атрибутам игрока\. Голову забивать этим не советую, далее в статье мы будем говорить только про атрибут "Сила"\.
В первую очередь я решил изучить весь класс и попробовать найти похожие места с такой же проверкой\. Искать пришлось недолго:
Файл: [ScriptDBCPlayer\.java\(224\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/scripted/entity/ScriptDBCPlayer.java#L224)
```cpp
....
int num = -1;
boolean number;
try {
num = Integer.parseInt(bonusID);
number = true;
} catch (Exception var33) {
number = false;
}
for (int i = 0; i < bonuses.length; ++i) {
....
if (number && i == num ||
!number && bonusValues[i][0].equals(bonusID)
) {
bonuses[i] = "";
break;
}
}
....
```
В обоих случаях разработчик парсит номер бонуса \(`bonusID`\), если удаётся, он получает бонус по его порядковой позиции в списке, если же не удаётся, то бонус ищется по названию\.
Исходя из этого, в ошибочном коде мы должны были сравнивать `startIndex` и переменную, полученную в результате парсинга `bonusID`\. Но прикол в том, что и "спаршенная" переменная называется `startIndex`, и счётчик цикла в ошибочном коде называется `startIndex`\. То есть это одно и тоже, и получается, что ошибки нет :\)
Если убрать шутки, блок `try` в ошибочном коде бесполезен, так как значение переменной `startIndex`перетирается в цикле\. Мы можем попробовать воспроизвести ошибку, выдав два бонуса какому\-либо атрибуту, и когда мы попытаемся поменять значение для второго бонуса, из\-за неправильного сравнения выставится не второй бонус, а тот, который будет в списке первым\.
Для того что бы это проверить, нам достаточно написать два небольших скрипта:
1. Скрипт, который будет создавать бонусные атрибуты персонажу и менять значения бонусов на 5 и 15 \(ожидаем сумму атрибутов 20\)\.
1. Скрипт, который будет выводить текущие состояния атрибутов в чат\.
Оба скрипта вызываются при событии Interact, то есть просто по нажатию правой кнопкой мыши по НИП\-у\.
Скрипт 1:
Скрипт 2:
Не буду вас томить, действительно баг воспроизводится: при ожидаемом общем бонусе 20 мы получаем бонус 16\.
P\.S\. Забавный метод\-геттер, возвращающий `void`, из\-за которого я час не понимал, почему у меня скрипт не выводил значение атрибута:
```cpp
public void getBonusAttribute(String stat, String bonusID){
bonusAttribute("get",stat,bonusID,"*",1.0,true);
}
```
### Ошибки в API строят коварные козни
Представлю вам забавную ошибку в логике программы:
Файл [Availability\.java\(273\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/controllers/data/Availability.java#L273)
```cpp
public int getDialog(int i) {
if (i < 0 && i > 3) {
throw new CustomNPCsException(
i + " isnt between 0 and 3", new Object[0]
);
} else if (i == 0) {
return this.dialogId;
} else if (i == 1) {
return this.dialog2Id;
} else {
return i == 2 ? this.dialog3Id : this.dialog4Id;
}
}
```
Разработчик перепутал операторы "и" и "или", по итогу правая часть выражения всегда будет `false`\. Результат — код, выбрасывающий исключения, недостижим\. Анализатор нашел аж 8 мест с той же ошибкой в этом классе\. Очевидно, что ошибка "размножилась" из\-за [Copy\-Paste программирования](https://pvs-studio.ru/ru/blog/terms/0068/)\.
Исправление тривиальное — заменяем && на \|\| и коммитим\.
Срабатывание анализатора:
[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'i \> 3' is always false\. Availability\.java 307
### Неверные координаты\.\.\. и немножечко фантазии
Я очень удивился, встретив эту ошибку в этом моде\. Разработчики неправильно проверяют координаты:
Файл [LinkedNpcController\.java\(123\)](https://github.com/KAMKEEL/CustomNPC-Plus/blob/f19797b0ac00ddee09653fcac19b91d94d22b3f8/src/main/java/noppes/npcs/controllers/LinkedNpcController.java#L123)
```cpp
public void loadNpcData(EntityNPCInterface npc) {
if (npc.linkedName.isEmpty())
return;
LinkedData data = getData(npc.linkedName);
if (data == null) {
npc.linkedLast = 0;
npc.linkedName = "";
npc.linkedData = null;
} else {
npc.linkedData = data;
if(npc.posX == 0 && npc.posY == 0 && npc.posX == 0)
return;
npc.linkedLast = data.time;
List points = npc.ai.getMovingPath();
NBTTagCompound compound = NBTTags.NBTMerge(readNpcData(npc), data.data);
npc.display.readToNBT(compound);
....
}
}
```
[V6001](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 'npc\.posX \=\= 0' to the left and to the right of the '&&' operator\. LinkedNpcController\.java 123
Этот метод используется для загрузки НИП\-а в мир из файлов игры\. Ошибка в том, что разработчик опечатался и вместо `posZ` снова сравнил `posX`\. Проверка нужна для уверенности в том, что НИП инициализирован\. В случае, если координаты по трём осям равны нулю, этот НИП невалидный\.
Я попробовал представить ситуацию, в которой координаты по оси X и Y будут равны 0, а по оси Z могли бы отличаться и хочу сказать, что ситуация, в которой эта опечатка могла бы сыграть какую\-либо роль, маловероятна\. Ниже нулевой координаты по высоте не могут стоять блоки, а значит НИП просто упадёт в бесконечную пропасть\. Кажется, что никто создавать НИП\-а на таких координатах не будет\.
Ошибку всё равно стоит исправить\. Как говорится: "Раз в год и палка стреляет"\.
## Итоги
Уже в третий раз мне удаётся помочь разработчикам открытого программного обеспечения в таком нелёгком деле, как поиск ошибок, и я этому очень рад\. Все описанные ошибки и не только собраны в [pull request'е](https://github.com/KAMKEEL/CustomNPC-Plus/pull/143) на официальном GitHub репозитории проекта\.
А вы хотите попробовать статический анализ? Вы всегда можете внедрить его в свой проект, используя инструмент PVS\-Studio\. Скачать триальную версию можно [здесь](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Для открытых проектов существует вариант [бесплатного лицензирования](https://pvs-studio.ru/ru/order/open-source-license/)\.