How to crash Minecraft with your mod

• Introduction
• Let's check the project
  • Localization? Localization who?
  • How to lose a block
  • Difficulties with control characters
  • Bonus program for the Minecraft mod
  • API errors are plotting insidious schemes
  • Wrong coordinates... and a touch of imagination
• Summary

Developing modifications for Minecraft is a fun and enjoyable hobby. In this article, we'll explore errors in mods for our favorite game through the Custom NPC+ project. We'll reproduce them in-game and crash Minecraft.

Introduction

Anyone who has played Minecraft is well aware of its vibrant modding community. By the way, the author of the article is also an avid modder. I've witnessed countless versions of the game:

• a truly industrial sandbox where you can build your own giant factory;
• a shooter featuring cool weapon animations and well-designed gameplay;
• AAA-scale RPGs that feel like they were developed by famous game studios.

One of the key mechanics in creating your own Minecraft-based RPG project is adding non-player characters (NPCs) for player interaction. The project we've checked is one of the most popular mods for this purpose.

This is how the project author describes it:

"CustomNPC+ is a Minecraft mod that allows you to add custom NPCs to your world. It is developed for creative and storytelling players who want to make their Minecraft worlds more in-depth and unique."

I'd like to mention that CustomNPC+ is a fork of the original CustomNPC that adds content from newer versions of the mod to the older one for Minecraft 1.7.10. However, in addition to backporting, it also includes a lot of its own changes. If you wish to see the full list, you can check out the official page of the mod on CurseForge.

Features of the mod:

• Enables creation of NPCs with any skin, item, or model that exists in the game (even if they're added by another mod).
• Provides a flexible system for creating quests with different completion conditions.
• Features a fully functional NPC/player dialog system.
• Adds a faction system that allows you to customize the relationship between the player and faction NPCs.
• Allows each NPC to have a different role (a Mailman, a Merchant, a Shipper, etc.).
• Introduces jobs in addition to roles (an Item Dispenser, a Healer, a Guardian, etc.).

All in all, the mod adds a lot of mechanics to the game and enables the creation of your own servers or maps for other players.

I use this mod for some of my personal projects, so I just couldn't resist checking it for errors using the PVS-Studio static code analyzer. Now I present you an article featuring the most interesting warnings I've found while creating a pull request for the mod authors.

Let's check the project

Localization? Localization who?

An integral part of any game is its localization. In Minecraft, the out-of-the-box way to localize are the .lang files in the game resources. An algorithm is very simple: you type some text in the target language into a .lang file, and then you get it by its individual name in the code. Can static analysis find a potential localization error? My answer is yes. Here's a prime example:

The GuiMailbox.java(76) file

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

The mod has a Mailman profession and a mailbox unit. When a player opens the mailbox, the mail receive menu should display the time a mail was sent. A programmer correctly handled hour/hours, but they made a mistake with minute/minutes, resulting in the incorrect translation. Here's an in-game example of how it shouldn't be:

Since the text resource for mailbox.minute is already in the game sources, sending the commit took longer than fixing the code.

The analyzer message:

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

How to lose a block

Looking at the next warnings, I noticed the integer division in the code for healer's profession. I found it odd that in the expand method, which takes values with double precision, we write a value with int precision.

The JobHealer.java(41) file

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

The analyzer message:

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

In this code snippet, all entities in a rectangular area around the healer NPC, with a radius of range, are searched at the interval specified in the speed field. However, due to integer division, the Y-coordinate calculation results in a loss of half a block both upward and downward along the same coordinate. Here's a screenshot from the game to illustrate what this looks like:

• The healer NPC's collision is highlighted in white.
• The collision implemented now (5 / 2) is highlighted in red.
• The collision without integer division (5 / 2.0) is highlighted in green.

So, we need to have an odd number in the range variable to reproduce the error. In the version of this mod for Minecraft 1.7.10, the default value of the range variable is 5, so we can reproduce the error unless we touch the NPC's healing radius settings.

Notably, developers have fixed this error in the original mod for Minecraft 1.12.2. Unfortunately, I can't provide any direct evidence, so let's say I asked the universe—and this code snippet is what it sent back:

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

Since devs have fixed the bug in the original modification, the same should apply here.

Difficulties with control characters

First of all, before I show you the error, let me explain what's going on here. In addition to handling NPCs, the modification enables users to write small (sometimes large, depending on the author's imagination) scripts in such languages as ECMAScript (a subset of JavaScript), Python, Ruby, or Lua.

The game has an internal editor for this purpose. Sure, it's not as feature-rich as modern IDEs—static analysis or autocompletion are hard to achieve within the Minecraft framework. Yet, at least the highlighting and compiler output are there :)

Let's move on to the error related to this menu.

The TextContainer.java(35) file:

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

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

The analyzer message:

