К юбилею выхода шутера от первого лица Serious Sam, который состоялся в марте 2016 года, разработчики игры из хорватской компании Croteam решили открыть исходный код игрового движка Serious Engine 1 v.1.10. Он заинтересовал много разработчиков, которые захотели изучить и улучшить движок. Я тоже решил поучаствовать в улучшении кода и подготовил статью с обзором ошибок, найденных с помощью статического анализатора PVS-Studio.
Serious Engine - игровой движок, разработанный хорватской компанией Croteam. Версия v.1.10 использовалась в играх Serious Sam Classic: The First Encounter и Serious Sam Classic: The Second Encounter. Впоследствии компанией Croteam были разработаны более совершенные игровые движки — Serious Engine 2, Serious Engine 3 и Serious Engine 4, а исходный код движка Serious Engine версии 1.10 был официально открыт и доступен под лицензией GNU General Public License v.2.
Хочу отметить, что проект легко собирается в Visual Studio 2013 и легко проверяется с помощью статического анализатора PVS-Studio 6.02.
V501 There are identical sub-expressions to the left and to the right of the '==' operator: tp_iAnisotropy == tp_iAnisotropy gfx_wrapper.h 180
class CTexParams {
public:
inline BOOL IsEqual( CTexParams tp) {
return tp_iFilter == tp.tp_iFilter &&
tp_iAnisotropy == tp_iAnisotropy && // <=
tp_eWrapU == tp.tp_eWrapU &&
tp_eWrapV == tp.tp_eWrapV; };
....
};Для наглядности я изменил форматирование этого фрагмента кода. Так дефект, который обнаружил анализатор, намного заметней. Переменная сравнивается сама с собой. У объекта с именем 'tp' есть поле 'tp_iAnisotropy', следовательно, по аналогии с соседним кодом часть условия должна выглядеть как "tp_iAnisotropy == tp.tp_iAnisotropy".
V501 There are identical sub-expressions 'GetShadingMapWidth() < 32' to the left and to the right of the '||' operator. terrain.cpp 561
void CTerrain::SetShadowMapsSize(....)
{
....
if(GetShadowMapWidth()<32 || GetShadingMapHeight()<32) {
....
}
if(GetShadingMapWidth()<32 || GetShadingMapWidth()<32) { // <=
tr_iShadingMapSizeAspect = 0;
}
....
PIX pixShadingMapWidth = GetShadingMapWidth();
PIX pixShadingMapHeight = GetShadingMapHeight();
....
}Здесь анализатор обнаружил подозрительный код для проверки ширины и высоты некой карты. Точнее только ширины, потому что в коде присутствуют две одинаковые проверки "GetShadingMapWidth()<32". Скорее всего, условие должно быть таким:
if(GetShadingMapWidth()<32 || GetShadingMapHeight()<32) {
tr_iShadingMapSizeAspect = 0;
}V501 There are identical sub-expressions '(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType)' to the left and to the right of the '&&' operator. worldeditor.h 580
inline BOOL CValuesForPrimitive::operator==(....)
{
return (
(....) &&
(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&// <=
(vfp_plPrimitive == vfpToCompare.vfp_plPrimitive) &&
....
(vfp_bDummy == vfpToCompare.vfp_bDummy) &&
(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&// <=
....
(vfp_fXMin == vfpToCompare.vfp_fXMin) &&
(vfp_fXMax == vfpToCompare.vfp_fXMax) &&
(vfp_fYMin == vfpToCompare.vfp_fYMin) &&
(vfp_fYMax == vfpToCompare.vfp_fYMax) &&
(vfp_fZMin == vfpToCompare.vfp_fZMin) &&
(vfp_fZMax == vfpToCompare.vfp_fZMax) &&
....
);Условие в перегруженном операторе сравнения занимает 35 строк. Неудивительно, что для удобства автор пользовался копированием строк, чтобы ускорить написание кода. Но при таком подходе легко допустить ошибку. Возможно, здесь присутствует лишняя проверка, но может и забыли переименовать скопированную строчку, и оператор сравнения теперь не всегда возвращает правильный результат.
V559 Suspicious assignment inside the condition expression of 'if' operator: pwndView = 0. mainfrm.cpp 697
void CMainFrame::OnCancelMode()
{
// switches out of eventual direct screen mode
CWorldEditorView *pwndView = (....)GetActiveView();
if (pwndView = NULL) { // <=
// get the MDIChildFrame of active window
CChildFrame *pfrChild = (....)pwndView->GetParentFrame();
ASSERT(pfrChild!=NULL);
}
CMDIFrameWnd::OnCancelMode();
}В коде движка присутствует много подозрительных сравнений. Например, в этом фрагменте получают указатель "pwndView", а потом ему присваивают NULL, из-за чего условие всегда ложно.
Скорее всего, здесь хотели написать оператор неравенства '!=' и код должен быть таким:
if (pwndView != NULL) {
// get the MDIChildFrame of active window
CChildFrame *pfrChild = (....)pwndView->GetParentFrame();
ASSERT(pfrChild!=NULL);
}Есть ещё похожий фрагмент кода:
V547 Expression is always false. Probably the '||' operator should be used here. entity.cpp 3537
enum RenderType {
....
RT_BRUSH = 4,
RT_FIELDBRUSH = 8,
....
};
void
CEntity::DumpSync_t(CTStream &strm, INDEX iExtensiveSyncCheck)
{
....
if( en_pciCollisionInfo == NULL) {
strm.FPrintF_t("Collision info NULL\n");
} else if (en_RenderType==RT_BRUSH && // <=
en_RenderType==RT_FIELDBRUSH) { // <=
strm.FPrintF_t("Collision info: Brush entity\n");
} else {
....
}
....
}Одну переменную с именем "en_RenderType" сравнивают с двумя разными константами. Ошибка заключается в том, что используется оператор логического умножения '&&'. Переменная не может быть равна двум константам одновременно, поэтому условие всегда ложно. В этом месте следует использовать оператор '||'.
V559 Suspicious assignment inside the condition expression of 'if' operator: _strModURLSelected = "". menu.cpp 1188
CTString _strModURLSelected;
void JoinNetworkGame(void)
{
....
char strModURL[256] = {0};
_pNetwork->ga_strRequiredMod.ScanF(...., &strModURL);
_fnmModSelected = CTString(strModName);
_strModURLSelected = strModURL; // <=
if (_strModURLSelected="") { // <=
_strModURLSelected = "http://www.croteam.com/mods/Old";
}
....
}Интересная ошибка. В этой функции выполняется некий запрос и в буфер с именем "strModURL" записывается результат (url-ссылка на "мод"). Далее этот результат сохраняется в объект с именем "_strModURLSelected" класса "CTString". Это собственная реализация класса для работы со строками. Из-за опечатки, в условии "if (_strModURLSelected="")" полученная ранее url-ссылка перетирается пустой строкой вместо сравнения с ней. Далее в дело вступает оператор приведения строки к типу const char *. В результате, в условии выполнится сравнение с нулём указателя, который хранит ссылку на пустую строку. Такой указатель всегда неравен нулю. Следовательно, условие всегда будет истинным.
Результатом этого кода будет всегда использование ссылки, которую жёстко прописали в коде. Хотя, планировали использовать эту ссылка как значение по умолчанию.
V547 Expression is always true. Probably the '&&' operator should be used here. propertycombobar.cpp 1853
CEntity *CPropertyComboBar::GetSelectedEntityPtr(void)
{
// obtain selected property ID ptr
CPropertyID *ppidProperty = GetSelectedProperty();
// if there is valid property selected
if( (ppidProperty == NULL) ||
(ppidProperty->pid_eptType != CEntityProperty::EPT_ENTITYPTR) ||
(ppidProperty->pid_eptType != CEntityProperty::EPT_PARENT) )
{
return NULL;
}
....
}Здесь анализатор обнаружил ошибку, противоположную предыдущей. Две проверки переменной "pid_eptType" на неравенство всегда дают истину из-за использования оператора '||'. Следовательно, выход из функции выполняется всегда, независимо от значения указателя "ppidProperty" и переменной "ppidProperty->pid_eptType".
V547 Expression 'ulUsedShadowMemory >= 0' is always true. Unsigned type value is always >= 0. gfxlibrary.cpp 1693
void CGfxLibrary::ReduceShadows(void)
{
ULONG ulUsedShadowMemory = ....;
....
ulUsedShadowMemory -= sm.Uncache(); // <=
ASSERT( ulUsedShadowMemory>=0); // <=
....
}В этом фрагменте кода выполняется небезопасный декремент переменной беззнакового типа, т.к. может возникнуть переполнение переменной "ulUsedShadowMemory". При этом рядом присутствует Assert(), который никогда не выдаст предупреждение. Очень подозрительное место, которое разработчикам следует проверить.
V704 'this != 0' expression should be avoided - this expression is always true on newer compilers, because 'this' pointer can never be NULL. entity.h 697
inline void CEntity::AddReference(void) {
if (this!=NULL) { // <=
ASSERT(en_ctReferences>=0);
en_ctReferences++;
}
};В коде движка присутствует 28 сравнений 'this' с нулём. Код писался давно, но согласно современному стандарту языка C++, указатель 'this' не может быть нулевым, следовательно, компилятор может выполнить оптимизацию и удалить проверку. В более сложных условиях это может приводить к неожиданным ошибкам. С примерами можно ознакомиться в документации к диагностике.
Конкретно Visual C++ пока так себя не ведёт. Но это дело времени. Такой код более вне закона.
V547 Expression 'achrLine != ""' is always true. To compare strings you should use strcmp() function. worldeditor.cpp 2254
void CWorldEditorApp::OnConvertWorlds()
{
....
char achrLine[256]; // <=
CTFileStream fsFileList;
// count lines in list file
try {
fsFileList.Open_t( fnFileList);
while( !fsFileList.AtEOF()) {
fsFileList.GetLine_t( achrLine, 256);
// increase counter only for lines that are not blank
if( achrLine != "") ctLines++; // <=
}
fsFileList.Close();
}
....
}Анализатор обнаружил неправильное сравнение строки с пустой строкой. Ошибка заключается в том, что проверка (achrLine != "") всегда истинна и инкремент переменной "ctLines" выполняется всегда. Хотя в комментарии написано, что это должно выполняться только для непустых строк.
Такое поведение вызвано тем, что в этом условии сравнивают два указателя: "achrLine" и указатель на временную пустую строку. Такие указатели никогда не будут равны.
Правильный код с использованием функции strcmp():
if(strcmp(achrLine, "") != 0) ctLines++;Есть ещё два таких неправильных сравнения:
V541 It is dangerous to print the string 'achrDefaultScript' into itself. dlgcreateanimatedtexture.cpp 359
BOOL CDlgCreateAnimatedTexture::OnInitDialog()
{
....
// allocate 16k for script
char achrDefaultScript[ 16384];
// default script into edit control
sprintf( achrDefaultScript, ....); // <=
....
// add finishing part of script
sprintf( achrDefaultScript, // <=
"%sANIM_END\r\nEND\r\n", // <=
achrDefaultScript); // <=
....
}Здесь формируют некую строку в буфере. Потом хотят получить новую строку, сохранив предыдущее значение строки, и добавить к ней ещё два слова. Вроде всё просто.
Для объяснения, почему здесь может получиться неожиданный результат, я процитирую простой пример из документации к этой диагностике:
char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s);В результате работы этого кода хочется получить строку:
N = 123, S = testНо на практике в буфере будет сформирована строка:
N = 123, S = N = 123, S =В подобных ситуациях аналогичный код может привести не только к выводу некорректного текста, но и к аварийному завершению программы. Код может быть исправлен, если использовать для сохранения результата новый буфер. Безопасный вариант:
char s1[100] = "test";
char s2[100];
sprintf(s2, "N = %d, S = %s", 123, s1);Аналогично, стоит поступить и в коде Serious Engine. Хотя благодаря везению код может работать, будет безопасней использовать дополнительный буфер для формирования строки.
V579 The qsort function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. mesh.cpp 224
// optimize lod of mesh
void CMesh::OptimizeLod(MeshLOD &mLod)
{
....
// sort array
qsort(&_aiSortedIndex[0] // <=
ctVertices
sizeof(&_aiSortedIndex[0]), // <=
qsort_CompareArray);
....
}Функция qsort() принимает третьим аргументом размер элемента сортируемого массива. Очень подозрительно, что туда всегда передают размер указателя. Скорее всего кто-то скопировал первый аргумент функции в третий, но забыл стереть символ взятия адреса.
V607 Ownerless expression 'pdecDLLClass->dec_ctProperties'. entityproperties.cpp 107
void CEntity::ReadProperties_t(CTStream &istrm) // throw char *
{
....
CDLLEntityClass *pdecDLLClass = en_pecClass->ec_pdecDLLClass;
....
// for all saved properties
for(INDEX iProperty=0; iProperty<ctProperties; iProperty++) {
pdecDLLClass->dec_ctProperties; // <=
....
}
....
}Непонятно, что делает выделенная строчка кода. Вернее, понятно, что как раз ничего. Поле класса никак не используется. Возможно это ошибка возникла после рефакторинга или просто строчка осталась после отладки.
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 2)' is negative. layermaker.cpp 363
void CLayerMaker::SpreadShadowMaskOutwards(void)
{
#define ADDNEIGHBOUR(du, dv) \
if ((pixLayerU+(du)>=0) \
&&(pixLayerU+(du)<pixLayerSizeU) \
&&(pixLayerV+(dv)>=0) \
&&(pixLayerV+(dv)<pixLayerSizeV) \
&&(pubPolygonMask[slOffsetMap+(du)+((dv)<<pixSizeULog2)])) {\
.... \
}
ADDNEIGHBOUR(-2, -2); // <=
ADDNEIGHBOUR(-1, -2); // <=
.... // <=
}Макрос "ADDNEIGHBOUR" объявлен в теле функции и используется подряд 28 раз. В этот макрос передают отрицательные числа, где выполняется их сдвиг. Согласно современным стандартам языка C и C++, сдвиг отрицательного числа приводит к неопределённому поведению.
V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. sessionstate.cpp 1191
void CSessionState::ProcessGameStream(void)
{
....
if (res==CNetworkStream::R_OK) {
....
} if (res==CNetworkStream::R_BLOCKNOTRECEIVEDYET) { // <=
....
} else if (res==CNetworkStream::R_BLOCKMISSING) {
....
}
....
}Глядя на оформление кода можно предположить, что в каскаде условий действительно пропущено ключевое слово 'else'.
Ещё такое место:
V595 The 'pAD' pointer was utilized before it was verified against nullptr. Check lines: 791, 796. anim.cpp 791
void CAnimObject::SetData(CAnimData *pAD) {
// mark new data as referenced once more
pAD->AddReference(); // <=
// mark old data as referenced once less
ao_AnimData->RemReference();
// remember new data
ao_AnimData = pAD;
if( pAD != NULL) StartAnim( 0); // <=
// mark that something has changed
MarkChanged();
}В завершение статьи хочу привести пример ошибки с потенциальным разыменованием нулевого указателя. Прочитав предупреждение анализатора, в этой небольшой функции легко заметить, как опасно используется указатель "pAD". Почти сразу после вызова "pAD->AddReference()" выполняется проверка "pAD != NULL", что говорит о возможной передаче нулевого указателя в эту функцию.
Весь список найденных опасных мест с указателями:
Проверка игрового движка Serious Engine 1 v.1.10 показала, что ошибки в коде проектов могут жить очень долго и даже отмечать юбилей! В статью вошли только некоторые самые интересные примеры из отчёта анализатора. Много предупреждений я привёл просто списком. Но весь отчёт содержит довольно много предупреждений для такого маленького проекта. У компании Croteam есть более совершенные игровые движки — Serious Engine 2, Serious Engine 3 и Serious Engine 4. Боюсь представить, сколько опасного кода могло перекочевать в новые серии движка. Я надеюсь, что после прочтения статьи, разработчики воспользуются статическим анализатором PVS-Studio в своих проектах и будут радовать пользователей качественными играми. Ведь анализатор легко скачать, легко запустить в Visual Studio, а для любых других сборочных систем есть утилита Standalone.