Михаил Евтихевич

31 Мар 2022

Теги:

#CSharp #GameDev

Проверка Barotrauma статическим анализатором PVS-Studio

31 Мар 2022

Автор: Михаил Евтихевич

Введение
Результаты проверки
Заключение

Barotrauma – игра, в которой можно поуправлять подлодкой, попрятаться от монстров и даже поиграть на аккордеоне в попытке не пойти ко дну. Посмотрим, как проект, начатый инди-студией Undertow Games и продолженный совместно с FakeFish, выглядит изнутри. Для этого исследуем исходный код, преимущественно написанный на языке C#, с помощью статического анализатора PVS-Studio.

Введение

Barotrauma – многопользовательский космический 2D-симулятор подлодки в жанре survival horror. Игроку предстоит управлять подлодкой, отдавать приказы, устранять протечки и противостоять опасностям.

Barotrauma не является Open Source проектом в обычном понимании. Ранняя версия игры доступна бесплатно, текущая версия доступна в Steam. Разработчики опубликовали исходный код на GitHub для того, чтобы сообщество игроков могло свободно разрабатывать более комплексные моды и находить существующие ошибки.

Результаты проверки

Ошибки в if

V3001 There are identical sub-expressions 'string.IsNullOrEmpty(EndPoint)' to the left and to the right of the '||' operator. BanList.cs 41

public bool CompareTo(string endpointCompare)
{
  if (string.IsNullOrEmpty(EndPoint) || string.IsNullOrEmpty(EndPoint)) 
  { return false; }
  ....
}

Значение EndPoint проверяется дважды. Разработчик, скорее всего, забыл поменять параметр EndPoint на endpointCompare при копировании метода string.IsNullOrEmpty. Вообще программисты очень часто ошибаются в методах сравнения. Рекомендую почитать статью коллеги об этом.

V3004 The 'then' statement is equivalent to the 'else' statement. ServerEntityEventManager.cs 314

public void Write(Client client, IWriteMessage msg, 
                  out List<NetEntityEvent> sentEvents)
{
  List<NetEntityEvent> eventsToSync = null;
  if (client.NeedsMidRoundSync)
  {
    eventsToSync = GetEventsToSync(client);
  }
  else
  {
    eventsToSync = GetEventsToSync(client);
  }
  ....
}

Вне зависимости от значения client.NeedsMidRoundSync будет выполняться одно и то же. Возможно следует убрать else-ветвь или переработать её поведение.

Следующий фрагмент кода вызвал два предупреждения:

V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless DebugConsole.cs 2177
V3022 Expression 'args.Length < 2' is always false. DebugConsole.cs 2183

