Библиотеки Electronic Arts почти хорошего качества

29 Июл 2019

Автор: Святослав Размыслов

Введение
Предупреждение 1
Предупреждение 2
Предупреждение 3
Предупреждение 4
Предупреждение 5
Заключение

Наше внимание привлёк репозиторий Electronic Arts на GitHub. Он очень маленький и из двадцати трёх проектов нас заинтересовали только несколько C++ библиотек: EASTL, EAStdC, EABase, EAThread, EATest, EAMain и EAAssert. Проекты оказались тоже очень маленькими (около 10 файлов), поэтому мы нашли ошибки только в "самом большом" из 20 файлов :D Но нашли, и интересные! Пока писалась заметка, мы с коллегами также бурно обсудили игры компании EA и её стратегию :D.

Введение

Electronic Arts (EA) - американская компания, которая занимается распространением видеоигр. На сайте GitHub у неё есть небольшой репозиторий и несколько C++ проектов. Это C++ библиотеки: EASTL, EAStdC, EABase, EAThread, EATest, EAMain и EAAssert. Они очень маленькие и мы нашли интересные ошибки с помощью анализатора PVS-Studio только в "самом большом" из них - EAStdC (20 файлов). При таких объёмах сложно говорить о качестве кода в целом. Просто оцените 5 предупреждений и решите сами.

Предупреждение 1

V524 It is odd that the body of '>>' function is fully equivalent to the body of '<<' function. EAFixedPoint.h 287

template <class  T,
  int  upShiftInt, int downShiftInt,
  int  upMulInt,   int downDivInt>

struct FPTemplate
{
  ....
  FPTemplate operator<<(int numBits) const { return value << numBits; }
  FPTemplate operator>>(int numBits) const { return value << numBits; }

  FPTemplate& operator<<=(int numBits) { value <<= numBits; return *this;}
  FPTemplate& operator>>=(int numBits) { value >>= numBits; return *this;}
  ....
}

При перегрузке операторов сдвига программист допустил опечатку в одном из них, перепутав операторы << и >>. Скорее всего, это результат Copy-Paste-программирования.

Предупреждение 2

V557 Array overrun is possible. The value of 'nFormatLength' index could reach 16. EASprintfOrdered.cpp 246

static const int kSpanFormatCapacity = 16;

struct Span8
{
  ....
  char mFormat[kSpanFormatCapacity];
  ....
};

static int OVprintfCore(....)
{
  ....
  EA_ASSERT(nFormatLength < kSpanFormatCapacity);
  if(nFormatLength < kSpanFormatCapacity)
    spans[spanIndex].mFormat[nFormatLength++] = *p;                        // <=
  else
    return -1;

  switch(*p)
  {
    case 'b': case 'd': case 'i': case 'u': case 'o': case 'x': case 'X':
    case 'g': case 'G': case 'e': case 'E': case 'f': case 'F': case 'a':
    case 'A': case 'p': case 'c': case 'C': case 's': case 'S': case 'n':
    {
      // Finalize the current span.
      spans[spanIndex].mpEnd = p + 1;
      spans[spanIndex].mFormat[nFormatLength] = 0;                         // <=
      spans[spanIndex].mFormatChar = *p;
      if(++spanIndex == kSpanCapacity)
        break;
  ....
}

Массив spans[spanIndex].mFormat состоит из 16 элементов, т.е. последний валидный элемент имеет индекс 15. Сейчас код функции OVprintfCore написан так, что если индекс nFormatLength будет иметь максимально возможный индекс - 15, то произойдёт инкремент до 16. Далее в операторе switch возможен выход за границу массива.

Этот фрагмент кода скопирован ещё в 2 места:

V557 Array overrun is possible. The value of 'nFormatLength' index could reach 16. EASprintfOrdered.cpp 614
V557 Array overrun is possible. The value of 'nFormatLength' index could reach 16. EASprintfOrdered.cpp 977

Предупреждение 3

V560 A part of conditional expression is always true: (result >= 0). EASprintfOrdered.cpp 489

static int OVprintfCore(....)
{
  ....
  for(result = 1; (result >= 0) && (p < pEnd); ++p)
  {
    if(pWriteFunction8(p, 1, pWriteFunctionContext8, kWFSIntermediate) < 0)
      return -1;
    nWriteCountSum += result;
  }
  ....
}

Условие result >= 0 всегда истинно, потому что переменная result нигде в цикле не меняется. Код выглядит очень подозрительно и, скорее всего, имеет место ошибка в этом коде.

Этот фрагмент кода скопирован ещё в 2 места:

V560 A part of conditional expression is always true: (result >= 0). EASprintfOrdered.cpp 852
V560 A part of conditional expression is always true: (result >= 0). EASprintfOrdered.cpp 1215

Предупреждение 4

V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. EASprintfOrdered.cpp 151

static int OVprintfCore(....)
{
  ....
  int spanArgOrder[kArgCapacity] = { -1 };
  ....
}

Возможно, это не является ошибкой, но разработчиков следует предупредить, что значением -1 инициализируется только первый элемент массива spanArgOrder, а все остальные будут иметь значение 0.

Этот фрагмент кода скопирован ещё в 2 места:

V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. EASprintfOrdered.cpp 518
V1009 Check the array initialization. Only the first element is initialized explicitly. The rest elements are initialized with zeros. EASprintfOrdered.cpp 881

Предупреждение 5

V728 An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. int128.h 1242

inline void int128_t::Modulus(....) const
{
  ....
  bool bDividendNegative = false;
  bool bDivisorNegative = false;
  ....
  if(    (bDividendNegative && !bDivisorNegative)
     || (!bDividendNegative &&  bDivisorNegative))
  {
    quotient.Negate();
  }
  ....
}

Этот фрагмент кода был отформатирован для удобства, но в оригинале это очень длинное условие, которое сложно читать. Другое дело, если мы упростим условное выражение, как советует анализатор:

if( bDividendNegative != bDivisorNegative)
{
  quotient.Negate();
}

Код стал значительно короче, что сильно упростило понимание логики условного выражения.

Заключение

Как вы могли заметить, большинство подозрительных предупреждений продублированы в трех местах, причём в пределах одного и того же файла. Дублирование кода является очень плохой практикой разработки, т.к. усложняет поддержку кода. А если в такой код попадает ошибка, то стабильность программы резко снижается из-за распространения ошибок по всему коду.

Будем надеется, что опубликуют ещё что-то интересное и мы вновь вернёмся в этот репозиторий :). Ну а пока предлагаю желающим скачать PVS-Studio и попробовать проверить собственные проекты.

#Cpp