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.
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:
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:
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.
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
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 expand(double x, double y, double z) method from the AxisAlignedBB class expands the collision size. The x, y, and z parameters are responsible for the volume and direction by which we should increase the collision size.
AxisAlignedBB (Axis Aligned Bounding Box) is a class that represents collision in Minecraft. It's widely used both to retrieve entities from a given area and to describe the collision box of those entities. The class stores the coordinates of the lower left and upper right edges of a rectangular parallelepiped.
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:
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.
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:
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|\\rIn this case, the regular expression will check the set of escaped line-break characters:
After a quick Google search and a moment of deep contemplation, I came to the following discovery:
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.
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:
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:
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:
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);
}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
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."
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.