Хабрахабр

VVVVVV??? VVVVVV!!! :)

Если вы читаете этот текст – значит, вы либо подумали, что с заголовком статьи что-то не то, либо увидели в нём название знакомой компьютерной игры. VVVVVV – это инди-игра в жанре «платформер», завоевавшая сердца многих игроков своей приятной внешней простотой и не менее приятной внутренней сложностью. Несколько дней назад VVVVVV исполнилось 10 лет, и автор игры – Terry Cavanagh – отметил этот праздник публикацией её исходного кода. Что же «вкусненького» можно в нём найти? Ответ читайте в данной статье.

Рисунок 1

Введение

Ох, VVVVVV… Помню, как наткнулся на неё вскоре после релиза, и, будучи большим любителем пиксельных ретро-игр, с радостью установил её себе на компьютер. Помню свои первые впечатления: «И что, это всё? Просто бегать по квадратным комнатам?» – подумал я после нескольких минут игры. Я тогда еще не знал, что меня ждёт. Стоило мне выйти из стартовой локации, как я оказался в небольшом, но запутанном и витиеватом двумерном мире, полном необычных ландшафтов и неизвестных мне пиксельных артефактов.

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

Вернемся к исходному коду, выложенному в честь юбилея игры.

Помимо непосредственно разработки, мы также занимаемся и продвижением нашего продукта. На данный момент я являюсь C++-разработчиком в команде PVS-Studio – статического анализатора кода для C, C++, C# и Java. Наши читатели получают интересные статьи на тематику программирования, а мы получаем возможность наглядно продемонстрировать возможности PVS-Studio. Для нас одним из лучших способов делать это является написание статей о проверке проектов с открытым исходным кодом. Поэтому, узнав об открытии исходного кода VVVVVV, я просто не смог пройти мимо.

Возвращайте вектор гравитации в положение «вниз» и усаживайтесь поудобнее: мы начинаем! В данной статье мы рассмотрим некоторые интересные ошибки, найденные анализатором PVS-Studio в коде VVVVVV, а также проведём детальный разбор этих ошибок.

Обзор предупреждений, выданных анализатором

Предупреждение 1

V512 A call of the 'sprintf' function will lead to overflow of the buffer 'fileSearch'. FileSystemUtils.cpp 307

#define MAX_PATH 260 .... void PLATFORM_migrateSaveData(char *output)
{ char oldLocation[MAX_PATH]; char newLocation[MAX_PATH]; char oldDirectory[MAX_PATH]; char fileSearch[MAX_PATH]; .... /* Same place, different layout. */ strcpy(oldDirectory, output); sprintf(fileSearch, "%s\\*.vvvvvv", oldDirectory); ....
}

Как можно заметить, строки fileSearch и oldDirectory имеют одинаковый размер: 260 символов. Строка форматирования (третий аргумент sprintf) после подстановки в нее содержимого строки oldDirectory будет иметь вид:

<i>содержимое_oldDirectory\*.vvvvvv</i>

Эта строка на 9 символов длиннее, чем исходное значение oldDirectory. Именно эта последовательность символов и будет далее записана в fileSearch. Что же случится, если длина строки oldDirectory будет больше 251? Результирующая строка окажется длиннее, чем сможет вместить в себя fileSearch, что приведет к записи за пределы массива. Какие данные в оперативной памяти могут быть повреждены и к какому результату это приведет – это вопрос риторический 🙂

Предупреждение 2

V519 The 'background' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1367, 1373. Map.cpp 1373

void mapclass::loadlevel(....)
{ .... case 4: //The Warpzone tmap = warplevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); roomname = warplevel.roomname; tileset = 1; background = 3; // <= dwgfx.rcol = warplevel.rcol; dwgfx.backgrounddrawn = false; warpx = warplevel.warpx; warpy = warplevel.warpy; background = 5; // <= if (warpy) background = 4; if (warpx) background = 3; if (warpx && warpy) background = 5; break; ....
}

