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

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

    Picture 1


    Введение


    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 и попробовать проверить собственные проекты.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Almost Perfect Libraries by Electronic Arts.
    • +31
    • 14,6k
    • 5
    PVS-Studio
    1 080,50
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 5

      +1
      В коде был int128_t
      Код не компилируется в MSVC получается?
        +3
        Компилируется. Я в Windows анализ выполнял. У этого типа под разные платформы разная реализация просто. В нашем анализаторе тоже есть такой тип и для MSVC используется специальное решение.
        +1
        Ну не знаю… Смысл проверять такие маленькие библиотеки? Когда они релизят свои игры, там баг на баге, что может говорить о качестве кода в больших проектах…
          0
          Смысл проверять такие маленькие библиотеки?
          В попытке поймать небольшой хайп. Конечно, получается не всегда. Но вот с калькулятором, например, получилось.
            +2

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

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

          Самое читаемое