Author: Andrey Moskalev

Pursuing Diablo, the nephalem falls into another one of Belial's traps. Plunged into a deadly nightmare by the Lord of Lies, the hero must repeat their past victories over the Great Evils to awaken. This time, however, the nephalem is doomed to defeat due to the inexplicable distortions of reality inherent in nightmares. But suddenly, a mysterious creature comes to help, and with it, hope arises once again.

Introduction

"There are a lot of things you can find on GitHub!", I think as I browse it and always find some interesting and unusual projects. This time, I came across a Diablo 3 game server emulator written in C#. Diablo 3 is a renowned game developed and published by Blizzard Entertainment. Like the other games in the franchise, it has won quite a lot of fans.

Being a developer in a company producing a static code analyzer, I couldn't resist testing our tool on the source code of one of my favourite games, even though it's not the original code. As a result, I've found a couple of code fragments with both potential and quite obvious errors. In this article, I suggest to take a look at them.

To make the content more engaging and varied, I've covered some of the bugs in a Diablo-themed narrative :) We're going to take on the role of a mysterious being who helps the protagonist escape the nightmarish vision Belial has cast in their mind. The story takes place many years after the defeat of Malthael, the Angel of Death, and the release of the Great Evils. Some of them, including Belial, the Lord of Lies, have since regained much of their former power.

Note that the connections between the stories and possible errors in the given code fragments are mostly made up and have been added to make the article more interesting. You may think of the code review subsections as mini-games, like the ones we go through when picking locks, cracking ciphers, or disarming traps in video games :) However, the code fragments that contain potential errors are real. They're present in the emulator source code at the time of writing the article.

Act I. Caldeum

Trapped in a nightmare, the nephalem found themselves in the heart of Caldeum, a city known as the "Jewel of the East".

The Palace of Caldeum is where the hero once defeated Belial. The task seemed obvious: infiltrate the palace and repeat the previous victory over the Lord of Lies.

