I found some bugs in Create mod and checked them in Minecraft

• Introduction
• Let's check the project
  • Ctrl + V doesn't work
  • Unnecessary maximum
  • Ignore error handling
  • The logging crash
  • The two-state theory
  • Method contracts must be followed
  • Interruption
• Conclusion

This article covers bugs in Create, one of the most popular Minecraft mods. Let's see how they impact the game and then submit pull requests with the necessary fixes.

Introduction

Create is a Minecraft mod that adds shafts, gears, and other mechanical components. You can combine them to build machines powered by rotational force, from a simple windmill to a fully automated cake factory.

The mod is hugely popular: it has more than 206 million downloads on CurseForge alone. That doesn't even include Modrinth, and the mod keeps getting updates for new versions of the game. I'm pretty sure almost everyone who opened the article has played this mod.

Let's check the project

Ctrl + V doesn't work

The SchematicEditScreen.java(130) file

Let's take a look at the menu for pasting a schematic.

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;
        }
      }
    }
  }
  ....
}

The PVS-Studio warning:

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

The idea is simple: the player copies the coordinates, presses Ctrl + V in a dedicated window, and the fields fill in automatically.

But the replaceAll call marked // <= serves no purpose. Strings in Java are immutable, so the method doesn't modify the original string; it returns a new one instead. The new string is immediately discarded, leaving the spaces instead.

The code then splits the string by commas, leaving a space at the beginning of the second and third elements, and Integer.parseInt(" 67") throws a NumberFormatException. As a result, the fields remain empty.

After opening the menu, I saw the expected behavior:

• -156, 67, 204 doesn't work;
• -156,67,204 works.

The fix fits on a single line. We just add an assignment before the replaceAll operation:

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

Unnecessary maximum

The ScheduleScreen.java(557) file

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;
}

The PVS-Studio warning:

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

The analyzer warning clearly explains the error: the result of the Math.max(....) call isn't written anywhere.

I looked through the commit history, expecting to find some traces of refactoring. I thought the developers might have moved the width calculation to a separate method and removed the assignment to maxWidth but left the Math#max call untouched. There was nothing like that, though. The file first appeared on February 1, 2022, in a single 941-line commit, and the Math#max call without an assignment had been there all along. The original initialization of maxWidth with a call to getConditionColumnWidth() was there too.

What would change if the devs added maxWidth = Math.max(....)? Nothing. The column width where the text appears (see the image below) is calculated in advance with some extra room: getConditionColumnWidth() goes through the entire column and takes the maximum width, so no line can extend beyond it. I checked this using the debugger: in every case I was able to test, it returned the same width. So, the code is searching for the maximum of two identical numbers.

The line didn't break over time, nor was it affected by any refactoring. From day one, it had done nothing—for four years and through several updates to newer Minecraft versions.

I'd suggest removing the unnecessary wrapper and leaving renderInput on its own.

Ignore error handling

The MechanicalCrafterBlockEntity.java(535) file

First of all, let me show you what the mechanical crafter looks like:

Let's take a look at a method in its logic:

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;
}

The PVS-Studio warning:

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

The getPrecedingCrafters method returns the crafters that come earlier in the chain. In other words, the ones that feed items into the current crafter. It can never return null. A list is created at the start, and both method exits return it. Even if the block isn't a crafter, the method returns an empty list:

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;
}

This means the branch with ejectWholeGrid() is unreachable. The preceding list can never be null, but it can be empty, and often is. What if we replace the check with isEmpty() and test it in the game? Let's see what happens.

Before the fix:

After the fix with isEmpty:

The video shows that, before the fix, items entering the loop were passed from one crafter to another indefinitely. That could potentially cause trouble and break someone's factory :)

However, after the check was replaced with isEmpty(), the loop didn't even have a chance to form. The method determined that there was nowhere else to expect items from, so it ejected them.

However, as soon as I added two more crafters to the chain, the fix stopped working:

Two options are possible here. Either the mechanical crafter needs to detect loops differently, then simply replacing the check with isEmpty() isn't enough. Or this looping behavior isn't a bug at all. In that case, we can just remove the null check.

The logging crash

The TrainRelocationPacket.java(52) file

Here's the client-server train movement package handler:

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;

  ....
}

The PVS-Studio warning:

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

The condition triggers in two cases: either the train can't be found, or the entity is of the wrong class. The second case is fine: it logs a proper warning. However, in the first case, the code accesses train.id when train is null.

The funny part is that the message we're trying to log ends with not present on server. The branch was created for a missing train, but it fails because of it.

The packet comes from the client, meaning it chooses the train ID. A modified client build can send requests to transfer nonexistent trains and make the server fill its log with stack traces.

The server won't crash: Create registers its packets so they execute safely, and even if an exception occurs, it gets logged, while the packet simply goes unhandled.

We can fix this by including the identifier in the package:

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

The two-state theory

The MechanicalMixerBlockEntity.java(238) file

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;
}

The PVS-Studio warning:

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

Here, basin.isEmpty() can never return true, so return in this branch is unreachable. Most likely, the developers meant to check basinBlockEntity rather than Optional.

This doesn't affect gameplay in any way. Further down, the code has other checks that work with the same block, and if the basin is empty, either the loop doesn't run or availableItems is null.

But a dead if is more dangerous than it looks. It's a safeguard that doesn't really protect against anything. Rewriting the checks below leaves a hole that no one will think to look for because the check is kind of there.

Method contracts must be followed

The Contraption.java(230) file

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

Can you see the error? What if we take a look inside 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();
}

Now it's clear. The method explicitly warns about null twice: with an annotation and a comment. However, the caller code doesn't check anything.

The PVS-Studio warning:

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

It's easier to break than it seems. The very first line calls nbt.getString("Type"), and if the key doesn't exist, an empty string is returned. This type isn't in the registry either, so the code returns null again.

We can verify this pretty easily by giving ourselves a minecart with a nonexistent contraption type:

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

The command won't break anything, and the item will go into the inventory without a problem. But once we put the minecart on the rails, MinecartContraptionItem will try to build a contraption from this data, and that's where we'll get a stack trace with NullPointerException. Minecraft will replace the player's contraption with a vanilla minecart.

We can easily add the check:

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

But then null will need to be handled in both code snippets that call it. That raises the question of what to do with a minecart whose contents the mod can no longer understand. Should we leave it as an empty minecart, or simply not load the entity at all? I think it's worth asking the mod developers. They clearly know better than anyone else what to do here.

Interruption

The ServerLagger.java(14) file

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

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

The PVS-Studio warning:

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

ServerLagger is a helper class for the killtps start <tickTime> debug command that puts the server to sleep for the specified number of milliseconds on every tick, allowing to see how the mod behaves when the TPS drops. It's available only in debug builds and only to operators (that's what server administrators are called), so there's no need to worry. However, the code is still incorrect, and it's worth explaining why.

When the thread is interrupted, sleep immediately throws an InterruptedException and also clears the interrupt flag. In other words, after catch, it seems like nothing's left: no flag, no exception, just a line in System.err. The method returns control without issue, and the server continues running as if it had never been called.

An interrupt is a request for the entire thread to stop, not just the part where you catch it. By interrupting the thread and doing nothing, you take that request and throw it away. The Javadoc explicitly points this out. One line is enough to pass the interrupt:

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

Conclusion

That's all for today. I collected all the bugs both in this article and in pull requests for the project's developers (click, click).

Want to try static analysis too? You can always integrate it into your own project with PVS-Studio. You can download a trial version here. The license is free for open-source projects.