V6010 The return value of function 'replaceAll' is required to be utilized. TextContainer.java 35

The developers have created a variable that isn't used anywhere, and replaceAll is idle. However, the most important thing is that the regular expression written here also looks incorrect. Look at the character sets that the replaceAll method should replace:

• \\n
• \r
• \r\n

Such a set of characters is quite unusual. I think the developers intended to replace escaped line-break characters (\r, \n, \r\n), so the first part of the expression should have been enclosed in parentheses:

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

In this case, the regular expression will check the set of escaped line-break characters:

• \n
• \r
• \r\n

With that in mind, I suggest the following fix:

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

The issue of different line-break patterns piqued my interest, so I did a little pentesting unrelated to the error above. What if I typed carriage return characters and saved a string of them to the clipboard?

First of all, I decided to test how IntelliJ IDEA would behave. When carriage return characters were inserted, new lines appeared there. After my luck with IDEA, I ran the mod and tried inserting the characters into the Minecraft IDE. And it crashed :)

Here's the unfortunate character combination: "\r\r\r\r\r\r\r test"

Honestly, it's a fair question how to properly fix it or ensure the correct behavior regardless of the used line-break characters. For now, it's probably worth fixing the safeguard against escaped characters in replaceAll and keeping in mind that the editor has some technical debt.

Bonus program for the Minecraft mod

Here is the code for which the analyzer issued a warning.

The ScriptDBCPlayer.java(289) file:

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

The analyzer messages:

• 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

Comparing a variable to itself is certainly weird. However, to understand the issue we're facing now, we'll have to dig a little deeper:

• The DBC acronym in the class name means that it is closely related to the support of the JinGames Dragon Block C mod.
• The Script name identifies it as an API. It means that the class is used by the scripting engine. Thanks to the previous error, we already know what it is.
• The code is in the general method, the given part is responsible for setting bonuses to player attributes. Don't worry about it, we'll talk only about the "Strength" attribute further in the article.

First, I decided to explore the whole class and look for similar parts with the same check. It didn't take long.

The ScriptDBCPlayer.java(224) file:

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

In both cases, the developers parse the bonus identification number (bonusID), if this succeeds, they get the bonus by its ordinal position in the list; if this fails, they search for the bonus by name.

Based on this, the devs should've compared startIndex and the variable resulting from parsing bonusID in the erroneous code. But the trick is that both the parsed variable and the loop counter in the erroneous code are called startIndex. So, they're the same thing, which means there's no error :)

Jokes aside, the try block in the erroneous code is useless because the value of the startIndex variable is overwritten in the loop. We can try to reproduce the error by applying two bonuses to some attribute, and when we try to change the value for the second bonus, not the second bonus but instead, the one listed first, will be set because of the wrong comparison.

To check this, we can write two small scripts:

• A script that creates bonus attributes for a character and changes the bonus values to 5 and 15 (we expect the sum of attributes to be 20).
• A script that outputs the current attribute states to the chat.

Both scripts are called on the Interact event, which is simply right-clicking the NPC.

The script 1:

The script 2:

I won't keep you in suspense, the bug is indeed reproducible: instead of an expected total bonus of 20, we get a bonus of 16.

P.S. Here's a funky getter method that returns void, which caused me to spend an hour wondering why my script wasn't outputting the attribute value:

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

API errors are plotting insidious schemes

Here's an amusing error in the program logic.

The Availability.java(273) file:

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

The developers mixed up the "and" and "or" operators, causing the right part of the expression to always evaluate false. The code that throws exceptions is unreachable as a result. The analyzer found eight code fragments containing the same error in this class. Obviously, the error has "multiplied" due to copy-paste programming.

The fix is trivial—we'll replace && with || and commit.

The analyzer message:

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

Wrong coordinates... and a touch of imagination

I was quite surprised to encounter this error in the mod. The developers are incorrectly checking coordinates.

The LinkedNpcController.java(123) file:

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

This method is used to load an NPC into the world from the game files. The error lies in a typo—the developers compared posX twice instead of comparing posZ. The check insures that the NPC is initialized. If all three coordinates are zero, this NPC is invalid.

I tried to imagine a situation where the X and Y coordinates would be 0 and Z would be different, and I'd like to say that a scenario in which this typo could play any role is unlikely. No blocks can be below the zero coordinate in height, meaning the NPC would just fall into an infinite abyss. I don't think anyone would intentionally create an NPC at those coordinates.

The error is still worth fixing, though. As the saying goes, "Even a broken clock is right twice a day."

Summary

This is the third time I have managed to help open-source software developers in a difficult endeavor of finding errors, and I couldn't be happier. All the described bugs and more are included in the pull request on the official GitHub repository of the project.

Would you like to try static analysis? You can always implement it into your project using the PVS-Studio tool. You can download the trial version here. For open-source projects, we have a free licensing option.