Максим Стефанов

21 Ноя 2016

Теги:

#Cpp

Linux-версия PVS-Studio устроила себе экскурсию по Disney

21 Ноя 2016

Автор: Максим Стефанов

Немного о Disney
Результаты проверки
Заключение

Недавно вышла в свет Linux-версия анализатора PVS-Studio. С ее помощью был проверен ряд проектов с открытым исходным кодом. Среди них Chromium, GCC, LLVM (Clang) и другие. И сегодня к этому списку присоединятся проекты, которые были разработаны Walt Disney Animation Studios для сообщества специалистов по созданию виртуальной реальности. Давайте приступим к рассмотрению найденных предупреждений анализатора.

Немного о Disney

Компания Walt Disney вот уже много лет радует телевизионную аудиторию разных стран мира восхитительными историями и персонажами, даря ей незабываемые впечатления. Из года в год Disney выпускает все более увлекательные, зрелищные и сложные по реализации фильмы, и мультфильмы. Соответственно, увеличивается потребность в разработке различных программ, которые будут способствовать реализации творческих замыслов художников по визуальным эффектам.

Программисты Walt Disney Animation Studios оказывают поддержку специалистам по анимации и визуальным эффектам, создавая технологии, доступные в виде программ на C и C++ с открытым кодом для всех представителей отрасли виртуальной реальности. К таким программам можно отнести:

Partio (позволяет работать со стандартными форматами файлов частиц через единый интерфейс, реализованный по тому же принципу, что и унифицированные библиотеки изображений)
Alembic (открытый формат обмена, который становится индустриальным стандартом для обмена анимированной компьютерной графикой между пакетами по созданию цифрового контента)
Universal Scene Description (эффективная система, способная считывать и передавать данные сцены для обмена между различными графическими приложениями)
OpenSubdiv (осуществляет детальный рендеринг поверхностей (subdivision surface) на основе уменьшенных моделей)
Dinamica (плагин для Autodesk Maya, разработанный на основе физического движка реального времени Bullet Physics Library )
PTex (система наложения текстур)

Открытые исходные коды программ от Disney можно скачать на сайте https://disney.github.io/ .

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

Рассмотренные проекты от Walt Disney невелики и насчитывают всего несколько десятков тысяч строк кода на C и C++. Отсюда и такое небольшое количество ошибок по проектам.

Проект Partio

Предупреждение PVS-Studio: V547 Expression '"R"' is always true. PDA.cpp 90

