Недавно вышла в свет Linux-версия анализатора PVS-Studio. С ее помощью был проверен ряд проектов с открытым исходным кодом. Среди них Chromium, GCC, LLVM (Clang) и другие. И сегодня к этому списку присоединятся проекты, которые были разработаны Walt Disney Animation Studios для сообщества специалистов по созданию виртуальной реальности. Давайте приступим к рассмотрению найденных предупреждений анализатора.
Компания Walt Disney вот уже много лет радует телевизионную аудиторию разных стран мира восхитительными историями и персонажами, даря ей незабываемые впечатления. Из года в год Disney выпускает все более увлекательные, зрелищные и сложные по реализации фильмы, и мультфильмы. Соответственно, увеличивается потребность в разработке различных программ, которые будут способствовать реализации творческих замыслов художников по визуальным эффектам.
Программисты Walt Disney Animation Studios оказывают поддержку специалистам по анимации и визуальным эффектам, создавая технологии, доступные в виде программ на C и C++ с открытым кодом для всех представителей отрасли виртуальной реальности. К таким программам можно отнести:
Открытые исходные коды программ от Disney можно скачать на сайте https://disney.github.io/ .
Рассмотренные проекты от Walt Disney невелики и насчитывают всего несколько десятков тысяч строк кода на C и C++. Отсюда и такое небольшое количество ошибок по проектам.
Предупреждение 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.
Аналогичные предупреждения анализатора:
Предупреждение 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 );Анализатор выдал еще одно такое предупреждение:
Предупреждение 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 должно быть другое поле.
Аналогичное предупреждение:
Предупреждение 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.
И еще парочка предупреждений:
Предупреждение 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 обнаружил еще несколько подобных мест:
Предупреждение 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'.
Предупреждение 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 свои поля, и не заметит хитрости своего коллеги. Поэтому лучше обнулять каждую переменную по отдельности.
Предупреждение 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 и обязательно это сделать.