В феврале 2014 аргентинская студия OKAM открыла исходный код мультиплатформенного игрового движка Godot Engine, а не так давно состоялся релиз версии 1.0. Как вы уже догадались, речь пойдёт об анализе исходного кода этого движка. В качестве инструмента анализа использовался статический анализатор кода PVS-Studio. Помимо ознакомительного характера, эта статья носит и практический: читатели могут пополнить багаж своих знаний, разработчики - исправить ошибочные или "узкие" места.  Но обо всём по порядку.
        
Об анализируемом проекте

Прежде чем приступать к анализу, хотелось бы вкратце рассказать об его объекте. Godot Engine - кроссплатформенный игровой движок с открытым исходным кодом. Был разработан аргентинской студией OKAM в 2001 году и использовался исключительно внутри студии. В 2014 году Godot Engine был выпущен под лицензией MIT. На движке можно создавать как 2D, так и 3D-игры. Список поддерживаемых платформ впечатляет: Windows, OS X, Linux, Android, iOS, BlackBerry 10, HTML5, flash, NaCl, PlayStation 3, PlayStation Vita и 3DS. Исходный код движка можно найти в соответствующем репозитории на GitHub.


Анализ исходного кода

Хочу отметить, что в этой статье приведены далеко не все предупреждения, выданные анализатором. Я собрал здесь наиболее интересные и кратко прокомментировал их. 

Статья получилось достаточно объёмной, так что запаситесь терпением, кофе и печеньками. И не забудьте поставить приятную музыку на фон. Приятного чтения, мы начинаем наш экскурс!


Кашу маслом не испортишь

Странный подзаголовок? И да, и нет. Если в жизни он действительно себя оправдывает, то в программировании всё совсем иначе. Порой повторяющиеся переменные или подвыражения могут быть куда опаснее, чем это кажется на первый взгляд. Почему? Сейчас увидите.

Начнём, пожалуй, с довольно распространённой ошибки - одинаковых подвыражениях в пределах одного выражения. Чаще всего такие конструкции возникают в случае копипаста или невнимательности. Хочу заметить, что странные (избыточные/ошибочные, нужное подчеркнуть) сравнения встречаются довольно часто как в этом проекте, так и в других.

Классический пример:



Предупреждение анализатора: V501 There are identical sub-expressions 'type' to the left and to the right of the '&&' operator. s3_pkt.c 971

Для наглядности выделим участок подвыражения, содержащего ошибку:



В выражении 2 раза встречается одинаковая переменная 'type'.  Этот код не опасен, но двойное использование переменной 'type' не имеет смысла. В случае, если 'type' или иное подвыражение имеет тип 'false', до последней проверки даже не дойдёт. Код избыточен.  Другое дело, если вместо переменной 'type' подразумевалась какая-нибудь другая, или же другое подвыражение (схожее с 'type != SSL3_RT_APPLICATION_DATA' или 'type != SSL3_RT_HANDSHAKE'). В таком случае это место уже не будет таким уж безобидным, так что не стоит недооценивать возможную опасность подобных участков кода.

Аналогичный фрагмент кода встретился ещё один раз. Приводить его не буду, но напишу сообщение анализатора: V501 There are identical sub-expressions 'type' to the left and to the right of the '&&' operator. d1_pkt.c 761

Подобный случай, но с другим подвыражением: V501 There are identical sub-expressions 'rs >= 4' to the left and to the right of the '&&' operator. http_client.cpp 290

Следующий пример подобной ошибки:



Предупреждение анализатора: V501 There are identical sub-expressions 'section == "float_array"' to the left and to the right of the '||' operator. collada.cpp 982

В принципе, всё понятно из предупреждения. В выражении дважды проверяется, что в переменной 'section' находится строка "float_array". Вопрос лишь в том, является ли это просто лишним сравнением, или подразумевалось что-то ещё, например (проявим фантазию) "double_array"? Сложно наверняка сказать, насколько глубока кроличья нора, но нужно быть внимательнее.

Эта ошибка, к слову, встретилась 2 раза. Соответствующее сообщение, указывающее на вторую ошибку:

• V501 There are identical sub-expressions 'section == "float_array"' to the left and to the right of the '||' operator. collada.cpp 1079

Продолжаем:



Предупреждение анализатора: V501 There are identical sub-expressions 'k.mod.command' to the left and to the right of the '||' operator. text_edit.cpp 1565

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

Продолжая тему странных сравнений:



Предупреждение анализатора: V501 There are identical sub-expressions '(c == ' ')' to the left and to the right of the '||' operator. a_print.c 76

Из кода видно, что дважды встречается подвыражение '(c == ' ')'. Возможно, что одно из них просто лишнее, но возможен и иной вариант - сравнение должно было осуществляться не с символом пробела.