ParticlesDataMutable* readPDA(....)
{
  ....
  while(input->good())
  {
    *input>>word;
    ....
    if(word=="V"){
        attrs.push_back(simple->addAttribute(....);
    }else if("R"){                                 // <=
        attrs.push_back(simple->addAttribute(....);
    }else if("I"){                                 // <=
        attrs.push_back(simple->addAttribute(....);
    }
    index++;
  }
  ....
}

Анализатор выдал сообщение, что условие всегда истина. Это будет приводить к тому, что действие, которое определено в else ветке, никогда не будет выполнено. Я считаю, такая ситуация возникла из-за невнимательности программиста, и условия, которые не будут приводить к такой ошибке, должны выглядеть следующим образом:

....
if(word=="V"){
    attrs.push_back(simple->addAttribute(....);
}else if(word=="R"){                                // <=
    attrs.push_back(simple->addAttribute(....);
}else if(word=="I"){                                // <=
    attrs.push_back(simple->addAttribute(....);
}
....

Предупреждение PVS-Studio: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *charArray[i] != '\0'. MC.cpp 109

int CharArrayLen(char** charArray)
{
  int i = 0;
  if(charArray != false)
  {
    while(charArray[i] != '\0')   // <=
    {
      i++;
    }
  }
  return i;
}

Если я правильно понимаю, функция CharArrayLen подсчитывает количество символов в строке charArray. Но действительно ли это так? По-моему, в условии цикла while есть ошибка, связанная с тем, что указатель на тип char сравнивается со значением '\0'. Высока вероятность, что забыта операция разыменования указателя. Поэтому условие цикла while должно выглядеть, например, так:

while ((*charArray)[i] != '\0')

Кстати проверка, расположенная чуть выше, тоже весьма странная:

if(charArray != false)

Проверка, конечно, работает, но будет намного лучше заменить её на такую:

if(charArray != nullptr)

В целом, создается впечатление, что функцию разрабатывал стажёр, или она не дописана. Не понятно, почему бы просто не написать код с использованием функции strlen():

int CharArrayLen(const char** charArray)
{
  if (charArray == nullptr)
    return 0;
  return strlen(*charArray);
}

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'attributeData[i]' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimple.cpp 266

ParticleIndex ParticlesSimple::
addParticle()
{
  ....
  for(unsigned int i=0;i<attributes.size();i++)
    attributeData[i]=
                  (char*)realloc(attributeData[i],       // <=
                                (size_t)attributeStrides[i]*
                                (size_t)allocatedCount);
  ....
}

Анализатор выявил в коде опасное использование realloc. Конструкция foo = realloc(foo, ...) опасна тем, что в случае невозможности выделения памяти функция вернет nullptr, тем самым перезаписав предыдущее значение указателя, что может привести к утечке памяти, а то и вовсе к падению программы. Возможно, такая ситуация крайне редка для многих случаев, но перестраховаться, я думаю, все же стоит. Чтобы предотвратить подобную ситуацию, рекомендуется сохранять значение указателя в дополнительной переменной перед использованием realloc.

Аналогичные предупреждения анализатора:

V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'attributeData[i]' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimple.cpp 280
V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'data' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimpleInterleave.cpp 281
V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'data' is lost. Consider assigning realloc() to a temporary pointer. ParticleSimpleInterleave.cpp 292

Проект Alembic

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'm_uKnot' to the left and to the right of the '||' operator. ONuPatch.h 253

class Sample
{
  public:
    ....
    bool hasKnotSampleData() const
    {
      if( (m_numU != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
          (m_numV != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
          (m_uOrder != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
          (m_vOrder != ABC_GEOM_NUPATCH_NULL_INT_VALUE) ||
           m_uKnot || m_uKnot)                            // <=
           return true;
      else
          return false;
    }
    ....
  protected:
    ....
    int32_t m_numU;
    int32_t m_numV;
    int32_t m_uOrder;
    int32_t m_vOrder;
    Abc::FloatArraySample m_uKnot;
    Abc::FloatArraySample m_vKnot;
    ....
}

И снова ошибка, связанная с рассеянностью программиста. Несложно догадаться, что вместо повторяющегося поля m_uKnot в условии должно стоять m_vKnot.

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. OFaceSet.cpp 230

void OFaceSetSchema::set( const Sample &iSamp )
{
  ....
  if ( iSamp.getSelfBounds().hasVolume() )
  {
      // Caller explicity set bounds for this sample of the faceset.
      
      m_selfBoundsProperty.set( iSamp.getSelfBounds() );   // <=
  }
  else                                       
  {
      m_selfBoundsProperty.set( iSamp.getSelfBounds() );   // <=
      
      // NYI compute self bounds via parent mesh's faces
  }
  ....
}

PVS-Studio обнаружил в коде оператор if..else, в котором в обоих исходах выполняется одно и то же, несмотря на разные комментарии. Вполне вероятно, что этот участок кода томится в очереди ближайших задач команды программистов, ну а пока этот участок кода ошибочен и требует доработки.

Предупреждение PVS-Studio: V629 Consider inspecting the '1 << iStreamID' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. StreamManager.cpp 176

void StreamManager::put( std::size_t iStreamID )
{
  ....
  // CAS (compare and swap) non locking version
  Alembic::Util::int64_t oldVal = 0;
  Alembic::Util::int64_t newVal = 0;

  do
  {
    oldVal = m_streams;
    newVal = oldVal | ( 1 << iStreamID );             // <=
  }
  while ( ! COMPARE_EXCHANGE( m_streams, oldVal, newVal ) );
}

Анализатор обнаружил потенциальную ошибку в выражении, которое содержит операцию сдвига.

В выражении newVal = oldVal | (1 << iStreamID ) смещается единица, представленная как int, и далее результат сдвига преобразуется к 64-битному типу. Потенциальная ошибка здесь заключается в том, что если значение переменной iStreamID может быть больше 32, то данный участок кода будет работать некорректно из-за возникновения неопределенного поведения.

Код станет безопаснее, если число 1 будет представлено 64-битным беззнаковым типом данных:

 newVal = oldVal | (  Alembic::Util::int64_t(1) << iStreamID );

Анализатор выдал еще одно такое предупреждение:

V629 Consider inspecting the '1 << (val - 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. StreamManager.cpp 148

Проект Universal Scene Description

Предупреждение PVS-Studio: V668 There is no sense in testing the '_rawBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. uvTextureStorageData.cpp 118

bool GlfUVTextureStorageData::Read(....) 
{
  ....
  _rawBuffer = new unsigned char[_size];                   // <=
  if (_rawBuffer == nullptr) {                             // <=
      TF_RUNTIME_ERROR("Unable to allocate buffer.");
      return false;
  }
  ....
  return true; 
}

Согласно современному стандарту языка, new в случае неудачного выделения памяти выбрасывает исключение, а не возвращает nullptr. Этот код - своеобразный архаизм программирования. Для современных компиляторов эти проверки больше не имеют смысла и их можно удалить.

Предупреждение PVS-Studio: V501 There are identical sub-expressions 'HdChangeTracker::DirtyPrimVar' to the left and to the right of the '|' operator. basisCurves.cpp 563

HdBasisCurves::_GetInitialDirtyBits() const
{
  int mask = HdChangeTracker::Clean;

  mask |= HdChangeTracker::DirtyPrimVar     // <=
       |  HdChangeTracker::DirtyWidths
       |  HdChangeTracker::DirtyRefineLevel
       |  HdChangeTracker::DirtyPoints
       |  HdChangeTracker::DirtyNormals
       |  HdChangeTracker::DirtyPrimVar     // <=
       |  HdChangeTracker::DirtyTopology
       ....
      ;

  return (HdChangeTracker::DirtyBits)mask;
}

Для определения mask использовано множество полей, среди которых есть повторяющиеся. Так не должно быть, поэтому программист или лишний раз по невнимательности использует одно и тоже поле, или, что скорее всего, вместо повторяющегося поля DirtyPrimVar должно быть другое поле.

Аналогичное предупреждение:

V501 There are identical sub-expressions 'HdChangeTracker::DirtyPrimVar' to the left and to the right of the '|' operator. mesh.cpp 1199

Проект OpenSubdiv

Предупреждение PVS-Studio: V595 The 'destination' pointer was utilized before it was verified against nullptr. Check lines: 481, 483. hbr_utils.h 481

template <class T> void
createTopology(....) 
{
  ....
  OpenSubdiv::HbrVertex<T> * destination = 
                        mesh->GetVertex( fv[(j+1)%nv] );
  OpenSubdiv::HbrHalfedge<T> * opposite  = 
                        destination->GetEdge(origin);  // <=

  if(origin==NULL || destination==NULL)                // <=
  {              
    printf(....);
    valid=false;
    break;
  }
  ....
}

Пожалуй, V595 является самым распространенным предупреждением, выдаваемым анализатором. PVS-Studio считает код опасным, если указатель разыменовывается, а потом ниже по коду проверяется. Если указатель проверяют, то предполагают, что он может быть равен нулю.

Так и происходит в вышеприведенном участке кода. Для инициализации указателя opposite происходит разыменование указателя destination, а далее идет проверка этих указателей на равенство NULL.

И еще парочка предупреждений:

V595 The 'destination' pointer was utilized before it was verified against nullptr. Check lines: 145, 148. hbr_tutorial_1.cpp 145
V595 The 'destination' pointer was utilized before it was verified against nullptr. Check lines: 215, 218. hbr_tutorial_2.cpp 215

Предупреждение PVS-Studio: V547 Expression 'buffer[0] == '\r' && buffer[0] == '\n ' ' is always false. Probably the '||' operator should be used here. hdr_reader.cpp 84

unsigned char *loadHdr(....)
{
  ....
  char buffer[MAXLINE];
  // read header
  while(true) 
  {
    if (! fgets(buffer, MAXLINE, fp)) goto error;
    if (buffer[0] == '\n') break;
    if (buffer[0] == '\r' && buffer[0] == '\n') break;   // <=
    ....
  }
  ....
}

Программист допустил ошибку в написании условия, которая приводит к тому, что условие всегда равно false. Скорее всего программист хотел сделать так, что если встречаются такие маркеры конца строки, как \n или \r\n, то необходимо выйти из цикла while. Поэтому ошибочное условие должно быть записано следующим образом:

 if (buffer[0] == '\r' && buffer[1] == '\n') break;

Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. main.cpp 652

main(int argc, char ** argv) 
{
  ....
  #if defined(OSD_USES_GLEW)
  if (GLenum r = glewInit() != GLEW_OK) {                 // <=
      printf("Failed to initialize glew. error = %d\n", r);
      exit(1);
  }
  #endif
....
}

Анализатор обнаружил потенциальную ошибку в выражении GLenum r = glewInit() != GLEW_OK, которое, скорее всего, работает не так, как задумывал программист. Создавая такой код, программист, как правило, хочет выполнить действия в следующем порядке:

(GLenum r = glewInit()) != GLEW_OK

Но приоритет оператора '!=' выше, чем приоритет оператора присваивания. Поэтому выражение вычисляется так:

GLenum r = (glewInit() != GLEW_OK)

Поэтому, если функция glewInit() отработает неправильно, на экране будет распечатан неверный код ошибки. Точнее, всегда будет напечатана единица.

Для исправления ошибки можно использовать скобки или вынести создание объекта за пределы условия, что придаст коду более читабельный вид. См. также главу 16 в книге "Главный вопрос программирования, рефакторинга и всего такого".

PVS-Studio обнаружил еще несколько подобных мест:

V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. glEvalLimit.cpp 1419
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. glStencilViewer.cpp 1128
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. farViewer.cpp 1406

Предупреждение PVS-Studio: V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_blocks' is lost. Consider assigning realloc() to a temporary pointer. allocator.h 145

template <typename T>
T*
HbrAllocator<T>::Allocate() 
{
  if (!m_freecount) 
  {
    ....
    // Keep track of the newly allocated block
    if (m_nblocks + 1 >= m_blockCapacity) {
        m_blockCapacity = m_blockCapacity * 2;
        if (m_blockCapacity < 1) m_blockCapacity = 1;
        m_blocks = (T**) realloc(m_blocks,                // <=
                                 m_blockCapacity * sizeof(T*));
    }
    m_blocks[m_nblocks] = block;                          // <=
    ....
  }
  ....
}

И снова опасное использование функции realloc. А почему оно опасное - описано выше в разделе 'Проект Partio'.

Проект Dynamica

Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to overflow of the buffer 'header.padding'. pdbIO.cpp 249

struct pdb_header_t
{
  int       magic;
  unsigned short swap;
  float       version;
  float       time;
  unsigned int data_size;
  unsigned int num_data;
  char      padding[32];
  //pdb_channel_t   **data;
  int             data;
};

bool pdb_io_t::write(std::ostream &out)
{
  pdb_header_t            header;
  ....
  header.magic = PDB_MAGIC;
  header.swap = 0;
  header.version = 1.0;
  header.time = m_time;
  header.data_size = m_num_particles;
  header.num_data = m_attributes.size();
  memset(header.padding, 0, 32 * sizeof(char) + sizeof(int));
  ....
}

Анализатор обнаружил потенциально возможную ошибку, связанную с заполнением буфера памяти header.padding. Через memset программист обнуляет 36 байтов в буфере header.padding, состоящий всего из 32 байт. На первый взгляд такое использование ошибочно, но, на самом деле, программист оказался хитрецом и вместе с header.padding обнуляет переменную data. Ведь поля padding и data структуры pdb_header_t расположены последовательно, а значит последовательно расположены и в памяти. Да! Ошибки нет в данной ситуации, но из-за такой хитрости в этом месте потенциально может появиться ошибка. Например, если другой программист изменит структуру pdb_header_t, добавив между полями padding и data свои поля, и не заметит хитрости своего коллеги. Поэтому лучше обнулять каждую переменную по отдельности.

Проект Ptex

Предупреждение PVS-Studio: V612 An unconditional 'return' within a loop. PtexHashMap.h 292

Entry* lockEntriesAndGrowIfNeeded(size_t& newMemUsed)
{
  while (_size*2 >= _numEntries) {
      Entry* entries = lockEntries();
      if (_size*2 >= _numEntries) {
          entries = grow(entries, newMemUsed);
      }
      return entries;
  }
  return lockEntries();
}

В вышеприведенной функции присутствует подозрительный цикл while, в котором при первом же проходе возвращается указатель на entries. Не кажется ли вам, что здесь что-то запутанное? Этот участок кода требует более детального рассмотрения.

Заключение

Статический анализ кода при написании качественного ПО играет очень важную роль, так как используя статический анализ на регулярной основе, вы сокращаете трудозатраты на устранение глупых или тяжело обнаруживаемых ошибок и сможете потратить больше времени на что-то полезное.

Если вы еще не проверяли свой проект на наличие ошибок и не пускались в увлекательные поиски багов, то советую Вам скачать PVS-Studio для Linux и обязательно это сделать.

#Cpp