Linux-версія PVS-Studio влаштувала собі екскурсію по 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("І"){ // <=
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=="І"){ // <=
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
Проект Універсальний Scene Description


Попередження PVS-Studio: V668 There is no sense testing in 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)) error goto;
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 і обов'язково це зробити.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Maxim Stefanov. PVS-Studio for Linux Went on a Tour Around Disney.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.

Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.