Как уронить Minecraft своим модом

Разработка модификаций для игры Minecraft — очень интересное и приятное хобби. В этой статье мы посмотрим на ошибки в модификациях для нашей любимой игры на примере проекта Custom NPC+. Воспроизведём их в игре и уроним Minecraft.

Все, кто играл в Minecraft, знают про огромное комьюнити мододелов для этой величайшей игры. Кстати, автор статьи тоже заядлый моддер. Каких только воплощений этой игры я не видел:

· До жути индустриальную песочницу, где можно построить свой огромный завод;

· Шутер с крутыми анимациями оружия и проработанным геймплеем;

· RPG на уровне AAA проектов от всем известных игровых студий.

Одна из ключевых механик при создании своего RPG-проекта на основе Minecraft — это возможность добавления неигровых персонажей (НИП-ов), с которыми игрок сможет взаимодействовать. Проверяемый нами проект — как раз один из самых популярных модов, который может такое реализовать.

Приведу описание от автора проекта:

CustomNPC+ — это Minecraft мод, позволяющий добавлять пользовательских NPC в ваш мир. Он разработан для креативных игроков, которые хотят сделать свои миры Minecraft более уникальными.

Хочу подметить, что CustomNPC+ — это форк оригинала (CustomNPC), который добавляет контент из более новых версий мода в старую версию для Minecraft 1.7.10. Но помимо бэкпортинга, он также включает много собственных изменений. Если вы хотите ознакомиться с полным списком, можно обратиться к официальной страничке модификации на CurseForge.

Функциональные возможности мода:

· Позволяет создавать неигровых персонажей с любым скином, предметом в руках или любой моделью, которая существует в игре (даже если они добавлены другим модом).

· Присутствует удобная система создания заданий с разными условиями выполнения.

· Полностью рабочая диалоговая система между НИП-ом и игроком.

· Система фракций, позволяющая регулировать отношения между игроком и НИП-ами фракций.

· У каждого НИП-а может быть своя роль (Почтальон, Торговец, Транспортёр и т.д.).

· Помимо роли существует работа (Раздатчик предметов, Целитель, Страж и т.д.).

В общем мод добавляет множество механик для игры, предоставляя возможности для создания собственных серверов или карт для других игроков.

Я сам использую этот мод для нескольких своих проектов и пройти мимо просто не мог. Я проверил его на предмет ошибок статическим анализатором кода PVS-Studio и представлю вам статью с самыми интересными срабатываниями, которые я нашел в ходе составления pull request'а для авторов мода.

Неотъемлемый атрибут любой игры — это её локализация. В Minecraft способом локализации "из коробки" являются .lang файлы в ресурсах игры. Алгоритм до жути прост: вы записываете текст на нужном языке в .lang файл, а после в коде получаете его по индивидуальному имени. Может ли статический анализ найти ошибку при локализации? Я вам отвечу "да". И вот наглядный пример:

Файл GuiMailbox.java(76)

В моде есть отдельная профессия "Почтальон", а также блок почтового ящика. Когда игрок открывает почтовый ящик, в меню получения письма должно отображаться время его отправления. Программист реализовал правильный перевод с выбором hour/hours, а вот с minute/minutes он допустил ошибку, из-за чего перевод будет неверным. Пример из игры, как быть не должно:

Учитывая, что перевод для `mailbox.minute` уже присутствует в ресурсах игры, отправка коммита заняла больше времени, чем правка в код.

Срабатывание анализатора:

V6004 The 'then' statement is equivalent to the 'else' statement. GuiMailbox.java 76

Рассматривая срабатывания дальше, я обратил внимание на целочисленное деление в профессии лекаря. Мне показалось странным, что в методе `expand`, который принимает значения с точностью `double`, мы записываем значение с точностью `int`.

Файл JobHealer.java(41)

Срабатывание анализатора:

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-лекаря.

2. Красным выделена коллизия, реализованная сейчас (5 / 2).

3. Зелёным выделена коллизия без целочисленного деления (5 / 2.0).

Таким образом, для воспроизведения ошибки в переменной `range` должно быть нечётное число. А если учесть, что в версии этого мода под Minecraft 1.7.10 значение переменной `range` по умолчанию 5, ошибка буквально воспроизводится, если не трогать настройки радиуса лечения вокруг НИП-а.

Очень важно то, что в оригинальной версии модификации для Minecraft 1.12.2 эту ошибку исправили. К сожалению, я не смогу предоставить прямые доказательства, поэтому давайте представим, что я совершил запрос в космос, который мне вернул фрагмент кода:

То есть баг был исправлен в оригинальной модификации, и тут надо сделать то же самое.

В первую очередь перед показом самой ошибки стоит объяснить, что вообще происходит. Помимо работы с НИП-ами, модификация позволяет писать небольшие (а иногда и большие, в зависимости от фантазии автора) сценарии на таких языках как ECMAScript (подмножество JavaScript), Python, Ruby, Lua.

Для написания скриптов в игре существует внутренний редактор. Функционал, конечно же, далёк от современных IDE, статический анализ или автодополнение тяжело достижимы в рамках Майнкрафта. Но, как минимум, подсветка и вывод компилятора присутствуют :)

