Clever Geek Handbook

Checking the XMage code and why special rare cards for the Dragon's Maze collection are not available

image1.png


XMage is a client / server application for playing Magic: The Gathering (MTG). XMage began to evolve back in early 2010. During this time, 182 releases were released, a whole army of contributors gathered, and the project is still actively developing. An excellent opportunity to participate in its development as well! Therefore, today the unicorn from PVS-Studio will check the XMage codebase, and who knows, it might clash with someone in battle.



Briefly about the project



XMage has been actively developing for 10 years now. Its goal is to make a free, open source, online version of the original Magic: the Gathering card game .



Application features:



  • access to ~ 19,000 unique cards issued over the 20-year history of MTG;
  • automatic control and application of all existing rules of the game;
  • ;
  • (AI);
  • (Standard, Modern, Vintage, Commander );
  • , .




Stumbled upon the work of students from Delft University of Technology 2018 (Master's course in Software Architecture ). It consisted in the fact that the guys took an active part in open-source projects, which had to be quite complex and actively develop. Over an eight-week period, students studied the course and open-source projects to understand and describe the architecture of the selected software.



So that's it. In this work, the guys analyzed the XMage project, and one of the aspects of their work was obtaining various metrics using SonarQube (number of lines of code, cyclomatic complexity, code duplication, code smells, errors, vulnerabilities, etc.).



My attention was attracted by the fact that at the time of 2018, SonarQube scanning showed 700 defects (bugs, vulnerabilities) per 1,000,000 lines of code.



After digging into the history of the contributing guys, I found out that from the received report with warnings they made a pull-request to fix about 30 defects from the "Blocker" or "Critical" category. What about the rest of the warnings is not known, but I hope that they were not missed.



It's been 2 years since then and the codebase has grown by about 250,000 lines of code - a good reason to see how things are going.



About analysis



For analysis, I took the release of XMage - 1.4.44V0 .



I was very lucky with the project. Building XMage using Maven turned out to be very simple (as it was written in the documentation): 



mvn clean install -DskipTests


Nothing more was required of me. Cool?



There were no problems with the integration of the PVS-Studio plugin into Maven either: everything is as in the documentation .



After the analysis, 911 warnings were received, of which 674 were for warnings of 1 and 2 confidence levels. For the purposes of this article, I have not considered level 3 warnings, since there is usually a high percentage of false positives. I would like to draw your attention to the fact that when using a static analyzer in a real battle, you cannot ignore such warnings, since they can also indicate significant defects in the code.



In addition, I did not consider the warnings of some of the rules for the reason that they are better considered by those who are familiar with the project than me:



  • V6022, /. 336 .
  • V6014, , . 73 .
  • V6021, , . 36 .
  • V6048, , . 17 .


In addition, several diagnostic rules produced about 20 obvious false positives of the same type. Recorded in todo!



As a result, if we subtract everything, then about 190 positives got to me for consideration.



While reviewing the triggers, many minor defects of the same type were identified, which were either related to debugging or meaningless checking or operation. Also, a lot of positives were associated with a very strange piece of code that begged for refactoring.



As a result, for this article I have identified 11 diagnostic rules and analyzed one of the most interesting triggers.



Let's take a look at what happened.



Warning N1



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


Everything is simple here: the body of the second conditional statement if (card! = Null) in the if-else-if construction will never be executed, since the program will either not reach this point, or card! = Null will always be false .



Warning N2



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


A commonplace mistake that often occurred in my practice of checking open-source projects. Copy-paste? Or am I missing something? I will assume that you still need to return false in the else branch . PS If anything, there is no recursive call applies (....) , since these are different methods. Similar triggering:











  • V6004 The 'then' statement is equivalent to the 'else' statement. GuiDisplayUtil.java (194), GuiDisplayUtil.java (198)


Warning N3



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


The V6007 diagnostic rule triggers are quite popular for every project being checked. XMage is no exception (79 pieces). Actuation of the rule, in principle, everything is on the case, but many cases fall on debug, then on reinsurance, then on something else. In general, it is better for the author of the code to watch such positives than to me.



This triggering, however, is definitely an error. Depending on the beginning of the line filter.getMessage () to sbthe text "has ..." or "have ..." is added. But the mistake is that the developers check that the string begins with an uppercase letter, having converted this same string to lowercase before that. Oops. As a result, the added line will always be "have ...". The result of the defect is not critical, but also unpleasant: an illiterately composed text will appear somewhere.



The positives that I found the most interesting:



  • V6007 Expression 't.startsWith ("-")' is always false. BoostSourceEffect.java (103)
  • V6007 Expression 'setNames.isEmpty ()' is always false. DownloadPicturesService.java (300)
  • V6007 Expression 'existingBucketName == null' is always false. S3Uploader.java (23)
  • V6007 Expression '! LastRule.endsWith (".")' Is always true. Effects.java (76)
  • V6007 Expression 'subtypesToIgnore :: contains' is always false. VerifyCardDataTest.java (893)
  • V6007 Expression 'notStartedTables == 1' is always false. MageServerImpl.java (1330)


Warning N4



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 parser complains about dereferencing the null reference savedSpecialRares when execution reaches the first filling of the collection.



The first thing that comes to mind is simply confusing savedSpecialRares == null with savedSpecialRares! = Null. But in such a case, NPE can happen in the constructor of ArrayList when the collection is returned from the method, since savedSpecialRares == null is still possible. Fixing the code with the first solution that comes to mind is not a good option. Having understood the code a little, I found out that s avedSpecialRares is immediately determined by an empty collection when it is declared and is not reassigned anywhere else. This tells us thatsavedSpecialRares will never be null, and the dereferencing of a null reference, which the analyzer warns about, will never happen, since it will never reach the collection. As a result, the method will always return an empty collection.



PS To fix it, you need to replace savedSpecialRares == null with savedSpecialRares.isEmpty () .



PPS Alas, while playing XMage, you will not be able to get special rare cards for the Dragon's Maze collection .



Another case of dereferencing a null reference:



  • V6008 Null dereference of 'match'. TableController.java (973)


Warning N5



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


Here the ternary operator ?: Returns the same value regardless of the condition table.getStartTime () == null . I believe that code completion has played a cruel joke on the developer. Correction option: 



private void checkTableHealthState() {
  ....
  logger.debug(.... + formatter.format(table.getStartTime() == null
                                        ? table.getCreateTime()
                                        : table.getStartTime()) + ....);
  ....
}


Warning N6



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


Duplicate assignment string. It looks like the developer got a little carried away using hotkeys and didn't notice it. But given the fact that the effect has a large number of fields, the fragment deserves to be focused on.



Warning N7



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


From the analyzer's warning, we can conclude that selectUser.get () may throw a NoSuchElementException.



Let's take a closer look at what's going on here.



If you believe the comment that user already exists, then no exception will be thrown: 



....
if (!selectUser.isPresent()) {  // user already exists
  ....
}
User user = selectUser.get()
....


In this case, the program execution will not go into the body of the conditional statement. And everything will be all right. But then the question arises: why do we need a conditional operator with some complex logic if it never gets executed?



But what if the comment is nothing? 



....
if (!selectUser.isPresent()) {  // user already exists
    selectUser = UserManager.instance.getUserByName(userName);
    if (selectUser.isPresent()) {
      ....
    }
}
User user = selectUser.get(); // <=
....


Then the execution enters the body of the conditional statement and re-gets the user via getUserByName (). The user is checked again for validity, which suggests that the selectUser might be uninitialized. There is no else branch for this case, which further will lead to a NoSuchElementException on the line of code in question.



Warning N8



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 the code is confused about something here: first he makes sure that model is not a CheckBoxListModel , and then, in the end, explicitly casts the object to this type. Because of this, the setModel method will immediately throw a ClassCastException when it gets there.



The CheckBoxList.java file was added 2 years ago and this bug lives on in the code ever since. Apparently, there are no tests for incorrect parameters, there is no real use of this method with objects of inappropriate types, so it lives.



If suddenly someone hooks up on this method and reads the Javadoc, they will expect an IllegalArgumentException , not a ClassCastException... I don't think anyone will deliberately run into this exception, but who knows.



Given the documentation, the code should most likely look like this: 



public void setModel(ListModel model) {
  if (!(model instanceof CheckBoxListModel)) {
     throw new IllegalArgumentException(
        "Model must be an instance of CheckBoxListModel!");  
  }
  else {
    super.setModel(model);
  }
}


Warning N9



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 warns the developer that an object is being accessed before it is checked for null . Triggers of this rule are often found in articles about checking open-source projects: usually the reason for this is unsuccessful refactoring or changing contracts for methods. If you pay attention to the declaration of the getPlayer () method , then everything will immediately fall into place: 



// Result must be checked for null.
// Possible errors search pattern: (\S*) = game.getPlayer.+\n(?!.+\1 != null)
Player getPlayer(UUID playerId);


Warning N10



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


Having seen that this error is in the tests, you can immediately devalue the found defect, thinking: "Well, these are tests." If so, then I disagree with you. After all, tests play a rather important role in development (although not as noticeable as programming), and when a defect appears in a release, they immediately start pointing fingers at the tests / testers. So, defective tests are untenable. Why then are such tests needed? Why waste resources on them?



The testArcaneAdaptationGiveType () method tests the "Arcane Adaptation" card. Each player is dealt cards to a specific playing area. And thanks to copy-paste, playerA got 2 identical "Silvercoat Lion" cards in the "Cemetery" play area , and playerBso nothing got. Then some magic and testing itself.



When testing comes to the "graveyard" of playerB in the current rally, then the test execution never enters the loop, because there was nothing in the "graveyard". This I found out with the good old System.out.println () when starting the test .



Corrected 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");   // <=
....


After I tweaked the code, when I ran the test, checking for creatures in playerB 's graveyard started working. Ave, System.out.println () !



The test is green both before and after the correction, which is a lot of luck. But in case of any edits that change the logic of the program execution, such a test will do you a disservice, notifying you of successful completion even if there are errors.



Same copy-paste elsewhere:



  • V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. PaintersServantTest.java (33), PaintersServantTest.java (29), PaintersServantTest.java (27), PaintersServantTest.java (31)
  • V6072 Two similar code fragments were found. Perhaps, this is a typo and 'playerB' variable should be used instead of 'playerA'. SubTypeChangingEffectsTest.java (32), SubTypeChangingEffectsTest.java (28), SubTypeChangingEffectsTest.java (26), SubTypeChangingEffectsTest.java (30)


Warning N11



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


Diagnostic rule V6086 diagnoses incorrect if-else-if formatting , implying the omission of else .



This code snippet demonstrates this. In this case, because of the return null expression, inaccuracy in formatting does not lead to anything, but it’s cool to find such cases, since you don’t have to.



Let's consider a case where omitting the else can lead 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, in the case of obj == null , the object in question will be assigned some value, and the missing else will cause the newly assigned object to start being checked along the if-else-if chain , while the object was supposed to immediately return from method.



Conclusion



Checking XMage is another article that reveals the capabilities of modern static analyzers. In modern development, the need for them only grows as the complexity of the software increases. And no matter how many releases, tests, user feedback you have: a bug will always find a loophole to get into your codebase. So why not add another barrier to your defense?



As you understand, analyzers are prone to false positives (including PVS-Studio Java). This can be the result of both an obvious flaw and too confusing code (alas, the analyzer did not figure it out). You need to treat them with understanding and immediately unsubscribe without hesitation , but while false positives are waiting for their correction, you can use one of the methodssuppressing warnings.



In conclusion, I suggest you personally "touch" the analyzer by downloading it from our website.





If you want to share this article with an English-speaking audience, please use the translation link: Maxim Stefanov. Checking the Code of XMage, and Why You Won't Be Able to Get the Special Rare Cards of the Dragon's Maze Collection .


More articles:


All Articles