Разработка модификаций для игры Minecraft — очень интересное и приятное хобби. В этой статье мы посмотрим на ошибки в модификациях для нашей любимой игры на примере проекта Custom NPC+. Воспроизведём их в игре и уроним Minecraft.
Все, кто играл в Minecraft, знают про огромное комьюнити мододелов для этой величайшей игры. Кстати, автор статьи тоже заядлый моддер. Каких только воплощений этой игры я не видел:
Одна из ключевых механик при создании своего 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(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 мы теряем по половине блока вверх и вниз по этой же координате. Чтобы наглядно продемонстрировать, как это выглядит, предлагаю вам скриншот из игры:
Таким образом, для воспроизведения ошибки в переменной 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, статический анализ или автодополнение тяжело достижимы в рамках Майнкрафта. Но, как минимум, подсветка и вывод компилятора присутствуют :)
Перейдём к ошибке, связанной с этим меню:
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:
Такой набор символов очень странный. Я думаю, что разработчик ожидал замену экранированных символов переноса (\r, \n, \r\n), поэтому первая часть выражения должна быть объединена в скобки:
(\\r)?\\n|\\rВ таком случае наше регулярное выражение будет проверять набор экранированных символов переноса:
Минутное изучение поисковых систем привело меня к пониманию, что:
Исходя из этого, я предлагаю исправление:
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;
}
}
....Срабатывания анализатора:
Сравнение переменной самой с собой — это уже очень странно. Но чтобы понять проблему, с которой мы сейчас столкнулись, придётся узнать дополнительную информацию:
В первую очередь я решил изучить весь класс и попробовать найти похожие места с такой же проверкой. Искать пришлось недолго:
Файл: 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перетирается в цикле. Мы можем попробовать воспроизвести ошибку, выдав два бонуса какому-либо атрибуту, и когда мы попытаемся поменять значение для второго бонуса, из-за неправильного сравнения выставится не второй бонус, а тот, который будет в списке первым.
Для того что бы это проверить, нам достаточно написать два небольших скрипта:
Оба скрипта вызываются при событии Interact, то есть просто по нажатию правой кнопкой мыши по НИП-у.
Скрипт 1:
Скрипт 2:
Не буду вас томить, действительно баг воспроизводится: при ожидаемом общем бонусе 20 мы получаем бонус 16.
P.S. Забавный метод-геттер, возвращающий void, из-за которого я час не понимал, почему у меня скрипт не выводил значение атрибута:
public void getBonusAttribute(String stat, String bonusID){
bonusAttribute("get",stat,bonusID,"*",1.0,true);
}Представлю вам забавную ошибку в логике программы:
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. Скачать триальную версию можно здесь. Для открытых проектов существует вариант бесплатного лицензирования.