Battling the minions of the Lesser Evil, the nephalem wandered the city streets, searching in vain for a way to the palace. In the nightmare, the city looked like a vast, unfamiliar maze of alleyways. So, an hour passed, and then another, and another... Eventually, the hero stumbled upon a folded parchment — a map of the city. They unfolded the parchment. It was... blank. "What a mockery!", the nephalem thought as a strange creature suddenly appeared out of the bright light next to the hero. It looked like something between a human and a unicorn (you can meet all sorts of creatures in a nightmare, can't you?), but the light it emitted made it difficult to see any particular detail. The creature looked as if it didn't want to harm the hero. On the contrary, it wanted to help. As it approached the protagonist, the creature used a mysterious artefact (let's call it a "distortion detector") next to the map.

Let's dispel distortion

No matter how realistic an illusion is, there are always distortions in it — errors in its source code. The absence of an image on the map seems to be the result of such distortion.

public MapRevealSceneMessage MapRevealMessage(Player plr)
{
  if (
       .... ||
       SceneSNO.Id == 183556 ||
       SceneSNO.Id == 183557 ||
       SceneSNO.Id == 183502 ||
       SceneSNO.Id == 183505 ||
       SceneSNO.Id == 183557 ||
       SceneSNO.Id == 183519 ||
       ....
     )
  {
    return new MapRevealSceneMessage
    {
      ....
      MiniMapVisibility = true
    };
  }
  else
  {
    return new MapRevealSceneMessage
    {
      ....
      MiniMapVisibility = false
    };
  }
}

To dispel it and help the hero, try to find an error in the following code fragment, and then compare your answer with the one below.

The distortion detector message:

V3001 There are identical sub-expressions 'SceneSNO.Id == 183557' to the left and to the right of the '||' operator. Scene.cs 581

In the above condition, the OR operator is probably used to enumerate the identifiers of the scenes (location options) in the city for which the mini-map should be visible. It would be reasonable to assume that all identifiers in the condition should be unique, but the distortion detector showed two identical identifiers (constants) with the 183557 value. One of them could be accidentally written instead of the unique identifier that belongs to the other scene. So, the MiniMapVisibility property for this scene now is erroneously set to false.

Act II. Burning Hells

After the creature dispelled the distortion, the image appeared on the map. With it, the nephalem easily found their way to the palace, where they once again defeated Belial (it was hardly the real Belial, but rather his nightmarish copy).

Upon leaving the palace, the hero inexplicably — as it often happens in nightmares — shifted straight to Hell and found themselves completely immobilised for some reason. The nephalem recognized the place where they ended up: here they had defeated Azmodan — another of the Great Evils. As soon as the nephalem thought about him, the monstrous voice of the Lord of Sin thundered from somewhere behind, rushing towards the defenseless hero.

In a moment of sheer desperation, the mysterious creature once again came to the rescue. It was now clear that the hero's inability to move was a result of some distortions.

The creature is trying to free the nephalem, but does it have enough time?

Let's dispel distortions

As you did last time, try to find an error in each of these code fragments. Remember: the nephalem's life depends on your success!

Distortion 1

public override bool Reveal(....)
{
  ....
  if (!randomed && Destination.DestLevelAreaSNO != 19794)
  ....
  if (World.IsPvP && Destination != null && ....)
  ....
  if (Destination != null)
  ....
  if (Destination == null || ....)
  ....
}

The distortion detector message:

V3095 The 'Destination' object was used before it was verified against null. Check lines: 1241, 1244. Portal.cs 1241

The detector indicates that the Destination property in the first conditional statement could potentially have a value of null, as indicated by all subsequent checks of the property. If Destination is null, an exception is thrown when Destination.DestLevelAreaSNO is dereferenced, causing the whole program to stop running.

To fix a potential issue, let's add an appropriate check to the first conditional statement:

if (!randomed && 
    Destination != null && 
    Destination.DestLevelAreaSNO != 19794)

Distortion 2

public override void Encode(GameBitBuffer buffer)
{
  ....
  buffer.WriteBool(EngagedWithRareTime.HasValue);
  if (EngagedWithRareTime.HasValue)
  {
    buffer.WriteInt(32, EngagedWithGoblinTime.Value);
  }
  buffer.WriteBool(EngagedWithGoblinTime.HasValue);
  if (EngagedWithGoblinTime.HasValue)
  {
    buffer.WriteInt(32, EngagedWithGoblinTime.Value);
  }
  buffer.WriteBool(InCombat.HasValue);
  if (InCombat.HasValue)
  {
    buffer.WriteBool(InCombat.Value);
  }
  ....
}

The distortion detector message:

V3095 The 'EngagedWithGoblinTime' object was used before it was verified against null. Check lines: 190, 192. TrickleMessage.cs 190

The detector shows a similar issue here. The value of the EngagedWithGoblinTime.HasValue property is written to the buffer, indicating that EngagedWithGoblinTime may be null. A few lines above, EngagedWithGoblinTime.Value is dereferenced, which may lead to an exception.

If you look at the condition of the if statement, in which the dangerous dereference occurs, you can easily see that it checks whether a property with a very similar name to EngagedWithGoblinTime has a value. One could think that instead of the EngagedWithRareTime value, the EngagedWithGoblinTime value should be checked in the condition. However, if we look at the following conditional operator, we can see that EngagedWithGoblinTime and EngagedWithRareTime are mixed up, indeed: not in the condition of the first if, but in its then block when dereferenced. So, here's how we can fix it:

if (EngagedWithRareTime.HasValue)
{
  buffer.WriteInt(32, EngagedWithRareTime.Value);
}
buffer.WriteBool(EngagedWithGoblinTime.HasValue);

Act III. High Heavens

Just mere moments before Azmodan struck, the creature managed to free the hero from the immobility. The nephalem instantly dodged the attack and, without wasting any time, violently struck back. It was a tough fight. Azmodan summoned his minions to help, but the nephalem still managed to win.

Once the Lord of Sin had fallen, the environment around the hero began to rapidly change. The scenery of the Burning Hells gave way to the scenery of... the blazing Heaven.

The nephalem stood at the entrance to the Crystal Arch — the source of all angels' power. That's where the hero once defeated the most insidious of the Great Evils — Diablo — when he, having absorbed the powers of all the other Great Evils, attempted to desecrate the Arch and thus claim the final victory in the war against Heaven.

It wasn't hard to guess what awaited the hero on the other side of the entrance, but there was no other choice. After all, the nephalem had already won this fight once, they could handle it now. With that in mind, the nephalem walked towards the Arch.

Things went wrong within the first few minutes of the fight. The protagonist's blows did no discernible damage to the Lord of Terror, but each of Diablo's attacks seemed deadly. Even the heat emanating from him felt so strong, as if the hero wasn't wearing any armor at all.

No doubt there was some distortion here. But it didn't make Diablo completely invincible. It changed the stats of the hero's own weapons and armor, making them the most mediocre. It would be dangerous to use such equipment against a reanimated skeleton, let alone the Prime Evil.

Clearly, the fight was hopeless. The nephalem dodged the attacks as best as they could, but their luck wasn't infinite. In the end, the hero missed a not-so-hard blow that could've been fatal. They fell to the ground, and the Lord of Terror prepared to deliver the final blow. At that moment, just in time as always, the nephalem's mysterious sidekick appeared to take the brunt of the blow. Severely wounded, the creature flew to the side and crashed next to the protagonist. With its last ounce of strength, it used the artefact, hoping to dispel the distortion in time.

Let's dispel distortion

public static void Generate(....)
{
  ....
  filteredList = filteredList.Where(
    a =>
    !a.Name.Contains("FireD") &&
    !a.Name.Contains("PoisonD") &&
    !a.Name.Contains("HolyD") &&
    !a.Name.Contains("ColdD") &&
    !a.Name.Contains("LightningD") &&
    !a.Name.Contains("ArcaneD") &&
    !a.Name.Contains("MinMaxDam") &&
    isCrafting ? !a.Name.ToLower().Contains(....)
               : !a.Name.Contains(....)
  );
  ....
}

The distortion detector message:

V3123 Perhaps the '?:' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its condition. AffixGenerator.cs 207

Look at the left side of the ternary expression. At first glance, it looks like this: "isCrafting ? ....". However, first impressions can often be deceiving. The precedence of the ? operator is lower than that of the && operator, so the left side of the ternary operator actually looks like this:

!a.Name.Contains("FireD") &&
....
!a.Name.Contains("MinMaxDam") &&
isCrafting ? ....

It seems that the code author didn't consider this, so the conditional expression passed to Where may not work as expected. We can resolve the potential issue by adding parentheses around the ternary expression:

filteredList = filteredList.Where(
    a =>
    ....
    !a.Name.Contains("MinMaxDam") &&
    (isCrafting ? ....)
);

Epilogue

With a feeling of absolute superiority, Diablo struck again, finally killing the hero... or so he thought. Suddenly, the attack bounced off of the armor, leaving only a scratch on it. The distortion dissipated, and the nephalem's equipment regained its former power! The hero perked up, sprang up with a swift movement, and charged at the demon. Just like that day, the Heavens were long shaken by the battle. Just like that day, Diablo has fallen. Belial's curse began to break. The protagonist then turned their attention to their companion. The hero wanted to go over and help him, to ask who he was, how he got here, and why he was helping them... So many questions and so little time. The nightmare dissipated, and the hero woke up.

But we're not finished yet...

This little story is over. However, in the previous section, I haven't covered all the code fragments containing errors that I'd like to discuss in this article. I want to undo this injustice and cover them here. I invite anyone who wants to dig a little deeper into the code to take a look.

To level up your analytical skills, try to find one error in each code fragment by yourself and then check the answers.

Case 1

public static void GrantCriteria(....)
{
  ....
  else
  {
    ....
    uint alcount = alreadycrits.CriteriaId32AndFlags8;

    var newrecord = D3.Achievements
                      ....
                      .SetQuantity32(alcount++) // <=
                      .Build();

    int critCount = UnserializeBytes(achievement.Criteria).Count;
    if (critCount >= 5)
    {
      GrantCriteria(client, 74987246353740);
    }
    client.Account.GameAccount.AchievementCriteria
                              .Remove(alreadycrits);

    client.Account.GameAccount.AchievementCriteria
                              .Add(newrecord);
  }
  ....
}

The PVS-Studio warning:

V3159 Modified value of the 'alcount' operand is not used after the postfix increment operation. AchievementManager.cs 360

We expect that alcount should be incremented by 1, then the resulting value should be passed to the SetQuantity32 method. In reality, the opposite happens: first, the alcount value is passed to the method, and only then the value of the variable is incremented by 1.

We can easily fix the error by replacing alcount++ with the alcount + 1 or ++alcount expression.

Case 2

public class LooterBrain : Brain
{
  ....
  public override void Think(int tickCounter)
  {
    ....
    if (LootLegendaries)
      targets.Concat(....);
    ....
  }
  ....
}

The PVS-Studio warning:

V3010 The return value of function 'Concat' is required to be utilized. LooterBrain.cs 154

Even if LootLegendaries is true, no new objects are added to targets. That happens because the Concat method doesn't modify any of the original enumerations but creates a new one. To fix the error, we can assign the result of the Concat method execution to the targets variable.

In the game, the error lies in in the inability of loot-collecting pets to pick up legendary items. This is indicated by the context in which the LooterBrain class is used, with the LootLegendaries property set to true. Its value is set using the second constructor argument:

public LooterPetAnniversary(....): base(....)
{
  ....
  SetBrain(new LooterBrain(this, true));
  ....
}

Case 3

public override bool Reveal(....)
{
  bool isGolem = false;
  if (this is BaseGolem ||
      this is IceGolem ||
      this is BoneGolem ||
      this is DecayGolem ||
      this is ConsumeFleshGolem ||
      this is BloodGolem)
  {
    ....
    isGolem = false;
  }
  ....
  if (....)
    player.InGameClient.SendMessage(new PetMessage()
    {
      ....
      Index = isGolem ? 9 : player.CountFollowers(SNO) + PlusIndex,
      ....
    });
}

The PVS-Studio warning:

V3022 Expression 'isGolem' is always false. Minion.cs 230

It seems reasonable to assume that within the first conditional expression, the isGolem variable should be assigned the true value, but instead it is reassigned false. As a result, an incorrect value may be assigned to the Index property.

Conclusion

I hope this article has refreshed your memories of past adventures in Diablo 3 or, if you haven't played it yet, made you want to. Also, this article has given you the opportunity to practice in analyzing C# code. It's no secret that learning from other people's mistakes minimises the risk of making them in your own code.

By the way, if you're interested in the tool used to find the errors, you can download it from the official website and try it for free.

See you in the next articles!