Нашёл ошибки в моде Create и проверил их в игре

• Введение
• Проверка проекта
  • Ctrl + V не работает
  • Ненужный максимум
  • Не обрабатываем ошибку
  • Падающее логирование
  • Теория о двух состояниях
  • Важно соблюдать контракты методов
  • Прерывание
• Заключение

В этой статье мы разберём ошибки в одном из самых популярных модов для Minecraft — Create. Посмотрим, как они проявляются прямо в игре, и отправим пулл-реквесты с исправлениями.

Введение

Create — это модификация для Minecraft, которая добавляет валы, шестерни и прочую механику. Из них собираются машины, работающие на вращении, от простой мельницы до полностью автоматизированного завода по производству тортиков.

Сам по себе этот мод очень популярен: только на CurseForge у него больше 206 миллионов скачиваний. И это без учёта Modrinth и того, что мод до сих пор портируется на новые версии игры. Я уверен, что почти каждый зашедший в эту статью читать тоже наверняка играл в этот мод.

Проверка проекта

Ctrl + V не работает

Файл SchematicEditScreen.java(130)

Заглянем в меню, отвечающее за вставку схематика.

public boolean keyPressed(int code, ....) {
  if (isPaste(code)) {
    String coords = minecraft.keyboardHandler.getClipboard();
    if (coords != null && !coords.isEmpty()) {
      coords.replaceAll(" ", "");             // <=
      String[] split = coords.split(",");
      if (split.length == 3) {
        boolean valid = true;
        for (String s : split) {
          try {
            Integer.parseInt(s);
          } catch (NumberFormatException e) {
            valid = false;
          }
        }
        if (valid) {
          xInput.setValue(split[0]);
          yInput.setValue(split[1]);
          zInput.setValue(split[2]);
          return true;
        }
      }
    }
  }
  ....
}

Предупреждение PVS-Studio:

V6010 The return value of function 'replaceAll' is required to be utilized. SchematicEditScreen.java 130

Задумка следующая: игрок скопировал координаты, нажал Ctrl + V в специальном окне, и поля в нём заполнились сами.

Но вызов replaceAll в строке, помеченной // <=, бесполезен. Строки в Java неизменяемы, поэтому метод не правит исходную строку, а возвращает новую. Она тут же теряется, и пробелы остаются на месте.

Дальше строка режется по запятой, и во втором и третьем элементах остаётся пробел. И Integer.parseInt(" 67") бросает NumberFormatException. В итоге поля остаются пустыми.

Открыв меню, я увидел ожидаемое поведение:

• -156, 67, 204 — не работает;
• -156,67,204 — работает.

Исправление умещается в одну строку: нужно добавить присваивание перед операцией replaceAll:

coords = coords.replaceAll(" ", "");

Ненужный максимум

Файл ScheduleScreen.java(557)

for (List<ScheduleWaitCondition> list : entry.conditions) {
  int maxWidth = getConditionColumnWidth(list);
  for (int i = 0; i < list.size(); i++) {
    ScheduleWaitCondition scheduleWaitCondition = list.get(i);
    Math.max(maxWidth, renderInput(....));   // <=
    scheduleWaitCondition.renderSpecialIcon(....);
  }
  AllGuiTextures.SCHEDULE_CONDITION_APPEND.render(
    graphics, 
    xOffset + (maxWidth - 10) / 2,
    29 + list.size() * 18
  );
  xOffset += maxWidth + 10;
}

Предупреждение PVS-Studio:

V6010 The return value of function 'max' is required to be utilized. ScheduleScreen.java 557

Суть ошибки хорошо объясняет само сообщение анализатора — вызов Math.max(....) никуда не записывается.

Я полез в историю, ожидая увидеть след рефакторинга: подсчёт ширины вынесли в отдельный метод, присваивание результата в maxWidth убрали, а вызов Math#max оставили. Но ничего подобного. Файл появился 1 февраля 2022 года одним коммитом на 941 строку, и Math#max без присваивания был в нём с первой же версии. Изначальная инициализация maxWidth через вызов getConditionColumnWidth() там уже тоже была.

А что изменилось бы, допиши автор maxWidth = Math.max(....)? Ничего. Ширина колонки, в которой будет располагаться текст (на фото ниже), считается заранее и с запасом: getConditionColumnWidth() проходит по всей колонке и берёт максимум, так что ни одна строка за неё не вылезет. Я проверил с помощью отладки: во всех случаях, которые мне удалось протестировать, возвращалась одна и та же ширина. Получается, в коде ищется максимум из двух одинаковых чисел.

Строка не сломалась со временем и не была затронута переделкой. Она не делала ничего с самого первого дня — на протяжении четырёх лет и нескольких портов на новые версии Minecraft подряд.

Предлагаю всё же убрать лишнюю обёртку и оставить renderInput в одиночестве.

Не обрабатываем ошибку

Файл MechanicalCrafterBlockEntity.java(535)

В первую очередь хочу показать, как выглядит механический крафтер:

Давайте посмотрим один метод из его логики:

protected void continueIfAllPrecedingFinished() {
  List<MechanicalCrafterBlockEntity> preceding = 
    RecipeGridHandler.getPrecedingCrafters(this);

  if (preceding == null) { // <=
    ejectWholeGrid();
    return;
  }

  for (MechanicalCrafterBlockEntity blockEntity : preceding)
    if (blockEntity.phase != Phase.WAITING)
      return;
  
  phase = Phase.ASSEMBLING;
  countDown = 1;
}

Предупреждение PVS-Studio:

V6007 Expression 'preceding == null' is always false. New returns not-null reference. MechanicalCrafterBlockEntity.java 535

Метод getPrecedingCrafters возвращает крафтеры, стоящие выше по цепочке, то есть те, что отдают предметы в текущий. Вернуть null он не может. Список создаётся в самом начале, и оба выхода из метода возвращают именно его. Даже если блок не оказался крафтером, наружу уйдёт просто пустой список:

public static List<MechanicalCrafterBlockEntity> getPrecedingCrafters(
  MechanicalCrafterBlockEntity crafter
) {
  BlockPos pos = crafter.getBlockPos();
  Level world = crafter.getLevel();
  List<MechanicalCrafterBlockEntity> crafters = new ArrayList<>();
  BlockState blockState = crafter.getBlockState();

  if (!isCrafter(blockState))
    return crafters;

  ....
  return crafters;
}

Получается, ветка с ejectWholeGrid() недостижима. Список preceding никогда не может быть null, но пустым бывает регулярно. А что если заменить проверку на isEmpty() и проверить в игре? Давайте посмотрим.

До:

После правки с isEmpty:

На видео видно, что до правки предметы при попадании в цикл перебрасывались из одного крафтера в другой бесконечно. Потенциально это могло кому-то насолить и сломать чей-то завод :)

А вот после замены на isEmpty() зацикливанию даже не дали образоваться. Метод увидел, что ожидать предметов больше неоткуда, и выбросил предметы.

Но стоило добавить к ним ещё два крафтера, и правка перестала работать:

Здесь возможны два варианта. Либо поиск зацикливаний в механическом крафтере должен работать как-то иначе — тогда простая правка с заменой на isEmpty() недостаточна. Либо это и ситуация с зацикливанием багом не является. В таком случае проверку на null можно просто удалить.

Падающее логирование

Файл TrainRelocationPacket.java(52)

Ниже представлен обработчик клиент-серверного пакета перемещения поезда:

public void handle(ServerPlayer sender) {
  Train train = Create.RAILWAYS.trains.get(trainId);
  Entity entity = sender.level().getEntity(entityId);

  String messagePrefix = sender.getName()
      .getString() + " could not relocate Train ";

  if (
    train == null || // <=
    !(entity instanceof CarriageContraptionEntity cce)
  ) {
    Create.LOGGER.warn(messagePrefix + train.id.toString()
        .substring(0, 5) + ": not present on server");
    return;
  }

  if (!train.id.equals(cce.trainId))
    return;

  ....
}

Предупреждение PVS-Studio:

V6008 Potential null dereference of 'train'. TrainRelocationPacket.java 52

Условие срабатывает в двух случаях: поезд не найден, либо сущность оказалась не того класса. Со вторым всё в порядке, в лог уйдёт аккуратное предупреждение. А вот в первом код обращается к train.id ровно тогда, когда train равен null.

Самое занятное, что сообщение, которое мы пытаемся записать, заканчивается словами not present on server. Ветка написана специально для отсутствующего поезда и об него же и спотыкается.

Пакет приходит с клиента, а значит идентификатор поезда выбирает клиент. Подменённая сборка самого клиента может слать запросы на перенос несуществующих составов и заставлять сервер сыпать себе stacktrace-ы в лог.

Сервер не упадёт: Create регистрирует пакеты так, что они выполняются безопасно, и даже если появится исключение, оно будет проглочено в лог, и пакет просто не обработается.

Чинится тем, что идентификатор и так лежит в самом пакете:

messagePrefix + trainId.toString().substring(0, 5)

Теория о двух состояниях

Файл MechanicalMixerBlockEntity.java(238)

protected List<Recipe<?>> getMatchingRecipes() {
  List<Recipe<?>> matchingRecipes = super.getMatchingRecipes();  
  if (!AllConfigs.server().recipes.allowBrewingInMixer.get())
    return matchingRecipes;

  Optional<BasinBlockEntity> basin = getBasin();
  if (!basin.isPresent())
    return matchingRecipes;

  BasinBlockEntity basinBlockEntity = basin.get();
  if (basin.isEmpty()) // <= 
    return matchingRecipes;

  IItemHandler availableItems = level.getCapability(
  Capabilities.ItemHandler.BLOCK, 
  basinBlockEntity.getBlockPos(), null
  );

  if (availableItems == null)
    return matchingRecipes;

  for (int i = 0; i < availableItems.getSlots(); i++) {
    ....
  }
  
  return matchingRecipes;
}

Предупреждение PVS-Studio:

V6007 Expression 'basin.isEmpty()' is always true. MechanicalMixerBlockEntity.java 238

В этом месте basin.isEmpty() никогда не вернёт true, return в этой ветке недостижим. Скорее всего, разработчик хотел проверить basinBlockEntity, а не сам Optional.

На геймплей игрока это никак не повлияет. Ниже по коду есть проверки, работающие с этим же блоком, и если "чаша" окажется пустой, то либо не запустится цикл, либо availableItems будет null.

Но мёртвый if опаснее, чем кажется. Это подстраховка, которая на самом деле не страхует ни от чего. Стоит кому-то переписать проверки ниже, и здесь останется дыра, которую никто не станет искать, ведь проверка вроде бы на месте.

Важно соблюдать контракты методов

Файл Contraption.java(230)

public static Contraption fromNBT(Level world, CompoundTag nbt, 
                  boolean spawnData) {
  String type = nbt.getString("Type");
  Contraption contraption = ContraptionType.fromType(type);
  contraption.readNBT(world, nbt, spawnData);
  ....
}

Увидели ошибку? А если заглянем внутрь ContraptionType#fromType?

/**
 * Lookup the ContraptionType with the given ID, 
 * and create a new Contraption from it if present.
 * If it doesn't exist, returns null.
 */
@Nullable
public static Contraption fromType(String typeId) {
  ContraptionType legacy = AllContraptionTypes.BY_LEGACY_NAME
                        .get(typeId);
  if (legacy != null) {
    return legacy.factory.get();
  }
  ResourceLocation id = ResourceLocation.tryParse(typeId);
  ContraptionType type = CreateBuiltInRegistries.CONTRAPTION_TYPE.get(id);
  return type == null ? null : type.factory.get();
}

Теперь понятно. Метод честно предупреждает о null дважды: аннотацией и комментарием. А вызывающий код не проверяет ничего.

Предупреждение PVS-Studio:

V6008 Potential null dereference of 'contraption'. Contraption.java 230

Причём сломать это проще, чем кажется. Первая же строка берёт nbt.getString("Type"), а если ключа нет, вернётся пустая строка. Такой тип в реестре тоже не найдётся, и наружу снова уйдёт null.

Убедиться можно так. Выдаём себе вагонетку с несуществующим типом штуковины:

/give @p
create:minecart_contraption[create:minecart_contraption_data=
{Type:"create:does_not_exist",InitialOrientation:"north"}]

Сама команда ничего не сломает, предмет спокойно ляжет в инвентарь. А вот когда мы поставим вагонетку на рельсы, MinecartContraptionItem полезет собирать штуковину из этих данных, и там нас встретит stacktrace с NullPointerException. В итоге вместо штуковины игрока, поставится обычная вагонетка из Minecraft.

Проверку добавить несложно:

Contraption contraption = ContraptionType.fromType(type);
if (contraption == null) {
  Create.LOGGER.warn("Unknown contraption type: {}", type);
  return null;
}

Но тогда null придётся корректно обработать в обоих вызывающих местах, а это уже вопрос о том, что делать с вагонеткой, содержимое которой мод больше не понимает. Оставить ли пустую вагонетку или просто не грузить сущность. Думаю, стоит спросить у разработчиков мода, они явно лучше знают, как поступить.

Прерывание

Файл ServerLagger.java(14)

public void tick() {
  if (!isLagging || tickTime <= 0)
    return;

  try {
    Thread.sleep(tickTime);
  } catch (InterruptedException e) {
    e.printStackTrace();
  }
}

Предупреждение PVS-Studio:

V6103 Ignored InterruptedException could lead to delayed thread shutdown. ServerLagger.java 14

ServerLagger — это вспомогательный класс для отладочной команды killtps start <tickTime>. Эта команда заставляет сервер спать указанное число миллисекунд на каждом тике, чтобы посмотреть, как мод себя ведёт на просевшем TPS. Доступна она только в debug-сборке и только оператору (так называется тип администратора сервера), так что паниковать не о чем. Но код внутри всё равно неправильный, и объяснить причину будет полезно.

Когда поток прерывают, sleep немедленно бросает InterruptedException и заодно сбрасывает флаг прерывания. То есть после catch внешне ничего не осталось: флага нет, исключения нет, есть только строчка в System.err. Метод спокойно возвращает управление, и сервер продолжает работать, как будто его никто не звал.

Прерывание — это просьба остановиться, адресованная всему потоку, а не только тому месту, где оно поймано. Перехватив его и ничего не сделав, вы забираете эту просьбу себе и выбрасываете. Об этом как раз указано в Javadoc. Достаточно одной строки, чтобы передать прерывание дальше:

} catch (InterruptedException e) {
  Thread.currentThread().interrupt();
}

Заключение

На этом на сегодня всё. Все ошибки я собрал как в статье, так и в пулл-реквестах для разработчиков этого проекта (тык, тык).

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