Перейдём к ошибке, связанной с этим меню:

Файл TextContainer.java(35)

Срабатывание анализатора:

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), поэтому первая часть выражения должна быть объединена в скобки:

В таком случае наше регулярное выражение будет проверять набор экранированных символов переноса:

· \n

· \r

· \r\n

Исходя из этого, я предлагаю исправление:

Тема разных схем переноса строки меня зацепила, и я провёл небольшой пентестинг, не связанный с ошибкой выше. Что если ввести символы возврата каретки, сохранив строку с ними в буфер обмена?

В первую очередь я решил проверить, как поведёт себя IntelliJ IDEA, и при вставке символов возврата каретки там добавились новые строки. После удачи с IDEA я запустил мод и попробовал вставить символы в интегрированную в Майнкрафт среду разработки. И всё упало)

Представляю тот самый злополучный набор символов: "\r\r\r\r\r\r\r test"

Честно говоря, как конкретно нужно исправить или сделать правильное поведение независимо от используемых символов переноса строки — вопрос открытый. Наверное, на текущий момент стоит поправить перестраховку от экранированных символов в `replaceAll` и учесть, что редактор имеет технический долг.

Представляю код, на который поругался анализатор:

Файл ScriptDBCPlayer.java(289)

Срабатывания анализатора:

· V6001 There are identical sub-expressions 'startIndex' to the left and to the right of the '==' operator. ScriptDBCPlayer.java 289

· V6007 Expression '!number' is always true. ScriptDBCPlayer.java 289

· V6033 An item with the same key 'startIndex' has already been changed. ScriptDBCPlayer.java 293

Сравнение переменной самой с собой — это уже очень странно. Но чтобы понять проблему, с которой мы сейчас столкнулись, придётся узнать дополнительную информацию:

· Аббревиатура DBC в названии класса означает, что он тесно связан с поддержкой мода JinGames Dragon Block C.

· Именование Script относит его к API и означает, что классом пользуются в механизме сценариев. Благо из-за прошлой ошибки мы уже знаем, что это.

· Код представлен в общем методе, конкретно приведённая часть отвечает за выставление бонусов к атрибутам игрока. Голову забивать этим не советую, далее в статье мы будем говорить только про атрибут "Сила".

В первую очередь я решил изучить весь класс и попробовать найти похожие места с такой же проверкой. Искать пришлось недолго:

Файл: ScriptDBCPlayer.java(224)

В обоих случаях разработчик парсит номер бонуса (`bonusID`), если удаётся, он получает бонус по его порядковой позиции в списке, если же не удаётся, то бонус ищется по названию.

Исходя из этого, в ошибочном коде мы должны были сравнивать `startIndex` и переменную, полученную в результате парсинга `bonusID`. Но прикол в том, что и "спаршенная" переменная называется `startIndex`, и счётчик цикла в ошибочном коде называется `startIndex`. То есть это одно и тоже, и получается, что ошибки нет :)

Если убрать шутки, блок `try` в ошибочном коде бесполезен, так как значение переменной `startIndex`перетирается в цикле. Мы можем попробовать воспроизвести ошибку, выдав два бонуса какому-либо атрибуту, и когда мы попытаемся поменять значение для второго бонуса, из-за неправильного сравнения выставится не второй бонус, а тот, который будет в списке первым.

Для того что бы это проверить, нам достаточно написать два небольших скрипта:

1. Скрипт, который будет создавать бонусные атрибуты персонажу и менять значения бонусов на 5 и 15 (ожидаем сумму атрибутов 20).

2. Скрипт, который будет выводить текущие состояния атрибутов в чат.

Оба скрипта вызываются при событии Interact, то есть просто по нажатию правой кнопкой мыши по НИП-у.

Скрипт 1:

Скрипт 2:

Не буду вас томить, действительно баг воспроизводится: при ожидаемом общем бонусе 20 мы получаем бонус 16.

P.S. Забавный метод-геттер, возвращающий `void`, из-за которого я час не понимал, почему у меня скрипт не выводил значение атрибута:

Представлю вам забавную ошибку в логике программы:

Файл Availability.java(273)

Разработчик перепутал операторы "и" и "или", по итогу правая часть выражения всегда будет `false`. Результат — код, выбрасывающий исключения, недостижим. Анализатор нашел аж 8 мест с той же ошибкой в этом классе. Очевидно, что ошибка "размножилась" из-за Copy-Paste программирования.

Исправление тривиальное — заменяем && на || и коммитим.

Срабатывание анализатора:

V6007 Expression 'i > 3' is always false. Availability.java 307

Я очень удивился, встретив эту ошибку в этом моде. Разработчики неправильно проверяют координаты:

Файл LinkedNpcController.java(123)

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'е на официальном GitHub репозитории проекта.

А вы хотите попробовать статический анализ? Вы всегда можете внедрить его в свой проект, используя инструмент PVS-Studio. Скачать триальную версию можно здесь. Для открытых проектов существует вариант бесплатного лицензирования.

#pvsstudio #codebugs #gamedev #java #staticanalysis #minecraft