Автор: Святослав Размыслов
К юбилею выхода шутера от первого лица 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
| C++ | 1
2
3
4
5
6
7
8
9
10
| 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; };
....
}; |
|
V501 There are identical sub-expressions 'GetShadingMapWidth() < 32' to the left and to the right of the '||' operator. terrain.cpp 561
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
| void CTerrain::SetShadowMapsSize(....)
{
....
if(GetShadowMapWidth()<32 || GetShadingMapHeight()<32) {
....
}
if(GetShadingMapWidth()<32 || GetShadingMapWidth()<32) { // <=
tr_iShadingMapSizeAspect = 0;
}
....
PIX pixShadingMapWidth = GetShadingMapWidth();
PIX pixShadingMapHeight = GetShadingMapHeight();
....
} |
|
| C++ | 1
2
3
| if(GetShadingMapWidth()<32 || GetShadingMapHeight()<32) {
tr_iShadingMapSizeAspect = 0;
} |
|
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
| 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) &&
....
); |
|
Много странных сравнений
V559 Suspicious assignment inside the condition expression of 'if' operator: pwndView = 0. mainfrm.cpp 697
| C++ | 1
2
3
4
5
6
7
8
9
10
11
| 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();
} |
|
Скорее всего, здесь хотели написать оператор неравенства '!=' и код должен быть таким:
| C++ | 1
2
3
4
5
| if (pwndView != NULL) {
// get the MDIChildFrame of active window
CChildFrame *pfrChild = (....)pwndView->GetParentFrame();
ASSERT(pfrChild!=NULL);
} |
|
- V559 Suspicious assignment inside the condition expression of 'if' operator: pwndView = 0. mainfrm.cpp 710
V547 Expression is always false. Probably the '||' operator should be used here. entity.cpp 3537
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
| 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 {
....
}
....
} |
|
V559 Suspicious assignment inside the condition expression of 'if' operator: _strModURLSelected = "". menu.cpp 1188
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
14
| 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";
}
....
} |
|
Результатом этого кода будет всегда использование ссылки, которую жёстко прописали в коде. Хотя, планировали использовать эту ссылка как значение по умолчанию.
V547 Expression is always true. Probably the '&&' operator should be used here. propertycombobar.cpp 1853
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
| 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;
}
....
} |
|
V547 Expression 'ulUsedShadowMemory >= 0' is always true. Unsigned type value is always >= 0. gfxlibrary.cpp 1693
| C++ | 1
2
3
4
5
6
7
8
| void CGfxLibrary::ReduceShadows(void)
{
ULONG ulUsedShadowMemory = ....;
....
ulUsedShadowMemory -= sm.Uncache(); // <=
ASSERT( ulUsedShadowMemory>=0); // <=
....
} |
|
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
| C++ | 1
2
3
4
5
6
| inline void CEntity::AddReference(void) {
if (this!=NULL) { // <=
ASSERT(en_ctReferences>=0);
en_ctReferences++;
}
}; |
|
Конкретно Visual C++ пока так себя не ведёт. Но это дело времени. Такой код более вне закона.
V547 Expression 'achrLine != ""' is always true. To compare strings you should use strcmp() function. worldeditor.cpp 2254
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
| 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" и указатель на временную пустую строку. Такие указатели никогда не будут равны.
Правильный код с использованием функции strcmp():
| C++ | 1
| if(strcmp(achrLine, "") != 0) ctLines++; |
|
- V547 Expression is always true. To compare strings you should use strcmp() function. propertycombobar.cpp 965
- V547 Expression 'achrLine == ""' is always false. To compare strings you should use strcmp() function. worldeditor.cpp 2293
Разные ошибки
V541 It is dangerous to print the string 'achrDefaultScript' into itself. dlgcreateanimatedtexture.cpp 359
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
14
| 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); // <=
....
} |
|
Для объяснения, почему здесь может получиться неожиданный результат, я процитирую простой пример из документации к этой диагностике:
| C++ | 1
2
| char s[100] = "test";
sprintf(s, "N = %d, S = %s", 123, s); |
|
Но на практике в буфере будет сформирована строка:
| C++ | 1
| N = 123, S = N = 123, S = |
|
| C++ | 1
2
3
| char s1[100] = "test";
char s2[100];
sprintf(s2, "N = %d, S = %s", 123, s1); |
|
V579 The qsort function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. mesh.cpp 224
| C++ | 1
2
3
4
5
6
7
8
9
10
11
| // optimize lod of mesh
void CMesh::OptimizeLod(MeshLOD &mLod)
{
....
// sort array
qsort(&_aiSortedIndex[0] // <=
ctVertices
sizeof(&_aiSortedIndex[0]), // <=
qsort_CompareArray);
....
} |
|
V607 Ownerless expression 'pdecDLLClass->dec_ctProperties'. entityproperties.cpp 107
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
| 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
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
| 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); // <=
.... // <=
} |
|
V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. sessionstate.cpp 1191
| C++ | 1
2
3
4
5
6
7
8
9
10
11
12
| void CSessionState::ProcessGameStream(void)
{
....
if (res==CNetworkStream::R_OK) {
....
} if (res==CNetworkStream::R_BLOCKNOTRECEIVEDYET) { // <=
....
} else if (res==CNetworkStream::R_BLOCKMISSING) {
....
}
....
} |
|
Ещё такое место:
- V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. terrain.cpp 759
V595 The 'pAD' pointer was utilized before it was verified against nullptr. Check lines: 791, 796. anim.cpp 791
| C++ | 1
2
3
4
5
6
7
8
9
10
11
| 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();
} |
|
Весь список найденных опасных мест с указателями:
- V595 The '_ppenPlayer' pointer was utilized before it was verified against nullptr. Check lines: 851, 854. computer.cpp 851
- V595 The '_meshEditOperations' pointer was utilized before it was verified against nullptr. Check lines: 416, 418. modelermeshexporter.cpp 416
- V595 The '_fpOutput' pointer was utilized before it was verified against nullptr. Check lines: 654, 664. modelermeshexporter.cpp 654
- V595 The '_appPolPnts' pointer was utilized before it was verified against nullptr. Check lines: 647, 676. modelermeshexporter.cpp 647
- V595 The 'pModelerView' pointer was utilized before it was verified against nullptr. Check lines: 60, 63. dlginfopgglobal.cpp 60
- V595 The 'pNewWT' pointer was utilized before it was verified against nullptr. Check lines: 736, 744. modeler.cpp 736
- V595 The 'pvpViewPort' pointer was utilized before it was verified against nullptr. Check lines: 1327, 1353. serioussam.cpp 1327
- V595 The 'pDC' pointer was utilized before it was verified against nullptr. Check lines: 138, 139. tooltipwnd.cpp 138
- V595 The 'm_pDrawPort' pointer was utilized before it was verified against nullptr. Check lines: 94, 97. wndanimationframes.cpp 94
- V595 The 'penBrush' pointer was utilized before it was verified against nullptr. Check lines: 9033, 9035. worldeditorview.cpp 9033
Заключение
Проверка игрового движка Serious Engine 1 v.1.10 показала, что ошибки в коде проектов могут жить очень долго и даже отмечать юбилей! В статью вошли только некоторые самые интересные примеры из отчёта анализатора. Много предупреждений я привёл просто списком. Но весь отчёт содержит довольно много предупреждений для такого маленького проекта. У компании Croteam есть более совершенные игровые движки — Serious Engine 2, Serious Engine 3 и Serious Engine 4. Боюсь представить, сколько опасного кода могло перекачивать в новые серии движка. Я надеюсь, что после прочтения статьи, разработчики воспользуются статическим анализатором PVS-Studio в своих проектах и будут радовать пользователей качественными играми. Ведь анализатор легко скачать, легко запустить в Visual Studio, а для любых других сборочных систем есть утилита Standalone.
|
Тег [CODE][/CODE] - не универсальное решение. Поставьте соответствующему коду соответствующий тег, раз уж хотите красиво сделать репост...
