Analyzing the TrinityCore project with PVS-Studio

Feb 24 2012

Author: Andrey Karpov

TrinityCore is a free project distributed under the GPL license. The project's purpose is to create alternative software to emulate a server of the multiplayer game World of Warcraft by Blizzard Entertainment. The main aim of the project is an educating one. The project is by no means intended to gain profit from its usage. The source code written in C and C++ is open, which means that it is distributed free and users are not imposed any obligations and responsibilities.

We have checked the project using the PVS-Studio 4.54 analyzer. There are few errors detected and they mainly relate to third-party libraries. The trial version of PVS-Studio can be used to thoroughly analyze the project: it will be sufficient to review all the odd code fragments. We will cite here only a few code fragments that caught our glance:

1. Misprint. 'other.y' is used twice.

inline Vector3int32& operator+=(const Vector3int32& other)
{
  x += other.x;
  y += other.y;
  z += other.y;
  return *this;
}

PVS-Studio: V537 Consider reviewing the correctness of 'y' item's usage. g3dlib vector3int32.h 77

2. Error in NEXT_CMP_VALUE macro.

static struct wordvalue doubles[] = {
 { "ch", (uchar*) "\014\031\057\057" },
 { "Ch", (uchar*) "\014\031\060\060" },
 { "CH", (uchar*) "\014\031\061\061" },
 { "c",  (uchar*) "\005\012\021\021" },
 { "C",  (uchar*) "\005\012\022\022" },
 };

#define NEXT_CMP_VALUE(src, p, store, pass, value, len) \
while (1)                                      \
{                                              \
  ......                                       \
  for (i = 0; i < (int) sizeof(doubles); i++)  \
  {                                            \
    const char * pattern = doubles[i].word;    \
    ...                                        \
    }                                          \
  }                                            \
  ......                                       \
}

PVS-Studio: V557 Array overrun is possible. The value of 'i' index could reach 39. libmysql ctype-czech.c 260

This is the correct code:

for (i = 0; i < (int) sizeof(doubles) / sizeof(doubles[0]); i++)

3. Only part of a string is copied.

class ACE_Name_Request
{
  ...
  char *type_;
};

void
ACE_Name_Request::type (const char *c)
{
  ACE_TRACE ("ACE_Name_Request::type");
  ACE_OS::strsncpy (this->type_,
                    c,
                    sizeof this->type_);
}

PVS-Studio: V579 The strsncpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. ace name_request_reply.cpp 251

The pointer's size is calculated instead of the string length.

4. Incompletely cleared buffer.

ACE_INLINE int
ACE_Thread::disablecancel(struct cancel_state *old_state)
{
  ...
  ACE_OS::memset (old_state,
                  0,
                  sizeof (old_state));
  ...
}

PVS-Studio: V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. ace thread.inl 172

5. Error in matrix comparison.

bool Matrix4::operator==(const Matrix4& other) const {
  if (memcmp(this, &other, sizeof(Matrix4) == 0)) {
    return true;
  } 
  ...
}

PVS-Studio: V575 The 'memcmp' function processes '0' elements. Inspect the third argument. g3dlib matrix4.cpp 385

6. Condition always true.

enum enum_mysql_timestamp_type
str_to_datetime(....)
{
  ...
  else if (str[0] != 'a' || str[0] != 'A')
  ...
}

PVS-Studio: V547 Expression 'str[0] != 'a' || str[0] != 'A'' is always true. Probably the '&&' operator should be used here. libmysql my_time.c 340

7. Parenthesis in a wrong place.

static my_bool socket_poll_read(my_socket sd, uint timeout)
{
  int res;
  ...
  if ((res = select((int) fd,
         &readfds, NULL, &errorfds, &tm) <= 0))
  {
    DBUG_RETURN(res < 0 ? 0 : 1);
  }
  ...
}

PVS-Studio: V593 Consider reviewing the expression of the 'A = B <= C' kind. The expression is calculated as following: 'A = (B <= C)'. libmysql viosocket.c 550

This is the correct code:

if ((res= select((int) fd,
            &readfds, NULL, &errorfds, &tm)) <= 0)

8. Incorrect checks whether the pointer is equal to 0.

There are rather many checks whether the pointer is equal to 0 after the pointer has been used. The first example:

bool OnCheck(Player* player, Unit* /*target*/)
{
  bool checkArea =
    player->GetAreaId() == AREA_ARGENT_TOURNAMENT_FIELDS ||
    player->GetAreaId() == AREA_RING_OF_ASPIRANTS ||
    player->GetAreaId() == AREA_RING_OF_ARGENT_VALIANTS ||
    player->GetAreaId() == AREA_RING_OF_ALLIANCE_VALIANTS ||
    player->GetAreaId() == AREA_RING_OF_HORDE_VALIANTS ||
    player->GetAreaId() == AREA_RING_OF_CHAMPIONS;

  return player && checkArea &&
         player->duel && player->duel->isMounted;
}

PVS-Studio: V595 The 'player' pointer was utilized before it was verified against nullptr. Check lines: 310, 312. scripts achievement_scripts.cpp 310

One more example:

CreatureAI* GetAI(Creature* creature) const
{
  ...
  Item* item =
    player->StoreNewItem(dest, ITEM_TEAR_OF_GODDESS, true);
  if (item && player)
    player->SendNewItem(item, 1, true, false, true);
  ...
}

PVS-Studio: V595 The 'player' pointer was utilized before it was verified against nullptr. Check lines: 224, 225. scripts hyjal.cpp 224

Conclusion

I can hardly believe that this project is being developed by enthusiasts learning how to program. We have cited just some of the detected issues, but still there are too few of them. Perhaps some of the developers have already used PVS-Studio to test TrinityCore. This thought is indirectly confirmed by the fact that most of the errors have been found with the help of the V595 diagnostic rule which has appeared quite recently.

#Cpp