Одной и той же переменной два раза подряд присваивается какое-то значение. При этом между присвоениями эта переменная нигде не используется. Странно… Возможно, такая последовательность не нарушает логику работы программы, но такие присвоения сами по себе говорят о некоторой путанице при написании кода. Является ли это ошибкой на самом деле – сможет сказать только автор. Хотя в коде есть и более яркие примеры этой ошибки:

void Game::loadquick(....)
....
}

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

Предупреждение 3

V808 'pKey' object of 'basic_string' type was created but was not utilized. editor.cpp 1866

void editorclass::load(std::string &_path)
{ .... std::string pKey(pElem->Value()); .... if (pKey == "edEntities") { int i = 0; for (TiXmlElement *edEntityEl = pElem->FirstChildElement(); edEntityEl; edEntityEl = edEntityEl->NextSiblingElement()) { std::string pKey(edEntityEl->Value()); // <= //const char* pText = edEntityEl->GetText() ; if (edEntityEl->GetText() != NULL) { edentity[i].scriptname = std::string(edEntityEl->GetText()); } edEntityEl->QueryIntAttribute("x", &edentity[i].x); edEntityEl->QueryIntAttribute("y", &edentity[i].y); edEntityEl->QueryIntAttribute("t", &edentity[i].t); edEntityEl->QueryIntAttribute("p1", &edentity[i].p1); edEntityEl->QueryIntAttribute("p2", &edentity[i].p2); edEntityEl->QueryIntAttribute("p3", &edentity[i].p3); edEntityEl->QueryIntAttribute("p4", &edentity[i].p4); edEntityEl->QueryIntAttribute("p5", &edentity[i].p5); edEntityEl->QueryIntAttribute("p6", &edentity[i].p6); i++; } EditorData::GetInstance().numedentities = i; } ....
}

Очень странный код. Анализатор предупреждает о созданной, но не использованной переменной pKey, но на деле проблема оказалась интереснее. Я специально отметил стрелкой строку, на которую было выдано предупреждение, потому что данная функция содержит более одного определения строки с именем pKey. Да-да, внутри цикла for объявляется еще одна такая переменная, и своим именем она перекрывает ту, что объявлена вне цикла.

Перекрытие имён – это достаточно грубая ошибка, которую бывает весьма тяжело отыскать самостоятельно в ходе обзора кода. Таким образом, если вы обратитесь к значению строки pKey вне цикла for, вы получите значение, равное pElem->Value(), но если сделать то же самое уже внутри цикла, то вы получите значение, равное edEntityEl->Value().

Предупреждение 4

V805 Decreased performance. It is inefficient to identify an empty string by using 'strlen(str) > 0' construct. A more efficient way is to check: str[0] != '\0'. physfs.c 1604

static char *prefDir = NULL; .... const char *PHYSFS_getPrefDir(const char *org, const char *app)
{ .... assert(strlen(prefDir) > 0); ... return prefDir;
} /* PHYSFS_getPrefDir */

Анализатор обнаружил место для потенциальной микрооптимизации. В нем для проверки C-style строки на пустоту используется функция strlen. Эта функция «пробегает» все элементы строки и проверяет каждый из них на равенство нуль-терминалу ('\0'). Если на вход подается большая строка, то каждый её символ всё равно будет сравнён со строковым нулём.

Для этого достаточно только узнать, является ли первый символ строки нуль-терминалом. Но ведь нам нужно только проверить, что строка непустая! Поэтому для оптимизации данной проверки внутри assert стоит написать:

str[0] != '\0'

Именно такую рекомендацию и даёт нам анализатор. Конечно, вызов функции strlen находится в условии макроса assert, поэтому он будет выполняться только в отладочной версии, где скорость не столь важна. В релизной версии вызов функции исчезнет и код будет работать быстро. Тем не менее, мне хотелось продемонстрировать возможности анализатора по предложению микрооптимизаций.

Предупреждение 5

Чтобы показать следующую ошибку, мне необходимо прикрепить сюда два участка кода: объявление класса entclass и его конструктор. Начнем с объявления:

class entclass
{
public: entclass(); void clear(); bool outside(); public: //Fundamentals bool active, invis; int type, size, tile, rule; int state, statedelay; int behave, animate; float para; int life, colour; //Position and velocity int oldxp, oldyp; float ax, ay, vx, vy; int cx, cy, w, h; float newxp, newyp; bool isplatform; int x1, y1, x2, y2; //Collision Rules int onentity; bool harmful; int onwall, onxwall, onywall; //Platforming specific bool jumping; bool gravity; int onground, onroof; int jumpframe; //Animation int framedelay, drawframe, walkingframe, dir, actionframe; int yp; int xp;
};

Конструктор этого класса выглядит вот так:

entclass::entclass()
{ clear();
} void entclass::clear()
{ // Set all values to a default, // required for creating a new entity active = false; invis = false; type = 0; size = 0; tile = 0; rule = 0; state = 0; statedelay = 0; life = 0; colour = 0; para = 0; behave = 0; animate = 0; xp = 0; yp = 0; ax = 0; ay = 0; vx = 0; vy = 0; w = 16; h = 16; cx = 0; cy = 0; newxp = 0; newyp = 0; x1 = 0; y1 = 0; x2 = 320; y2 = 240; jumping = false; gravity = false; onground = 0; onroof = 0; jumpframe = 0; onentity = 0; harmful = false; onwall = 0; onxwall = 0; onywall = 0; isplatform = false; framedelay = 0; drawframe = 0; walkingframe = 0; dir = 0; actionframe = 0;
}

Достаточно много полей, не правда ли? Неудивительно, что здесь затаилась ошибка, на которую PVS-Studio выдал предупреждение:

Consider inspecting: oldxp, oldyp. V730 It is possible that not all members of a class are initialized inside the constructor. Ent.cpp 3

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

Рисунок 2

Предупреждение 6

Рассмотрим код:

void mapclass::loadlevel(....)
{ .... std::vector<std::string> tmap; .... tmap = otherlevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); .... // Вектор tmap изменяется еще много раз.
}

Предупреждение PVS-studio: V688 The 'tmap' local variable possesses the same name as one of the class members, which can result in a confusion. Map.cpp 1192

Действительно, если заглянуть внутрь класса mapclass, то можно обнаружить там такой же вектор с таким же именем:

class mapclass
{
public: .... std::vector <int> roomdeaths; std::vector <int> roomdeathsfinal; std::vector <int> areamap; std::vector <int> contents; std::vector <int> explored; std::vector <int> vmult; std::vector <std::string> tmap; // <= ....
};

К сожалению, объявление вектора с таким же именем внутри функции делает невидимым вектор, объявленный в классе. Получается, что на протяжении всей функции loadlevel вектор tmap изменяется только внутри функции. Вектор, объявленный в классе, остается неизменным!

По большей части они связаны со временными переменными, которые «для удобства» были объявлены, как члены класса. Занимательно, что PVS-Studio обнаружил аж целых 20 таких фрагментов кода! Вы можете прочитать об этом в посте, ссылку на который я прикрепил в начале этой статьи. Автор игры (и единственный её разработчик) сам писал, что раньше имел эту плохую привычку.

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

Предупреждение 7

V601 The integer type is implicitly cast to the char type. Game.cpp 4997

void Game::loadquick(....)
{ .... else if (pKey == "totalflips") { totalflips = atoi(pText); } else if (pKey == "hardestroom") { hardestroom = atoi(pText); // <= } else if (pKey == "hardestroomdeaths") { hardestroomdeaths = atoi(pText); } ....
}

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

//Some stats:
int totalflips;
std::string hardestroom;
int hardestroomdeaths;

Переменные totalflips и hardestroomdeaths имеют целочисленный тип, и поэтому присвоить в них результат функции atoi – совершенно нормально. Но что будет, если присвоить целочисленное значение в std::string? Оказывается, с точки зрения языка такое присвоение совершенно валидно. В итоге в переменную hardestroom будет записано какое-то непонятное значение!

Предупреждение 8

V1004 The 'pElem' pointer was used unsafely after it was verified against nullptr. Check lines: 1739, 1744. editor.cpp 1744

void editorclass::load(std::string &_path)
{ .... TiXmlHandle hDoc(&doc); TiXmlElement *pElem; TiXmlHandle hRoot(0); version = 0; { pElem = hDoc.FirstChildElement().Element(); // should always have a valid root // but handle gracefully if it does if (!pElem) { printf("No valid root! Corrupt level file?\n"); } pElem->QueryIntAttribute("version", &version); // <= // save this for later hRoot = TiXmlHandle(pElem); } ....
}

Анализатор предупреждает, что указатель pElem небезопасно используется сразу после того, как он был проверен на nullptr. Чтобы убедиться, что анализатор прав, заглянем в определение функции Element(), возвращаемым значением которой инициализируется указатель pElem:

/** @deprecated use ToElement. Return the handle as a TiXmlElement. This may return null.
*/
TiXmlElement *Element() const
{ return ToElement();
}

Как видно по комментарию, эта функция может вернуть null.

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

Предупреждение 9

На следующий участок кода PVS-Studio выдал четыре предупреждения:

  • V560 A part of conditional expression is always true: x >= 0. editor.cpp 1137
  • V560 A part of conditional expression is always true: y >= 0. editor.cpp 1137
  • V560 A part of conditional expression is always true: x < 40. editor.cpp 1137
  • V560 A part of conditional expression is always true: y < 30. editor.cpp 1137

int editorclass::at( int x, int y )
{ if(x<0) return at(0,y); if(y<0) return at(x,0); if(x>=40) return at(39,y); if(y>=30) return at(x,29); if(x>=0 && y>=0 && x<40 && y<30) { return contents[x+(levx*40)+vmult[y+(levy*30)]]; } return 0;
}

Все предупреждения относятся к последнему if-выражению. Проблема в том, что все четыре проверки, которые в нём выполняются, всегда будут возвращать true. Не сказал бы, что это серьезная ошибка, но получилось довольно забавно. Автор решил отнестись к этой функции серьезно и на всякий случай проверил каждую переменную еще раз 🙂

поток выполнения всё равно никогда не дойдет до выражения "return 0;". Данную проверку можно убрать, т.к. Хоть это и не изменит логики программы, но освободит её от лишних проверок и мертвого кода.

Предупреждение 10

В своей статье про юбилей игры Терри с иронией рассказывает, что одним из элементов, управляющих логикой игры, был огромный switch из функции Game::updatestate(), отвечающий сразу за большое количество различных состояний игры. И было довольно ожидаемо, что обнаружу следующее предупреждение:

Consider refactoring the 'Game::updatestate' function. V2008 Cyclomatic complexity: 548. Game.cpp 612

Пятьсот сорок восемь!!! Да, вы все правильно поняли: PVS-Studio оценил цикломатическую сложность функции в 548 единиц. И это при том, что, по сути, кроме switch-выражения в функции ничего больше нет. Вот это я понимаю – «опрятный код». В самом switch я насчитал более 300 case-выражений.

Я бы с радостью привел сюда весь код функции (3450 строк), но такая победа была бы нечестной, поэтому я ограничусь просто ссылкой на громадный switch. У нас в офисе идет небольшое соревнование между авторами за самую длинную статью. Кстати, помимо Game::updatestate(), PVS-Studio нашел еще целых 44 функции с завышенной цикломатической сложностью, 10 из которых имеют сложность более 200. Рекомендую перейти по ней и оценить весь масштаб самостоятельно!

Рисунок 3

Заключение

Думаю, выписанных ошибок достаточно для статьи. Да, ошибок в проекте оказалось немало, но в этом как раз и фишка: выложив свой код, Terry Cavanagh показал, что не обязательно быть хорошим программистом, чтобы сделать хорошую игру. Сейчас, спустя 10 лет, Терри вспоминает те времена с иронией. Важно уметь учиться на своих ошибках, и практика – лучший способ делать это. А если ваша практика еще может породить такую игру, как VVVVVV, то это вообще шикарно! Эхх… пойду-ка я, наверное, еще в неё поиграю 🙂

Если вы хотите самостоятельно посмотреть, что еще можно найти – предлагаю скачать и попробовать PVS-Studio! Это были не все ошибки, найденные в коде игры. Также не забываете, что для открытых проектов мы предоставляем бесплатную лицензию.

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш адрес email не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть