XMage is a client-server application for playing Magic: The Gathering (MTG). XMage's development was started in early 2010. Since then, it has seen 182 releases, attracted an army of contributors, and it's still being actively developed even now. All that makes it a good reason for us to contribute to its development too! So, today the PVS-Studio unicorn is going to check the code base of XMage and maybe even get into a fight with some entities.
XMage is an actively developing application that has been around for 10 years already. Its goal is to make a free, open-source, online playable computer game version of the original Magic: the Gathering card game.
Its features include:
I've stumbled across a 2018 work made by students of Delft University of Technology (Software Architecture graduate master-level course). They were participating in active development of rather complex open-source projects. Over eight weeks, they were taking the course and studying those open-source projects to understand and then describe the architecture of the software application of their choice.
They picked the XMage project, and one of the aspects of their work was to gather a number of metrics using SonarQube (LOC count, cyclomatic complexity, code duplicates, code smells, bugs, vulnerabilities, etc.).
What attracted my attention was the fact that in 2018, SonarQube had detected 700 bugs and vulnerabilities on the total of 1,000,000 lines of code.
I've looked through the history of contributions and discovered that based on that analysis report, a pull-request was made for fixing about 30 defects in the categories "Blocker" or "Critical". What happened to the rest of the warnings is unknown, but I hope they weren't just left unaddressed.
That was two years ago; the code base has gained about 250,000 LOC more since then, which makes it an interesting idea to take a look into it and see how it's doing.
The check was done on the XMage 1.4.44V0 release.
It's always nice to have a project which is able to be built with Maven without any issues (just as described in the documentation):
mvn clean install -DskipTestsNo other actions were required of me. Isn't that cool?
Neither did I have any problems integrating the PVS-Studio plugin into Maven – again, just as described in the documentation.
The check finished with 911 warnings, 674 out of which were of the first and second levels. I left out the third-level warnings for this article since many of them usually turn out to be false positives. However, don't ignore third-level warnings in "actual combat" because some of them may point at critical defects too.
Besides, I didn't include a number of warnings produced by some of the diagnostics because I believe they should be checked by those who are more familiar with the project than I am:
Add to that about 20 similar warnings produced by several diagnostics, which were clearly false positives. Those we have put on our own todo list!
All in all, leaving out the ones mentioned above, I was left with about 190 warnings to examine.
Looking through them, I found lots of similar minor defects – either debug-specific or related to meaningless checks and operations. A lot of warnings were issued on one exceptionally strange code fragment asking to be refactored.
Out of those, I picked 11 diagnostic rules and one most interesting example to illustrate each.
Let's take a look at them.
Warning 1
V6003 The use of 'if (card != null) {...} else if (card != null) {...}' pattern was detected. There is a probability of logical error presence. TorrentialGearhulk.java(90), TorrentialGearhulk.java(102)
@Override
public boolean apply(Game game, Ability source) {
....
Card card = game.getCard(....);
if (card != null) {
....
} else if (card != null) {
....
}
....
}This one is simple: the body of the second conditional statement if (card != null) in the if-else-if construct will never execute because either execution won't reach this spot or card != null will always be false.
Warning 2
V6004 The 'then' statement is equivalent to the 'else' statement. AsThoughEffectImpl.java(35), AsThoughEffectImpl.java(37)
@Override
public boolean applies(....) {
// affectedControllerId = player to check
if (getAsThoughEffectType().equals(AsThoughEffectType.LOOK_AT_FACE_DOWN)) {
return applies(objectId, source, playerId, game);
} else {
return applies(objectId, source, playerId, game);
}
}This is a plain mistake that I see quite often in open-source projects. Is this copy-paste? Or am I missing something? My guess is that the else branch should be returning false.
P.S. Just in case you may be wondering, there are no recursive calls of applies(....) here since these are different methods.
A similar warning:
Warning 3
V6007 Expression 'filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")' is always false. SetPowerToughnessAllEffect.java(107)
@Override
public String getText(Mode mode) {
StringBuilder sb = new StringBuilder();
....
if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {
sb.append(" has base power and toughness ");
} else {
sb.append(" have base power and toughness ");
}
....
return sb.toString();
}V6007 warnings are pretty common in each project we've checked, and XMage is no exception (79 warnings). All of them are generally correct, but many are specific only to debugging mode or point at spots where the developers chose to play safe, and so on. It's usually better to leave such warnings to the project authors to investigate.
However, this particular one is definitely an error. Depending on the beginning of the filter.getMessage() string, either the string " has ..." or " have ..." is to be appended to sb. The problem is that the developer is checking if the string starts with an upper-case letter while having that same string converted to lower-case before the check. Oops. This will result in appending the " have ..." string every time. The effect of this bug isn't drastic, but it's still unpleasant as it leads to incorrect grammar somewhere in the game.
Other warnings of this type that I found worth checking:
Warning 4
V6008 Null dereference of 'savedSpecialRares'. DragonsMaze.java(230)
public final class DragonsMaze extends ExpansionSet {
....
private List<CardInfo> savedSpecialRares = new ArrayList<>();
....
@Override
public List<CardInfo> getSpecialRare() {
if (savedSpecialRares == null) { // <=
CardCriteria criteria = new CardCriteria();
criteria.setCodes("GTC").name("Breeding Pool");
savedSpecialRares.addAll(....); // <=
criteria = new CardCriteria();
criteria.setCodes("GTC").name("Godless Shrine");
savedSpecialRares.addAll(....);
....
}
return new ArrayList<>(savedSpecialRares);
}
}The analyzer points out the null dereference of savedSpecialRares, which takes place when the collection is about to be filled for the first time.
The first explanation that comes to mind is that the developer simply wrote savedSpecialRares == null instead of savedSpecialRares != null by mistake. But in that case, an NPE could occur in the ArrayList constructor when returning the collection from the method since the savedSpecialRares == null scenario is still possible. Relying on the first intuitive solution to fix the code isn't a good idea. After digging a bit deeper, I figured out that savedSpecialRares is defined as an empty collection right off at declaration and is not re-assigned anywhere after that. It means savedSpecialRares will never be null, so the null dereference pointed out by the analyzer will never take place as the collection will never be filled. Therefore, the method will always return an empty collection.
P.S. This bug can be fixed by replacing savedSpecialRares == null with savedSpecialRares.isEmpty().
P.P.S. I'm afraid you won't be able to get the rare cards of the Dragon's Maze set in XMage for some time.
Another null dereference:
Warning 5
V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value 'table.getCreateTime()'. TableManager.java(418), TableManager.java(418)
private void checkTableHealthState() {
....
logger.debug(.... + formatter.format(table.getStartTime() == null
? table.getCreateTime()
: table.getCreateTime()) + ....);
....
}The ternary operator ?: returns the same value no matter the result of the table.getStartTime() == null condition. I guess we should blame autocompletion for that. This is how it could be fixed:
private void checkTableHealthState() {
....
logger.debug(.... + formatter.format(table.getStartTime() == null
? table.getCreateTime()
: table.getStartTime()) + ....);
....
}Warning 6
V6026 This value is already assigned to the 'this.loseOther' variable. BecomesCreatureTypeTargetEffect.java(54)
public
BecomesCreatureTypeTargetEffect(final BecomesCreatureTypeTargetEffect effect) {
super(effect);
this.subtypes.addAll(effect.subtypes);
this.loseOther = effect.loseOther;
this.loseOther = effect.loseOther;
}Two identical assignments in a row. It looks as if the developer got carried away pressing the shortcuts and didn't notice duplicating a line. But since effect has a lot of fields, this snippet calls for a closer look.
Warning 7
V6036 The value from the uninitialized 'selectUser' optional is used. Session.java(227)
public String connectUserHandling(String userName, String password)
{
....
if (!selectUser.isPresent()) { // user already exists
selectUser = UserManager.instance.getUserByName(userName);
if (selectUser.isPresent()) {
User user = selectUser.get();
....
}
}
User user = selectUser.get(); // <=
....
}The warning seems to suggest that the selectUser.get() method could throw a NoSuchElementException.
Let's take a closer look at that.
If we assume that the comment about user already existing is true, no exception will be thrown:
....
if (!selectUser.isPresent()) { // user already exists
....
}
User user = selectUser.get()
....In this case, execution won't enter the body of the conditional statement, and everything will be fine. But then we may ask: why make a conditional statement with intricate logic if it never executes?
Now, what if the comment is wrong?
....
if (!selectUser.isPresent()) { // user already exists
selectUser = UserManager.instance.getUserByName(userName);
if (selectUser.isPresent()) {
....
}
}
User user = selectUser.get(); // <=
....In that case, execution enters the body of the conditional statement and retrieves the user by calling the getUserByName() method. That user is again checked for validity, which implies that selectUser may be uninitialized. There's no else branch provided to handle that case, so we'll end up with a NoSuchElementException in that line.
Warning 8
V6042 The expression is checked for compatibility with type 'A' but is cast to type 'B'. CheckBoxList.java(586)
/**
* sets the model - must be an instance of CheckBoxListModel
*
* @param model the model to use
* @throws IllegalArgumentException if the model is not an instance of
* CheckBoxListModel
* @see CheckBoxListModel
*/
@Override
public void setModel(ListModel model) {
if (!(model instanceof CheckBoxListModel)) {
if (model instanceof javax.swing.DefaultListModel) {
super.setModel((CheckBoxListModel)model); // <=
}
else {
throw new IllegalArgumentException(
"Model must be an instance of CheckBoxListModel!");
}
}
else {
super.setModel(model);
}
}The author of this code messed it up a bit: they first check that model is not CheckBoxListModel but then explicitly cast the object to that very type anyway. This will cause the setModel method to throw a ClassCastException once it gets to this spot.
The CheckBoxList.java file was added two years ago, and the bug is still there. There seem to be no tests for incorrect parameters, nor does this method seem to actually work with objects of inappropriate types, which explains why the bug has survived for so long.
If someone relies on this method and reads the Javadoc comment, they will expect an IllegalArgumentException rather than a ClassCastException. I don't think anyone would consciously handle this exception, but you never know.
Based on the documentation, this is what I think the code should look like:
public void setModel(ListModel model) {
if (!(model instanceof CheckBoxListModel)) {
throw new IllegalArgumentException(
"Model must be an instance of CheckBoxListModel!");
}
else {
super.setModel(model);
}
}Warning 9
V6060 The 'player' reference was utilized before it was verified against null. VigeanIntuition.java(79), VigeanIntuition.java(78)
@Override
public boolean apply(Game game, Ability source) {
MageObject sourceObject = game.getObject(source.getSourceId());
Player player = game.getPlayer(source.getControllerId());
Library library = player.getLibrary(); // <=
if (player != null && sourceObject != null && library != null) { // <=
....
}
}V6060 warnings indicate situations where an object is used before being checked for null. You can often see such warnings mentioned in articles about checks of open-source projects: they usually result from sloppy refactoring or changing methods' contracts. If you look at the declaration of the getPlayer() method, the problem becomes clear:
// Result must be checked for null.
// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)
Player getPlayer(UUID playerId);Warning 10
V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SubTypeChangingEffectsTest.java(162), SubTypeChangingEffectsTest.java(158), SubTypeChangingEffectsTest.java(156), SubTypeChangingEffectsTest.java(160)
@Test
public void testArcaneAdaptationGiveType() {
addCard(Zone.HAND, playerA, "Arcane Adaptation", 1); // Enchantment {2}{U}
addCard(Zone.BATTLEFIELD, playerA, "Island", 3);
addCard(Zone.HAND, playerA, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
addCard(Zone.HAND, playerB, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
....
for (Card card : playerB.getGraveyard().getCards(currentGame)) {
if (card.isCreature()) {
Assert.assertEquals(card.getName() + " should not have ORC type",
false, card.getSubtype(currentGame).contains(SubType.ORC));
Assert.assertEquals(card.getName() + " should have CAT type",
true, card.getSubtype(currentGame).contains(SubType.CAT));
}
}
}The fact that this defect was found in tests may force you to underestimate its significance ("It's not a big deal with tests.") If so, I disagree. Actually, tests play quite an important part in development (though not as prominent as coding does), and when bugs show up in the release, it's tests and testers that fingers are pointed at. Flawed tests are worthless. Then why have them? Why waste resources on them?
The testArcaneAdaptationGiveType() method tests the "Arcane Adaptation" card. Each player is dealt cards in several playing zones. Thanks to copy-paste, playerA will get two "Silvercoat Lion" cards in the "Graveyard" zone, while playerB will have none. This is followed by some magic and then the actual testing happens in the end.
When it comes to testing playerB's "graveyard" in the current deal, the test never enters the loop since the "graveyard" has been empty from the start. I found this out using the good old System.out.println() method when running the test.
The fixed copy-paste:
....
addCard(Zone.HAND, playerA, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion"); // <=
addCard(Zone.HAND, playerB, "Silvercoat Lion");
addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
addCard(Zone.GRAVEYARD, playerB, "Silvercoat Lion"); // <=
....Once I've fixed the code, the test was able to check the creatures in playerB's "graveyard". Viva System.out.println()!
Interestingly, the test was green both before and after the fix, which was great luck. On the other hand, when you introduce any modifications that change execution logic, such test could do you a disservice by reporting a pass even with errors present.
A few more copy-paste related defects of this type:
Warning 11
V6086 Suspicious code formatting. 'else' keyword is probably missing. DeckImporter.java(23)
public static DeckImporter getDeckImporter(String file) {
if (file == null) {
return null;
} if (file.toLowerCase(Locale.ENGLISH).endsWith("dec")) { // <=
return new DecDeckImporter();
} else if (file.toLowerCase(Locale.ENGLISH).endsWith("mwdeck")) {
return new MWSDeckImporter();
} else if (file.toLowerCase(Locale.ENGLISH).endsWith("txt")) {
return new TxtDeckImporter(haveSideboardSection(file));
}
....
else {
return null;
}
}The V6086 diagnostic rule detects incorrectly formatted if-else-if constructs, with the else keyword missing.
This is exactly what the snippet above demonstrates. In this case, the inaccurate formatting won't do any harm thanks to the return null statement, but it's cool to find issues like that since you can never be sure.
Here's an example of how a missing else leads to unexpected behavior:
public SomeType smtMethod(SomeType obj) {
....
if (obj == null) {
obj = getNewObject();
} if (obj.isSomeObject()) {
// some logic
} else if (obj.isOtherSomething()) {
obj = calulateNewObject(obj);
// some logic
}
....
else {
// some logic
}
return obj;
}Now, if obj == null, the object will be assigned some value and, because of the missing else, forced to go down the entire if-else-if line for checking, while the original idea was to have the object returned right off.
With this article about the results of checking the XMage project we have demonstrated yet one more time the abilities of modern static analyzers. In modern development, they are becoming more and more a necessity as software's complexity keeps growing. No matter how many releases, tests, and how much feedback you have, bugs will always find a way into your code base. Why not put up another barrier to strengthen your defense system then?
As you have seen, analyzers are subject to producing false positives (PVS-Studio Java being no exception). They may result either from imperfect implementation of the analyzer's diagnostic rules or from overcomplicated code, which the analyzer has a difficult time figuring out. Please be understanding about it and don't hesitate to report any false positives you encounter, and while they are waiting to be fixed, you can use one of the warning suppression mechanisms.
Feel free to download the analyzer and give it a try.