private static void InitProjectSpecific()
{
  ....
  AssignOnClientRequestExecute(
    "setclientcharacter",
    (Client senderClient, Vector2 cursorWorldPos, string[] args) =>
    {
      if (args.Length < 2)
      {
        GameMain.Server.SendConsoleMessage("....", senderClient);
        return;
      }

      if (args.Length < 2)
      {
        ThrowError("....");
        return;
      }
    );
  ....
}

Две одинаковые проверки. В случае выполнения условия первого if метод завершит работу, иначе же обе then-ветви не будут выполнены.

При такой работе сообщение будет отправляться, но запись ошибки методом ThrowError не произойдёт. Следует объединить два тела if или изменить условие второго.

V3022 Expression 'newPrice > 0' is always true. DebugConsole.cs 3310

private static void PrintItemCosts(....)
{
  if (newPrice < 1)
  {
    NewMessage(depth + materialPrefab.Name + 
    " cannot be adjusted to this price, because it would become less than 1.");
    return;
  }

  ....

  if (newPrice > 0)
  {
    newPrices.TryAdd(materialPrefab, newPrice);
  }
  ....
}

Если newPrice меньше или равен 0, следует выполнение тела первого if. После этого исполнение метода завершается. Следовательно условие второго if всегда будет истинно. Поэтому можно добавить тело второго if в else-ветвь первого или же вовсе его убрать.

Опечатки

V3005 The 'arrowIcon.PressedColor' variable is assigned to itself. ChatBox.cs 164

public ChatBox(GUIComponent parent, bool isSinglePlayer)
{
  ....
  arrowIcon = new GUIImage(....)
  {
    Color = new Color(51, 59, 46)
  };
  arrowIcon.HoverColor = arrowIcon.PressedColor = 
  arrowIcon.PressedColor = arrowIcon.Color;
  ....  
}

Значение переменной arrowIcon.PressedColor присваивается самой себе. В то же время внутри класса GUIIMage содержится свойство SelectedColor. Скорее всего, разработчик хотел использовать его, но опечатался.

V3005 The 'Penetration' variable is assigned to itself. Attack.cs 324

public Attack(float damage, 
              float bleedingDamage, 
              float burnDamage, 
              float structureDamage,
              float itemDamage, 
              float range = 0.0f, 
              float penetration = 0f)
{
   ....
   Range = range;
   DamageRange = range;
   StructureDamage = LevelWallDamage = structureDamage;
   ItemDamage = itemDamage;     
   Penetration = Penetration;                // <=
}

Ещё одна подобная ошибка. В этом случае программист хотел проинициализировать свойства объекта, но вместо параметра penetration присвоил свойству Penetration своё же значение.

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: t.Character.Name. DebugConsole.cs 1123

private static void InitProjectSpecific()
{
  AssignOnClientRequestExecute("traitorlist", 
      (Client client, Vector2 cursorPos, string[] args) =>
  {
    ....
    GameMain.Server.SendTraitorMessage(
     client, 
     string.Format("- Traitor {0} has no current objective.",            // <=
                   "",                                                   // <=
                   t.Character.Name),                                    // <=
     "",
     TraitorMessageType.Console);   
  });
}

Исходя из смысла использованной в методе GameMain.Server.SendTraitorMessage фразы, логично предположить, что спецификатор ввода {0} должен был содержать t.Character.Name. Однако же там окажется пустая строка.

Ошибка, скорее всего, является следствием неудачного copy-paste предыдущего использования метода GameMain.Server.SendTraitorMessage:

GameMain.Server.SendTraitorMessage(client, 
"There are no traitors at the moment.", "", TraitorMessageType.Console);

Возможно возникновение NullReferenceException

V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Voting.cs 181

public void ClientRead(IReadMessage inc)
{
  ....
  foreach (GUIComponent item in
           GameMain.NetLobbyScreen?.SubList?.Content?.Children)    // <=
  {
    if (item.UserData != null && item.UserData is SubmarineInfo) 
    {
      serversubs.Add(item.UserData as SubmarineInfo); 
    }
  }
  ....
}

Если хотя бы один компонент из последовательности GameMain.NetLobbyScreen?.SubList?.Content?.Children будет равен null, то результат всего выражения тоже будет равен null. В таком случае будет выброшено исключение NullReferenceException при попытке перебора в foreach.

Подробно про использование оператора ?. в foreach можно прочитать в этой статье.

V3027 The variable 'spawnPosition' was utilized in the logical expression before it was verified against null in the same logical expression. LevelObjectManager.cs 274

private void PlaceObject(LevelObjectPrefab prefab, 
                         SpawnPosition spawnPosition, 
                         Level level, Level.Cave parentCave = null)
{
  float rotation = 0.0f;
  if (   prefab.AlignWithSurface 
      && spawnPosition.Normal.LengthSquared() > 0.001f          // <=
      && spawnPosition != null)                                 // <=
  {
    rotation = MathUtils.VectorToAngle(new Vector2(spawnPosition.Normal.Y, 
                                                   spawnPosition.Normal.X));
  }
  ....
}

Из кода видно, что сначала идёт вызов метода LengthSquared у поля Normal переменной spawnPosition и сравнение его с заданным значением, а затем переменная проверяется на null. Если spawnPosition будет равна null, то возникнет исключение NullReferenceException.

Простейшим решением будет поместить проверку на null в начало условия.

V3095 The 'level' object was used before it was verified against null. Check lines: 107, 115. BeaconMission.cs 107

public override void End()
{
  completed = level.CheckBeaconActive();                        // <=
  if (completed)
  {
    if (Prefab.LocationTypeChangeOnCompleted != null)
    {
      ChangeLocationType(Prefab.LocationTypeChangeOnCompleted);
    }
    GiveReward();
    if (level?.LevelData != null)                               // <=
    {
      level.LevelData.IsBeaconActive = true;
    }
  }
}

Сначала значение level.CheckBeaconActive присваивается, а затем используется оператор ?. в выражении level?.LevelData. В этом случае возможны ситуации, когда level будет равен null — и будет выброшено исключение NullReferenceException либо level никогда не будет null — и проверка избыточна.

Выход за границы

V3106 Possibly index is out of bound. The '0' index is pointing beyond 'Sprites' bound. ParticlePrefab.cs 303

public ParticlePrefab(XElement element, ContentFile file)
{
  ....
  if (CollisionRadius <= 0.0f) 
    CollisionRadius = Sprites.Count > 0 ? 1 : 
                                          Sprites[0].SourceRect.Width / 2.0f;
}

При выполнении условия тернарного оператора значение переменной CollisionRadius станет равно 1. В противном случае значение Sprites.Count равно 0, и при обращении к первому элементу коллекции возникнет исключение IndexOutOfRangeException.

Ранее по коду встречается проверка: является ли коллекция пустой.

if (Sprites.Count == 0)
{
  DebugConsole.ThrowError($"Particle prefab \"{Name}\" in the file \"{file}\"
                            has no sprites defined!");
}

Однако блокирование выполнения дальнейшего кода не происходит. Разработчику следует пересмотреть условие тернарного оператора.

Лишние действия

V3107 Identical expression 'power' to the left and to the right of compound assignment. RelayComponent.cs 150

public override void ReceivePowerProbeSignal(Connection connection, 
                                             Item source, float power)
{
  ....
  if (power < 0.0f)
  {
    ....
  }
  else
  {
    if (connection.IsOutput || powerOut == null) { return; }

    if (currPowerConsumption - power < -MaxPower)
    {
      power += MaxPower + (currPowerConsumption - power);
    }
  }
}

Программист пытается прибавить переменной power переменную MaxPower и разницу между переменными currPowerConsumption и power. В разложенном варианте выражение будет иметь вид:

power = power + MaxPower + (currPowerConsumption - power);

Выражение можно упростить, так как переменная power вычитается сама из себя. Упрощённый код будет выглядеть так:

power = MaxPower + currPowerConsumption;

Всегда false

V3009 It's odd that this method always returns one and the same value of 'false'. FileSelection.cs 395

public static bool MoveToParentDirectory(GUIButton button, object userdata)
{
  string dir = CurrentDirectory;
  if (dir.EndsWith("/")) { dir = dir.Substring(0, dir.Length - 1); }
  int index = dir.LastIndexOf("/");
  if (index < 0) { return false; }
  CurrentDirectory = CurrentDirectory.Substring(0, index+1);

  return false;
}

Странный метод, всегда возвращающий false. Возможно, здесь нет ошибки и так задумано, либо один из return должен возвращать true.

Утраченное значение

V3010 The return value of function 'Trim' is required to be utilized. GameServer.cs 1589

private void ClientWriteInitial(Client c, IWriteMessage outmsg)
{
  ....

  if (gameStarted)
  {
    ....

    if (ownedSubmarineIndexes.Length > 0)
    {
      ownedSubmarineIndexes.Trim(';');
    }
    outmsg.Write(ownedSubmarineIndexes);
  }
}

Метод Trim не меняет значение ownedSubmarineIndexes, поэтому нет смысла вызывать его, не сохраняя результат. Правильный вариант:

ownedSubmarineIndexes = ownedSubmarineIndexes.Trim(';');

Заключение

PVS-Studio обнаружил ряд ошибок, опечаток и недочётов в исходном коде Baratrauma. Многие из них довольно легко допустить при разработке и оставить незамеченными на code-review.

Статический анализ может помочь разработчикам сохранить время, которое они бы потратили на поиск и исправление багов, и уделить его разработке нового контента. Однако мало проверить код один раз. Максимальная эффективность достигается при регулярном использовании анализатора в процессе написания кода.

О проверке других проектов с помощью статического анализатора PVS-Studio можно прочесть в нашем блоге.

#CSharp #GameDev