Разработка модификаций для игры 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)

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 The 'then' statement is equivalent to the 'else' statement. GuiMailbox.java 76

Как потерять блок

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

Пояснение про метод expand

Метод expand(double x, double y, double z) из класса AxisAlignedBB расширяет размер коллизии. Параметры x, y, z отвечают за объем и направление, по которому мы должны увеличить размер коллизии.

AxisAlignedBB (Axis Aligned Bounding Box) — класс, представление коллизии в рамках Minecraft. Используется повсеместно как для получения сущностей из определённого пространства, так и для описания самой коллизии этих сущностей. Хранит в себе координаты левого нижнего и правого верхнего краёв прямоугольного параллелепипеда.

Файл JobHealer.java(41)

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 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 эту ошибку исправили. К сожалению, я не смогу предоставить прямые доказательства, поэтому давайте представим, что я совершил запрос в космос, который мне вернул фрагмент кода:

....
this.npc.world.getEntitiesWithinAABB(
    EntityLivingBase.class, 
    npc.getEntityBoundingBox().expand(range, range / 2.0, range)
);
....

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

Сложности с управляющими символами

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

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

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

Файл TextContainer.java(35)

public class TextContainer {
    public String text;
    ....

    public TextContainer(String text) {
        this.text = text;
        text.replaceAll("\\r?\\n|\\r", "\n");
        double l = 1.0D;
    }
    ....
}

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

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

(\\r)?\\n|\\r

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

• \n

• \r

• \r\n

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

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)

....
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 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)

....
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).

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

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

Скрипт 1:

Скрипт 2:

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

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

public void getBonusAttribute(String stat, String bonusID){
    bonusAttribute("get",stat,bonusID,"*",1.0,true);
}

Ошибки в API строят коварные козни

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

Файл Availability.java(273)

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 программирования.

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

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

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

Неверные координаты... и немножечко фантазии

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

Файл LinkedNpcController.java(123)

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<int[]> points = npc.ai.getMovingPath();

        NBTTagCompound compound = NBTTags.NBTMerge(readNpcData(npc), data.data);
                    
        npc.display.readToNBT(compound);
        ....
    }
}

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. Скачать триальную версию можно здесь. Для открытых проектов существует вариант бесплатного лицензирования.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Kirill Epifanov. How to crash Minecraft with your mod.