# Система в шоке: интересные ошибки в исходных кодах легендарного System Shock Меня зовут Виктория, и недавно я влилась в команду PVS\-Studio в качестве С\+\+ программиста\. Один из способов познакомиться с анализатором и его диагностиками \- это проверить какой\-то проект и подробно разобраться в предупреждениях, которые он выдаёт\. А раз я этим занялась, то будет не лишним оформить результаты проверки в виде статьи\. Поэтому предлагаю вашему вниманию обзор кода System Shock\. Приятного чтения\. ![0575_SystemShock_ru/image1.png](https://import.viva64.com/docx/blog/0575_SystemShock_ru/image1.png) ## "How can you challenge a perfect immortal machine?" Не так давно был опубликован исходный код легендарной игры System Shock\. Того самого киберпанк\-шутера, повлиявшего в свое время на дальнейшее развитие всего направления приключенческих боевиков и ставшего предтечей такой игры, как Bioshock, а также вдохновившего многие сюжетные и игровые решения: Metal Gear Solid, Resident Evil и даже Half\-Life\. Возможно, это было сделано для привлечения внимания к многообещающему [ремейку](http://systemshock.com/) оригинальной первой части, дела у которого похоже [идут не лучшим образом](https://www.polygon.com/2018/2/16/17016744/system-shock-reboot-hiatus-nightdive-interview)\. Поэтому, когда передо мной встал выбор, какой проект проверить с помощью PVS\-Studio, я не смогла пройти мимо такого титана игровой индустрии\. Конечно же сложно избежать ошибок в столь крупном проекте\. Есть достаточное количество примеров, когда даже крайне надежные системы несут в себе различные недостатки\. Чего стоит только ошибка, из\-за которой [$370 000 000 взлетели на воздух](https://pvs-studio.ru/ru/blog/posts/cpp/0426/)\. И игровые проекты также не избегают этой участи\. С наиболее интересными ошибками, найденными с помощью PVS\-Studio в сфере видеоигр вы можете ознакомиться в нашей недавней статье "[Статический анализ в видеоигровой индустрии: топ\-10 программных ошибок](https://pvs-studio.ru/ru/blog/posts/cpp/0570/)"\. В этой статье были собраны некоторые ошибки исходного кода игры, доступ к которому открыт для всех желающих на [GitHub](https://github.com/nightdive-studio/shockmac)\. Спору нет, проект старый\. Тем не менее, на чужих ошибках стоит поучиться, особенно с учетом того, что большинство недочетов достаточно типичны и повторяются во многих проектах, да еще и могут привести к серьезным последствиям\. ## Логический или побитовый операнд? **Предупреждение PVS\-Studio:** [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: 0xffff0000\. INTERP\.C 355 ```cpp temp = (((ulong) _view_position.gX)>>16); // get high 16 bits if (((temp<>16); // get high 16 bits if (((temp<>16); // get high 16 bits if (((temp< 0'\. TMAP\.C 221 ```cpp for (i=nverts; i--; i>0) { .... } ``` ![0575_SystemShock_ru/image2.png](https://import.viva64.com/docx/blog/0575_SystemShock_ru/image2.png) В данном случае ошибка состоит в самом синтаксисе оператора _for_: перепутано положение 2\-го и 3\-го подвыражений\. И это не единственная ошибка такого рода: **Предупреждение PVS\-Studio**: [V607](https://pvs-studio.ru/ru/docs/warnings/v607/) Ownerless expression 'i \>\= 0'\. INTERP\.C 366 ```cpp for (i=N_RES_POINTS-1; i--; i>=0) ....; ``` Аналогичные предупреждения: **Предупреждения PVS\-Studio**: * V607 Ownerless expression 'i \> 0'\. TMAP\.C 532 * V607 Ownerless expression 'i \> 0'\. POLYGON\.C 77 * V607 Ownerless expression 'i \> 0'\. POLYGON\.C 268 ## Учтено далеко не всё **Предупреждения PVS\-Studio:** * [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Potentially uninitialized pointer 'pc1' used\. AI\.C 597 * [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Potentially uninitialized pointer 'pc2' used\. AI\.C 609 ```cpp typedef enum ObjClass { CLASS_GUN, CLASS_AMMO, CLASS_PHYSICS, .... CLASS_CRITTER, .... } ObjClass; errtype do_random_loot(ObjID corpse){ int *pc1, *pc2; if (....) { switch (objs[corpse].obclass) { case CLASS_CONTAINER: .... *pc1 = 0; *pc2 = 0; break; case CLASS_SMALLSTUFF: .... pc1 = &objSmallstuffs[osid].data1; pc2 = &objSmallstuffs[osid].data2; break; } if (*pc1 == 0) { .... } if (*pc2 == 0) { .... } } .... } ``` Переменным pc1 и pc2 не во всех случаях были присвоены значения, так как не все варианты поведения были приняты во внимание\. Так, конкретно здесь, _objs\[corpse\]\.obclass_ может принимать куда больше значений, чем _CLASS\_CONTAINER_ или _CLASS\_SMALLSTUFF_\. Если _objs\[corpse\]\.obclass_ примет иные значения, то указатели _pc1_ и _pc2_ останутся неинициализированными, и их разыменование ниже приведет к неопределенному поведению\. ## Проверка на выход за границу массива \+ проверка на ненулевой указатель **Предупреждение PVS\-Studio:** [V781](https://pvs-studio.ru/ru/docs/warnings/v781/) The value of the 'num\_args' variable is checked after it was used\. Perhaps there is a mistake in program logic\. Check lines: 224, 225\. FIX24TST\.C 224 ```cpp #define MAX_ARGS 8 .... bool args_neg[MAX_ARGS]; .... void parse (char *str, bool command) { .... args_neg[num_args] = neg = FALSE; if (num_args == MAX_ARGS) break; .... } ``` Логическая ошибка, способная понести за собой выход за границы массива\. Проверка на границу должна происходить до доступа к элементу массива\. Похожие случаи: **Предупреждение PVS\-Studio:** [V781](https://pvs-studio.ru/ru/docs/warnings/v781/) The value of the 'model\_num' variable is checked after it was used\. Perhaps there is a mistake in program logic\. Check lines: 567, 569\. RENDTOOL\.C 567 ```cpp uchar model_base_nums[MAX_VTEXT_OBJS];.... void load_model_vtexts(char model_num){ short curr = model_base_nums[model_num]; .... if (model_num >= MAX_VTEXT_OBJS) return; } ``` **Предупреждение PVS\-Studio:** [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'ch' pointer was utilized before it was verified against nullptr\. Check lines: 200, 202\. HOTKEY\.C 200 ```cpp hotkey_link *chain = (hotkey_link*)(ch->keychain.vec); if (ch == NULL) return FALSE; ``` И ещё несколько аналогичных предупреждений, для которых я не буду приводить код: **Предупреждения PVS\-Studio:** * V595 The 'ch' pointer was utilized before it was verified against nullptr\. Check lines: 381, 392\. EVENT\.C 381 * V595 The 'dp' pointer was utilized before it was verified against nullptr\. Check lines: 2508, 2522\. INVENT\.C 2508 * V595 The 'mug' pointer was utilized before it was verified against nullptr\. Check lines: 702, 704\. EMAIL\.C 702 ## Как можно больше комментирования ![0575_SystemShock_ru/image3.png](https://import.viva64.com/docx/blog/0575_SystemShock_ru/image3.png) **Предупреждение PVS\-Studio:** [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'len <\= 0' is always true\. COMPOSE\.C 235 ```cpp len = 0; // len = ....; // .... if (len <= 0) { .... } ``` Один из недочетов, встречающийся на протяжении всего кода, это использование переменных, которые были изменены внутри закомментированного блока\. Их использование при проверке некоторых условий в итоге теряет смысл\. Возможен и другой сценарий: **Предупреждение PVS\-Studio:** [V785](https://pvs-studio.ru/ru/docs/warnings/v785/) Constant expression in switch statement\. BitmapTest\.C 198 ```cpp c = 0; //if (....) c = evt.message & charCodeMask; switch (c) { case 'i': .... break; .... case 'O': .... break; default: break; } ``` В случае, если закомментированный код не нужен, то можно упростить код, убрав условные операторы\. Однако в некоторых ситуациях проблема может быть серьезнее: **Предупреждение PVS\-Studio**: [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized variable 'err' used\. EVENT\.C 953 ```cpp errtype err; .... // err = ui_init_cursors(); .... if (err != OK) return err; ``` Из\-за того, что закомментировали код, переменная _err_ будет не инициализирована, и её использование приводит к неопределенному поведению\. Но комментарии не ограничивались только скрытием "ненужных" блоков кода и пояснениями\. То тут, то там внезапно встречаются шутливые и ироничные замечания, и даже стихи\. ```cpp // I'll give you fish, I'll give you candy, // I'll give you, everything I have in my hand ``` ```cpp // it's a wonderful world, with a lot of strange men // who are standing around, and they all wearing towels // Returns whether or not in the humble opinion of the // sound system, the sample should be politely obliterated // out of existence // that kid from the wrong side came over my house again, // decapitated all my dolls // and if you bore me, you lose your soul to me // - "Gepetto", Belly, _Star_ // And here, ladies and gentlemen, // is a celebration of C and C++ and their untamed passion... // ================== TerrainData terrain_info; // Now the actual stuff... // ======================= // this is all outrageously horrible, as we dont know what // we really need to deal with here // And if you thought the hack for papers was bad, // wait until you see the one for datas... - X ``` Это конечно не ошибка, но мне показалась, что читателю будет интересно познакомиться с некоторыми из комментариев :\)\. ## Побитовый сдвиг отрицательного числа **Предупреждение PVS\-Studio**: [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<'\. The left operand is negative \('\(\(rand\(\) % 4000\) \- 2000\)' \= \[\-2000\.\.1999\]\)\. STAR\.C 407 ```cpp v.gX = ((rand()%4000) - 2000) << 8; v.gY = ((rand()%4000) - 2000) << 8; v.gZ = ((rand()%4000) - 2000) << 8; ``` Один из примеров неопределенного поведения в побитовых операциях\. Здесь, _rand\(\)%4000_ возвращает значение в промежутке \[0\.\.\.3999\]\. Далее этот интервал сдвигается на 2000, и мы получаем значение в диапазоне \[\-2000\.\.1999\]\. Согласно стандарту С и С\+\+, побитовый сдвиг отрицательного числа приводит к неопределенному поведению\. Аналогичный случай: **Предупреждение PVS\-Studio**: [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<'\. The left operand is negative \('\(axis\_x \- 1\)' \= \[\-1\.\.2147483646\]\)\. ALLOC\.C 122 ```cpp short g3_init(short max_points,int user_x_axis,int user_y_axis,int user_z_axis){ .... long axis_x; .... if (user_x_axis<0) { user_x_axis = -user_x_axis; } .... axis_x = user_x_axis; .... axis_x_ofs = ((axis_x-1)<<1) + (axis_x-1); .... } ``` Значение _axis\_x_ в результате преобразований может принимать значения из промежутка \[0\.\. 2147483647\]\. В случае, если _axis\_x \= 0_, _\(axis\_x\-1\)_ примет значение \-1, что приведет к описанному выше неопределенному поведению\. И идентичные случаи для осей Y и Z: **Предупреждения PVS\-Studio**: * [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<'\. The left operand is negative \('\(axis\_y \- 1\)' \= \[\-1\.\.2147483646\]\)\. ALLOC\.C 123 * [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<'\. The left operand is negative \('\(axis\_z \- 1\)' \= \[\-1\.\.2147483646\]\)\. ALLOC\.C 124 ## Ловушка копипасты **Предупреждение PVS\-Studio:** [V751](https://pvs-studio.ru/ru/docs/warnings/v751/) Parameter 'Y' is not used inside function body\. BTEST\.C 67 ```cpp fix Terrain( fix X, fix Y, int deriv ) { if( deriv == 0 ) return fix_mul(...., (X - ....) ); if( deriv == 1 ) return fix_mul(...., (X - ....) ); if( deriv == 2 ) return 0; return 0; } ``` Судя по тому, что в функцию подается и _X_, и _Y_, а также два разных условия имеют одинаковое тело, можно предположить, что второе условие должно было использовать _Y_, но при копировании схожих строк кода этот момент был упущен\. ## Break **Предупреждение PVS\-Studio:** [V796](https://pvs-studio.ru/ru/docs/warnings/v796/) It is possible that 'break' statement is missing in switch statement\. OLH\.C 142 ```cpp switch (objs[obj].obclass) { case CLASS_DOOR: .... break; case CLASS_BIGSTUFF: .... if (....) { .... break; } case CLASS_SMALLSTUFF: .... if (....) { .... break; } // smallstuff falls through to default. default: .... break; } ``` В обеих ветках _switch_ _break_ присутствует внутри условий, и, в результате, если ни одно из них не будет выполнено, произойдет fallthrough\. Во втором случае указано, что так и нужно, но в первом такого комментария нет, следовательно, весьма вероятно, что это логическая ошибка\. Подобное предупреждение: **Предупреждение PVS\-Studio**: * V796 It is possible that 'break' statement is missing in switch statement\. GAMEREND\.C 777 ## Приоритет операций и неблагополучный макрос **Предупреждение PVS\-Studio:** [V634](https://pvs-studio.ru/ru/docs/warnings/v634/) The priority of the '\-' operation is higher than that of the '<<' operation\. It's possible that parentheses should be used in the expression\. FRCLIP\.C 256 ```cpp #define span_right(y,s) \ (x_span_lists[((y)<frpipe_dist) .... .... } ``` При работе препроцессора, получится код следующего вида: ```cpp x_span_lists[((y)<