Автор: Святослав Размыслов
В марте 2016 года шутер от первого лица «Серьезный Сэм» отпраздновал годовщину выхода. В честь этого разработчики из хорватской компании Croteam решили открыть исходный код игрового движка Serious Engine 1 v.1.10. Это вызвало интерес большого количества разработчиков, которые получили возможность ознакомиться с кодом и улучшить его. Я тоже решил поучаствовать в улучшении кода и написал статью с обзором ошибок, которые обнаружил анализатор PVS-Studio.
Введение
Serious Engine — игровой движок, разработанный хорватской компанией Croteam. V 1.1o и использовался в играх 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 Слева и справа от оператора == одинаковые подвыражения: 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.
V501 Слева и справа от оператора || одинаковые подвыражения GetShadingMapWidth() ‹ 32. 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 Слева и справа от оператора && одинаковые подвыражения (vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType). 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 Подозрительное присваивание внутри условного выражения оператора if: 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);
}
Еще два похожих фрагмента кода:
- V559 Подозрительное присвоение внутри условного выражения оператора if: pwndView = 0. mainfrm.cpp 710
Выражение V547 всегда ложно. Вероятно, здесь следует использовать оператор ||. сущность.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 Подозрительное присваивание внутри условного выражения оператора if: _strModURLSelected = «». меню.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» (ссылка на «mod»). Позже этот результат сохраняется в объекте под именем «_strModURLSelected». Это собственная реализация класса, которая работает со строками. Из-за опечатки в условии «if (_strModURLSelected=»»)» полученный ранее url вместо сравнения будет заменен на пустую строку. Далее действует оператор, приводящий строку к типу const char*. В результате мы получим проверку на null указателя, содержащего ссылку на пустую строку. Такой указатель никогда не может быть равен нулю. Следовательно, условие всегда будет истинным. Таким образом, программа всегда будет использовать жестко заданную ссылку, хотя она должна была использоваться как значение по умолчанию.
Выражение V547 всегда истинно. Вероятно, здесь следует использовать оператор &&. свойствоcombobar.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 Выражение ulUsedShadowMemory ›= 0 всегда верно. Значение беззнакового типа всегда ›= 0. gfxlibrary.cpp 1693
void CGfxLibrary::ReduceShadows(void)
{
ULONG ulUsedShadowMemory = ....;
....
ulUsedShadowMemory -= sm.Uncache(); // <=
ASSERT( ulUsedShadowMemory>=0); // <=
....
}
В этом фрагменте кода выполняется небезопасное уменьшение беззнаковой переменной, так как переменная «ulUsedShadowMemory» может переполниться, при этом существует Assert(), который никогда не выдает предупреждение. Это очень подозрительный фрагмент кода, разработчикам следует его перепроверить.
Следует избегать выражения this != 0 V704 — это выражение всегда истинно в более новых компиляторах, потому что указатель this никогда не может быть NULL. сущность.ч 697
inline void CEntity::AddReference(void) {
if (this!=NULL) { // <=
ASSERT(en_ctReferences>=0);
en_ctReferences++;
}
};
В коде движка есть 28 сравнений this с null. Код был написан давно, но согласно последнему стандарту языка C++ указатель this никогда не может быть нулевым, и поэтому компилятор может сделать оптимизацию и удалить проверку. Это может привести к неожиданным ошибкам в случае более сложных условий. Примеры можно найти в документации по этой диагностике.
На данный момент Visual C++ так не работает, но это всего лишь вопрос времени. Этот кодекс отныне вне закона.
V547 Выражение ‘achrLine != «»’ всегда верно. Для сравнения строк следует использовать функцию strcmp(). 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++;
Еще два неверных сравнения:
- V547 Выражение всегда верно. Для сравнения строк следует использовать функцию strcmp(). свойствоcombobar.cpp 965
- V547 Выражение ‘achrLine == «»’ всегда ложно. Для сравнения строк следует использовать функцию strcmp(). worldeditor.cpp 2293
Разные ошибки
V541 Печатать строку achrDefaultScript в себя опасно. 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 Функция qsort получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. меш.cpp 224
// optimize lod of mesh
void CMesh::OptimizeLod(MeshLOD &mLod)
{
....
// sort array
qsort(&_aiSortedIndex[0] // <=
ctVertices
sizeof(&_aiSortedIndex[0]), // <=
qsort_CompareArray);
....
}
Функция qsort() принимает в качестве третьего аргумента размер сортируемого элемента массива. Очень подозрительно, что туда всегда передается размер указателя. Возможно, программист скопировал первый аргумент функции в третий, а амперсанд забыл удалить.
V607 Бесхозяйное выражение ‘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; // <=
....
}
....
}
Непонятно, что делает выделенная строка. Ну понятно же, что ничего. Поле class никак не используется, возможно, эта ошибка попала сюда после рефакторинга или после отладки строку оставили без изменений.
V610 Неопределенное поведение. Проверьте оператор сдвига ‘‹‹’. Левый операнд ‘(- 2)’ отрицательный. 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++ сдвиг отрицательного числа приводит к неопределенному поведению.
V646 Попробуйте проверить логику приложения. Возможно, отсутствует ключевое слово else. sessionstate.cpp 1191
void CSessionState::ProcessGameStream(void)
{
....
if (res==CNetworkStream::R_OK) {
....
} if (res==CNetworkStream::R_BLOCKNOTRECEIVEDYET) { // <=
....
} else if (res==CNetworkStream::R_BLOCKMISSING) {
....
}
....
}
Глядя на форматирование кода, мы можем предположить, что в каскаде условий отсутствует ключевое слово else.
Еще один похожий фрагмент:
- V646 Рассмотрите возможность проверки логики приложения. Возможно, отсутствует ключевое слово else. Terrain.cpp 759
V595 Указатель «pAD» использовался до того, как он был проверен на соответствие nullptr. Контрольные строки: 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», что означает возможную передачу указателя на эту функцию.
Вот полный список опасных фрагментов, содержащих указатели:
- V595 Указатель _ppenPlayer использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 851, 854. computer.cpp 851
- V595 Указатель ‘_meshEditOperations’ использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 416, 418. modelermeshexporter.cpp 416
- V595 Указатель «_fpOutput» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 654, 664. modelermeshexporter.cpp 654
- V595 Указатель «_appPolPnts» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 647, 676. modelermeshexporter.cpp 647
- V595 Указатель «pModelerView» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 60, 63. dlginfopgglobal.cpp 60
- V595 Указатель «pNewWT» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 736, 744. modeler.cpp 736
- V595 Указатель pvpViewPort использовался до того, как он был проверен на соответствие nullptr. Строки проверки: 1327, 1353. 1 327 1 327 1 327
- V595 Указатель «pDC» использовался до того, как он был проверен на соответствие nullptr. Проверьте строки: 138, 139. tooltipwnd.cpp 138
- V595 Указатель «m_pDrawPort» использовался до того, как он был проверен на соответствие nullptr. Контрольные строки: 94, 97. wndanimationframes.cpp 94
- V595 Указатель «penBrush» использовался до того, как он был проверен на соответствие nullptr. Контрольные строки: 9033, 9035. worldeditorview.cpp 9033
Вывод
Анализ Serious Engine 1 v.1.10 показал, что баги могут жить в программе очень долго, и даже отмечать юбилеи! В этой статье собраны только самые интересные примеры из отчета анализатора. Несколько предупреждений были даны в виде списка. Но в целом отчет имеет достаточно большое количество предупреждений, учитывая, что проект не очень большой. У компании Croteam есть более совершенные игровые движки — Serious Engine 2, Serious Engine 3 и Serious Engine 4. Страшно подумать, сколько небезопасного кода могло попасть в новые версии движка. Надеюсь, что разработчики будут использовать статический анализатор кода, и радовать пользователей, выпуская качественные игры. Тем более, что анализатор легко скачать, легко запустить в Visual Studio, а для других систем есть Standalone утилита.