Думали, что на этом подозрительные сравнения закончились? А вот и нет. Я же говорил, что их много. Так что продолжаем разговор:



Сообщение анализатора: V590 Consider inspecting the 'arg[0] != 0 && arg[0] == ' '' expression. The expression is excessive or contains a misprint. godot_win.cpp 175

Про этот случай точно можно сказать, что он не опасен. Но выражение избыточно. Можно обойтись просто условием (arg[0] == ' '). 




Ошибки, связанные с типами данных

Вам уже, наверное, хочется отвлечься от дублирующихся сравнений и переключиться на что-нибудь другое? Что ж, тогда у меня для вас хорошие новости. 

Теперь предлагаю рассмотреть ошибку, также достаточно часто встречающуюся среди новичков. Однако и настоящие профессионалы иногда наступают на те же грабли. Взглянем на код и несколько объявлений:



Предупреждение анализатора: V557 Array overrun is possible. The 'Physics2DServer::SHAPE_CONVEX_POLYGON' index is pointing beyond array bound. test_physics_2d.cpp 194

Объявление массива 'body_shape_data' и перечисления 'ShapeType' приведены здесь не случайно, так как именно в них и кроется причина ошибки. Кто быстро догадался в чём дело - мои поздравления. Для остальных разъясню. Из определения видно, что размер массива 'body_shape_data' равен 6, а с учётом того, что нумерация индексов начинается с 0, индекс последнего элемента будет равен 5. Теперь взглянем на перечисление 'ShapeType'. В перечислениях нумерация элементов также начинается с 0, тогда элемент 'SHAPE_CONVEX_POLYGON' будет иметь индекс 6. Как результат - выход за границы массива. 

Привожу сообщение анализатора об аналогичной ошибке: V557 Array overrun is possible. The 'Physics2DServer::SHAPE_CONVEX_POLYGON' index is pointing beyond array bound. test_physics_2d.cpp 209

Если посмотрите в код, то видно, что дело в том же перечислении и даже в том же его элементе. Что неудивительно, ведь если не знать, что код неверен - его так и можно клонировать дальше. А после всего этого пожинать плоды своей деятельности.

Следующий пример кода весьма подозрителен. Взглянем на него:



Сообщение анализатора: V547 Expression 'p_bytes < 0' is always false. Unsigned type value is never < 0. memory_pool_static_malloc.cpp 159

Дело в том, что аргумент 'p_bytes' имеет беззнаковый тип 'size_t'. Наименьшее значение, которое может принять 'p_bytes' это 0. Это означает, что условие p_bytes < 0 всегда будет ложным. При этом соседнее условие p_bytes <= 0 будет истинным только в одном случае - когда p_bytes==0. Одним словом, этот код возможно содержит ошибку.

Схожий пример. 



Сообщение анализатора: V547 Expression 's < 0' is always false. Unsigned type value is never < 0. particles_2d.cpp 230

Переменная 's' имеет беззнаковый тип, следовательно, её значение никогда не будет отрицательным. Выражение (s < 0) всегда будет ложным и переменная 's' не будет увеличена на 2147483647.

Встретился такой участок кода: 



Предупреждение анализатора: V674 The '0.5' literal of the 'double' type is compared to a value of the 'int' type. tween.cpp 272

Объявление функции '_run_equation' выглядит следующим образом:



Итак, функция вернула значение, представленное типом с плавающей точкой. Это значения явно приведено к целочисленному типу 'int'. После чего, оно вдруг сравнивается с константой 0.5. Что-то здесь не так.

Как вариант - неправильно расставлены скобки и на самом деле должно быть написано:





Опечатки и неаккуратный копипаст

Найти опечатку не всегда просто. Особенно когда код синтаксически полностью корректен и не вызывает предупреждений компилятора. Другое дело, что логики в нём нет.  Давайте взглянем на следующий фрагмент кода:



Сообщение анализатора: V519 The 'ret[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 305, 306. physics_server.cpp 306

Если на фоне множества строк кода разглядеть ловушку в подобном участке тяжело. Но если сократить функцию, как здесь, то ляп сразу становится виден.  Два раза подряд производится присваивание одному и тому же элементу массива разных значений. Естественно, смысла в себе это не несёт. Но стоит заметить, что перед этим размер массива увеличивается до 2, так что очевидна допущенная опечатка - один из индексов должен был быть равен 1.

Подобная ошибка встретилась ещё раз. Соответствующее сообщение анализатора: V519 The 'ret[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 287, 288. physics_2d_server.cpp 288

Теперь давайте взглянем на пример, связанный c копипастом:



Предупреждения анализатора:

• V523 The 'then' statement is equivalent to the 'else' statement. scroll_bar.cpp 57
• V523 The 'then' statement is equivalent to the 'else' statement. scroll_bar.cpp 67

Интересный случай. Обе ветви оператора 'if' содержат одинаковые тела, причём данный код встречается буквально два раза друг за другом. Сложно, конечно, сказать, что хотел сделать программист на самом деле. Может быть в одной ветви, в отличие от другой вместо знака '+' необходимо использовать '-', а может и нет. Лично мне, как человеку, который с данным кодом не работал, сказать затруднительно. Но вот те, кто писал код, должны понять, на что указал анализатор и как это исправить.

Другой интересный тип опечаток приводит к бесконечным циклам. Взглянем на пример:



Предупреждение анализатора: V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'i'. scene_preloader.cpp 410

Опечатка, увы, вовсе не безобидна и рано или поздно приведёт к исчерпанию кучи. Как видно из кода, во втором цикле инкрементируется переменная 'i', хотя в условии выхода из цикла используется переменная 'j', которую и следовало бы увеличивать. Как следствие - цикл будет выполняться бесконечно. Ввиду того, что в теле цикла происходит добавление элементов к вектору 'rconns', этот процесс может идти достаточно долго, но в конечном итоге всё равно закончится плачевно.


Указатели

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



Сообщение анализатора: V532 Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. trex.c 506

Хочу отметить, что подобный код в том же файле встретился ещё 4 раза. Помимо того подобный код неоднократно встречался и прочих проектах, что свидетельствует о том, что данная потенциальная ошибка весьма распространена.

Дело в том, что здесь используется разыменовывание указателя, а после производится его инкремент. Но при этом значение, полученное при разыменовывании, никак не используется. Тогда напрашивается вопрос, зачем было производить сразу 2 операции? Если требовалось увеличить значение указателя - следует опустить операцию разыменовывания, если же требовалось изменить значение, следовало поставить скобки. Скорее всего хотели именно сделать инкремент для указателя, а звездочка затесалась в выражение случайно. Возможно, что это не ошибка, но поправить код всё же стоит.

Продолжая тему указателей. Эдакий деликатес - разыменовывание нулевого указателя. Меньше речи, больше кода:



Предупреждение анализатора: V522 Dereferencing of the null pointer 'create_trimesh_collision_node()' might take place. mesh_instance.cpp 177

Прежде чем перейти к сообщению анализатора, обращаю внимание читателя на интересный момент в теле метода 'create_trimesh_collision_node', а именно - на последнюю строку, которая никогда не будет выполнена. Интересно, для чего она была написана? В любом случае, смотрится интересно. 

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

Подобная ошибка: V522 Dereferencing of the null pointer 'create_convex_collision_node()' might take place. mesh_instance.cpp 211



Неопределённое поведение

Раз уж речь зашла о неопределённом поведении, рассмотрим ещё несколько примеров на эту тему:



Предупреждение анализатора: V567 Undefined behavior. The 'selected_track' variable is modified while being used twice between sequence points. animation_editor.cpp 1378

Не будем обращать внимание на лишнюю точку с запятой, так как это, скорее всего, просто опечатка, не ведущая к каким либо последствиям. Интереснее выражение слева от них, где выполняются операции постфиксного декремента и присваивания. Подобная конструкция приводит к неопределённому поведению. Непонятно, почему было бы не использовать просто декремент:



Продолжая раскрывать тему неопределённого поведения:



Сообщение анализатора: V567 Undefined behavior. The 't' variable is modified while being used twice between sequence points. tween_interpolaters.cpp 265

Данную запись можно было бы разбить на 2 строки, упростив и её понимание, и избавившись от неопределённого поведения. Достаточно было бы отдельно вынести выражение:



Но здесь оно является подвыражением. Получается, что слева и справа от оператора умножения располагается (t = t / d - 1) и (t). Неизвестно, какое из выражений будет вычислено первым. Однако, от этого зависит результат. Подробнее про неопределённое поведение, точки следования и с ними связанные вопросы можно почитать в описании диагностики V567.

Приведу ещё 2 предупреждения анализатора, указывающие на код, где встречаются подобные случаи:

• V567 Undefined behavior. The 't' variable is modified while being used twice between sequence points. tween_interpolaters.cpp 271
• V567 Undefined behavior. The 't' variable is modified while being used twice between sequence points. tween_interpolaters.cpp 367

7 раз отмерь - 1 отрежь

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

Пример ошибки - объявление переменных, которые нигде не используются.  Даже если это не влечёт за собой печальных последствий, такие действия попросту ни к чему. Яма может оказаться глубже, чем кажется, если подразумевались действия над переменными с большей областью видимости, но при этом используются вновь объявленные.

Взглянем на фрагмент кода:



Предупреждение анализатора: V561 It's probably better to assign value to 'styles_count' variable than to declare it anew. Previous declaration: export.cpp, line 610. export.cpp 633

Как видно, в теле оператора 'switch' (а точнее - в одной из его ветвей) объявляются переменные, имеющие те же типы и названия, что и переменные во внешней области видимости. При этом в дальнейшем работа производится именно с переменными с более узкой областью видимостью. Внешние же переменные нигде не используются. Порой подобные ошибки могут приводить к возникновению крайне неприятных ситуаций, так как, возможно, работа осуществляется не с той переменной, с которой планировалось. Порой подобные ошибки достаточно тяжело найти и исправить, особенно если проект достаточно объёмный. 

Случай схожий, но более безобидный. Здесь страшных последствий не предвидится, но ровно до тех пор, пока код не будет изменён. Тогда эта завуалированная ошибка и может дать о себе знать, а потом поди найди её... 



Предупреждение анализатора: V561 It's probably better to assign value to 'all_const' variable than to declare it anew. Previous declaration: shader_language.cpp, line 1225. shader_language.cpp 1274

Как я уже упоминал, в чём-то случай схож. Объявляются две переменные с одинаковыми именами и типами, но разными областями видимости. При этом первая переменная используется на протяжении метода, но вот вторая не используется никак (код объёмен, поэтому приводить его не стал, но поверьте вашему верному слуге на слово). Ввиду того, что она объявлена внутри оператора 'if', областью видимости данной переменной будет участок от её объявления до конца блока 'if'. Вот тут-то и притаилась опасность. Бесспорно, в коде, который имеется сейчас, ничего страшного нет - объявляется ненужная переменная, которая никак не используется в области видимости, а после благополучно удаляется - нехорошо, но не критично. Но стоит модифицировать код, добавив использование данной переменной, как тут же могут начаться неприятности, если подразумевалась переменная с большей областью видимости. Как итог - необходимо избегать подобных случаев, даже если на первый взгляд они и выглядят не страшно.

Другой случай - возврат из функций или методов неопределённых значений. Для начала взглянем на код:



Предупреждение анализатора: V614 Potentially uninitialized pointer 'name' used. cp_player_data_control.cpp 244

Не во всех случаях 'name' будет содержать осмысленные значения. У оператора 'if' нет 'else'. Стоит добавить 'else' и присвоить 'name', например 'NULL', или что-то ещё.

Подобная ошибка встретилась ещё раз: V614 Potentially uninitialized pointer 'name' used. cp_player_data_control.cpp 313

Продолжаем обзор. Взглянем на следующий код:



Сообщение анализатора:  V607 Ownerless expression 'm_angularLimits[p_axis].m_limitSoftness'. generic_6dof_joint_sw.cpp 539

Очевидно, что в 'case'-ветви, на которую указал анализатор, пропущено присваивание. Это единственная ветвь в теле данного оператора 'switch', в которой не выполняется присваивание. Скорее всего, правильной была бы запись по аналогии с предыдущими:



Пример с аналогичной ошибкой:



Сообщение анализатора: V607 Ownerless expression 'ie.action.pressed'. variant_op.cpp 2410

В данном методе в зависимости от значения переменной 'str' возвращается то или иное значение. Но, как видно из данного кода, в одной из условных ветвей пропущен оператор 'return', в итоге чего значение 'ie.action.pressed' не возвращается из метода.

Другой пример. На этот раз неправильное использование функции:



Предупреждение анализатора: V576 Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. editor_sample_import_plugin.cpp 705

Здесь расписывать особо нечего. Как видно из сообщения, всё дело в функции 'printf', точнее - в неверной строке форматирования. В итоге значение переменной  'xm_sample' на экран выведено не будет.



Заключение

Если вы дочитали до этого момента, причём именно дочитали, а не просто пролистали - примите мои поздравления и уважение за усидчивость. Эта статья вышла достаточно объёмной, даже с учётом того, что я выписывал не все ошибки. Надеюсь, вы узнали что-то интересное, и впредь будете внимательнее относиться к подобным участкам кода. 

Помимо описанных выше примеров кода и комментариев к ним, я хотел донести ещё одну мысль -  важность использования статических анализаторов кода в крупных проектах. Так или иначе, "узкие" или ошибочные места всегда встречаются, но они могут быть так завуалированы, что обнаружить их становится весьма проблематично, причём, чем дальше,  тем сложнее. И чем раньше будет обнаружена и исправлена ошибка, тем дешевле она обойдётся. Внедрение таких инструментов, как статический анализатор PVS-Studio, использовавшийся для написания данной статьи, облегчает процесс разработки, способствуя скорейшему обнаружению и исправлению ошибок и узких мест проектов, что положительно сказывается на качестве продукта.

P.S.: Версия на английском.