Vor der Veröffentlichung von Amnesia: Rebirth hat Fractional Games den Quellcode für die legendäre Amnesia: The Dark Descent und ihre Fortsetzung Amnesia: A Machine For Pigs veröffentlicht. Warum nicht ein statisches Analysewerkzeug verwenden, um zu sehen, wie schreckliche Fehler in den Innenseiten dieser Kult-Horror-Spiele verborgen sind?
Als ich auf Reddit die Nachricht sah, dass der Quellcode der Spiele " Amnesia: The Dark Descend " und " Amnesia: Eine Maschine für Schweine " veröffentlicht worden war, konnte ich diesen Code nicht mit PVS-Studio passieren und überprüfen und gleichzeitig schreiben über diesen Artikel. Besonders angesichts der Tatsache, dass am 20. Oktober (und zum Zeitpunkt der Veröffentlichung dieses Artikels war es bereits erschienen) ein neuer Teil dieser Reihe: " Amnesia: Rebirth ".
Amnesia: The Dark Descent wurde 2010 veröffentlicht und ist zu einem legendären Survival-Horror-Spiel geworden. Ehrlich gesagt, ich habe es noch nie geschafft, auch nur ein bisschen zu bestehen, da ich Horrorspiele nach demselben Algorithmus spiele: Wetten, fünf Minuten eingeben, im ersten Moment mit "alt + f4" beenden und das Spiel löschen.Aber ich habe mir die Passage dieses Spiels auf YouTube gern angesehen.
Und falls jemand noch nicht mit PVS-Studio vertraut ist, ist dies ein statischer Analysator, der nach Fehlern und verdächtigen Stellen im Quellcode von Programmen sucht.
Ich schaue mir besonders gerne den Quellcode von Spielen an. Wenn Sie sich also fragen, welche Fehler darin gemacht wurden, können Sie meine vorherigen Artikel lesen. Oder schauen Sie sich die Artikel meiner Kollegen über das Überprüfen des Quellcodes von Spielen an.
Nach der Überprüfung stellte sich heraus, dass sich ein Großteil des Codes in "The Dark Descend" und "A Machine For Pigs" überlappte und die Berichte für die beiden Projekte sehr ähnlich waren. So sind fast alle Fehler, die ich unten zitieren werde, in beiden Projekten enthalten.
Die größte Fehlerschicht aller in diesen Projekten festgestellten Analysegeräte war die Schicht der "Copy-Paste" -Fehler. Daher der Titel des Artikels. Der Hauptgrund für diese Fehler ist der " Last-Line-Effekt ".
Kommen wir zur Sache.
Fehler beim Kopieren und Einfügen
Es gab viele verdächtige Stellen, die dem unaufmerksamen Kopieren ähnelten. Einige Fälle werden möglicherweise irgendwie durch die interne Logik des Spiels selbst verursacht. Aber wenn sie mich und den Analysator in Verlegenheit brachten, war es wert, zumindest einen Kommentar zu ihnen abzugeben. Schließlich können andere Entwickler genauso schlampig sein wie ich.
Snippet 1.
Beginnen wir mit einem Beispiel, in dem die gesamte Funktion aus dem Vergleich der Ergebnisse von Methoden und Feldern zweier Objekte aObjectDataA und aObjectDataB besteht . Ich werde diese Funktion aus Gründen der Klarheit vollständig geben. Versuchen Sie selbst festzustellen, wo der Fehler in der Funktion gemacht wurde:
static bool SortStaticSubMeshesForBodies(const ....& aObjectDataA,
const ....& aObjectDataB)
{
//Is shadow caster check
if( aObjectDataA.mpObject->GetRenderFlagBit(....)
!= aObjectDataB.mpObject->GetRenderFlagBit(....))
{
return aObjectDataA.mpObject->GetRenderFlagBit(....)
< aObjectDataB.mpObject->GetRenderFlagBit(....);
}
//Material check
if( aObjectDataA.mpPhysicsMaterial != aObjectDataB.mpPhysicsMaterial)
{
return aObjectDataA.mpPhysicsMaterial < aObjectDataB.mpPhysicsMaterial;
}
//Char collider or not
if( aObjectDataA.mbCharCollider != aObjectDataB.mbCharCollider)
{
return aObjectDataA.mbCharCollider < aObjectDataB.mbCharCollider;
}
return aObjectDataA.mpObject->GetVertexBuffer()
< aObjectDataA.mpObject->GetVertexBuffer();
}Ein Bild, um die Antwort nicht versehentlich auszuspionieren:
Könnten Sie den Fehler finden? Ja, die letzte Rückgabe ist ein Vergleich mit aObjectDataA auf beiden Seiten. Beachten Sie, dass alle Ausdrücke im Originalcode in einer Zeile geschrieben wurden. Hier habe ich Silbentrennungen vorgenommen, damit alles genau in die Zeilenbreite passt. Stellen Sie sich vor, wonach ein solcher Defekt am Ende des Arbeitstages suchen wird. Der Analysator findet ihn sofort nach dem Zusammenstellen und Ausführen der inkrementellen Analyse.
V501 Links und rechts vom Operator '<' befinden sich identische Unterausdrücke 'aObjectDataA.mpObject-> GetVertexBuffer ()'. WorldLoaderHplMap.cpp 1123
Infolgedessen wird ein solcher Fehler fast zum Zeitpunkt des Schreibens des Codes gefunden, anstatt sich in mehreren QA-Phasen in den Tiefen des Codes zu verstecken, was Ihre Suche um ein Vielfaches erschwert.
Anmerkung von Kollege Andrey Karpov. Ja, dies ist ein klassischer "Last Line Error". Dies ist jedoch auch ein klassisches Fehlermuster beim Vergleich zweier Objekte. Siehe den Artikel " Böse Leben in Vergleichsfunktionen ".Fragment 2.
Schauen wir uns den Code an, der die Warnung verursacht hat:
Ich präsentiere den Code zur Verdeutlichung mit einem Screenshot.
Die Warnung selbst sieht folgendermaßen aus:
V501 Links und rechts vom '||' befinden sich identische Unterausdrücke 'lType == eLuxJournalState_OpenNote'. Operator. LuxJournal.cpp 2262
Der Analysator hat festgestellt, dass beim Überprüfen des Werts der Variablen lType ein Fehler aufgetreten ist . Die Gleichheit wird zweimal mit demselben Mitglied des Enumerators eLuxJournalState_OpenNote überprüft .
Zunächst möchte ich, dass eine solche Bedingung in einer "tabellarischen" Form geschrieben wird, um die Lesbarkeit zu verbessern. Siehe Kapitel N13 im Minibuch Die größte Frage der Programmierung, des Refactorings und all dessen .
if(!( lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenDiary
|| lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenNarratedDiary))
return false;In dieser Form wird es auch ohne Analysator viel einfacher, einen Fehler zu erkennen.
Es stellt sich jedoch die Frage, ob eine solche fehlerhafte Prüfung zu einer Verzerrung der Programmlogik führt. Es ist zwar möglich, dass ein anderer lType- Wert überprüft werden sollte, die Überprüfung wurde jedoch aufgrund eines Fehlers beim Kopieren und Einfügen verpasst. Werfen wir einen Blick auf die Aufzählung selbst:
enum eLuxJournalState
{
eLuxJournalState_Main,
eLuxJournalState_Notes,
eLuxJournalState_Diaries,
eLuxJournalState_QuestLog,
eLuxJournalState_OpenNote,
eLuxJournalState_OpenDiary,
eLuxJournalState_OpenNarratedDiary,
eLuxJournalState_LastEnum,
};Es gibt nur drei Bedeutungen, deren Name das Wort "Offen" enthält. Und alle drei sind im Scheck vorhanden. Höchstwahrscheinlich gibt es hier keine Verzerrung der Logik, aber es ist immer noch unmöglich, sicher zu sagen. Daher fand der Analysator entweder einen logischen Fehler, den der Spieleentwickler beheben konnte, oder einen "hässlichen" geschriebenen Ort, der zur besseren Übersichtlichkeit neu geschrieben werden sollte.
Fragment 3.
Der folgende Fall ist im Allgemeinen das offensichtlichste Beispiel für einen Fehler beim Kopieren und Einfügen.
V778 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise ist dies ein Tippfehler und die Variable 'mvSearcherIDs' sollte anstelle von 'mvAttackerIDs' verwendet werden. LuxSavedGameTypes.cpp 615
void cLuxMusicHandler_SaveData::ToMusicHandler(....)
{
....
// Enemies
//Attackers
for(size_t i=0; i<mvAttackerIDs.Size(); ++i)
{
iLuxEntity *pEntity = apMap
->GetEntityByID(mvAttackerIDs[i]);
if(....)
{
....
}
else
{
Warning("....", mvAttackerIDs[i]);
}
}
//Searchers
for(size_t i=0; i<mvSearcherIDs.Size(); ++i)
{
iLuxEntity *pEntity = apMap->GetEntityByID(mvSearcherIDs[i]);
if(....)
{
....
}
else
{
Warning("....", mvAttackerIDs[i]);
}
}
}Im ersten Zyklus geht die Arbeit mit dem pEntity- Zeiger , der über mvAttackerIDs empfangen wurde, und wenn die Bedingung nicht erfüllt ist, wird eine Nachricht zum Debuggen für dieselben mvAttackerIDs ausgegeben . In der nächsten Schleife, die genauso wie der vorherige Codeabschnitt gestaltet ist , wird pEntity jedoch mithilfe von mvSearcherIDs erhalten . Die Warnung wird jedoch weiterhin mit der Erwähnung von mvAttackerIDs ausgegeben .
Höchstwahrscheinlich wurde der signierte Codeblock "Searchers" aus dem Block "Attackers" kopiert, mvAttackerIDs wurden durch mvSearcherIDs ersetzt , aber der else- Block wurde nicht geändert. Infolgedessen verwendet die Fehlermeldung ein Element des falschen Arrays.
Dieser Fehler wirkt sich nicht auf die Logik des Spiels aus, aber auf diese Weise können Sie einer Person, die diesen Ort debuggen und Zeit mit der Arbeit mit dem falschen mvSearcherIDs- Element verschwenden muss, ein ernstes Schwein auferlegen .
Fragment 4.
Der Analysator zeigte den folgenden verdächtigen Ort mit drei Warnungen an:
- V547 Der Ausdruck 'pEntity == 0' ist immer falsch. LuxScriptHandler.cpp 2444
- V649 Es gibt zwei ' if' -Anweisungen mit identischen bedingten Ausdrücken. Die erste 'if'-Anweisung enthält die Funktionsrückgabe. Dies bedeutet, dass die zweite 'if'-Anweisung sinnlos ist. Überprüfen Sie die Zeilen: 2433, 2444. LuxScriptHandler.cpp 2444
- V1051 Prüfen Sie, ob Druckfehler vorliegen . Es ist möglich, dass die 'pTargetEntity' hier überprüft wird. LuxScriptHandler.cpp 2444
Schauen wir uns den Code an:
void __stdcall cLuxScriptHandler::PlaceEntityAtEntity(....)
{
cLuxMap *pMap = gpBase->mpMapHandler->GetCurrentMap();
iLuxEntity *pEntity = GetEntity(....);
if(pEntity == NULL) return;
if(pEntity->GetBodyNum() == 0)
{
....
}
iPhysicsBody *pBody = GetBodyInEntity(....);
if(pBody == NULL) return;
iLuxEntity *pTargetEntity = GetEntity(....);
if(pEntity == NULL) return; // <=
iPhysicsBody *pTargetBody = GetBodyInEntity(....);
if(pTargetBody == NULL) return;
....
}Für die zweite Prüfung pEntity == NULL wurde eine V547- Warnung ausgegeben . Für den Analysator, wird diese Prüfung immer falsch , denn wenn diese Bedingung war wahr , dann wäre die Funktion höher die vorhergehenden Kontrolle durch beenden. Die nächste Warnung, V649, wurde genau ausgegeben, weil wir zwei identische Prüfungen haben. Normalerweise ist ein solcher Fall kein Fehler. Plötzlich implementiert ein Teil des Codes eine Logik, und in einem anderen Teil des Codes muss gemäß derselben Prüfung etwas anderes implementiert werden. In diesem Fall besteht der Hauptteil der ersten Prüfung jedoch aus der Rückgabe
Wenn die Bedingung sich als wahr herausstellt, erreicht die zweite Prüfung nicht einmal den Punkt. Durch die Verfolgung einer solchen Logik reduziert der Analysator die Anzahl falscher Nachrichten für verdächtigen Code und gibt sie nur für sehr seltsame Logik aus.
Der in der letzten Warnung angegebene Fehler ist dem vorherigen Beispiel von Natur aus sehr ähnlich. Höchstwahrscheinlich wurden alle Prüfungen ab der ersten Prüfung if (pEntity == NULL) dupliziert , und dann wurde das geprüfte Objekt durch das erforderliche ersetzt. Bei pBody- und pTargetBody-Objekten wurde die Ersetzung vorgenommen, das pTargetEntity- Objekt wurde jedoch vergessen. Infolgedessen wird dieses Objekt nicht überprüft.
In dem Beispiel, das wir betrachten, stellt sich heraus, dass ein solcher Fehler im Allgemeinen den Programmverlauf nicht beeinflusst, wenn Sie etwas tiefer in den Code eintauchen. Der pTargetBody- Zeiger erhält seinen Wert von der GetBodyInEntity- Funktion :
iPhysicsBody *pTargetBody = GetBodyInEntity(pTargetEntity,
asTargetBodyName);Das erste Argument hier ist nur ein zuvor nicht aktivierter Zeiger, der nirgendwo anders verwendet wird. Und zum Glück wird in dieser Funktion das erste Argument für NULL überprüft :
iPhysicsBody* ....::GetBodyInEntity(iLuxEntity* apEntity, ....)
{
if(apEntity == NULL){
return NULL;
}
....
}Infolgedessen funktioniert dieser Code am Ende korrekt, obwohl er einen Fehler enthält.
Fragment 5.
Und noch ein verdächtiger Ort mit Copy-Paste!
Diese Methode löscht die Felder des Objekts der cLuxPlayer- Klasse .
void cLuxPlayer::Reset()
{
....
mfRoll=0;
mfRollGoal=0;
mfRollSpeedMul=0; //<-
mfRollMaxSpeed=0; //<-
mfLeanRoll=0;
mfLeanRollGoal=0;
mfLeanRollSpeedMul=0;
mfLeanRollMaxSpeed=0;
mvCamAnimPos =0;
mvCamAnimPosGoal=0;
mfRollSpeedMul=0; //<-
mfRollMaxSpeed=0; //<-
....
}Aus irgendeinem Grund werden die beiden Variablen mfRollSpeedMul und mfRollMaxSpeed zweimal ungültig gemacht :
- V519 Der Variablen 'mfRollSpeedMul' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 298, 308. LuxPlayer.cpp 308
- V519 Der Variablen 'mfRollMaxSpeed' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 299, 309. LuxPlayer.cpp 309
Schauen wir uns die Klasse selbst an und sehen wir, welche Felder sie enthält:
class cLuxPlayer : ....
{
....
private:
....
float mfRoll;
float mfRollGoal;
float mfRollSpeedMul;
float mfRollMaxSpeed;
float mfLeanRoll;
float mfLeanRollGoal;
float mfLeanRollSpeedMul;
float mfLeanRollMaxSpeed;
cVector3f mvCamAnimPos;
cVector3f mvCamAnimPosGoal;
float mfCamAnimPosSpeedMul;
float mfCamAnimPosMaxSpeed;
....
}Interessanterweise gibt es drei ähnliche Variablenblöcke mit verwandten Namen: mfRoll , mfLeanRoll und mvCamAnimPos . Beim Zurücksetzen werden diese drei Blöcke gelöscht, mit Ausnahme der letzten beiden Variablen aus dem dritten Block, mfCamAnimPosSpeedMul und mfCamAnimPosMaxSpeed . Anstelle dieser beiden Variablen werden doppelte Zuordnungen gefunden. Höchstwahrscheinlich wurden alle diese Zuweisungen aus dem ersten Zuweisungsblock kopiert, und dann wurden die Variablennamen durch die erforderlichen ersetzt.
Es kann sein, dass zwei fehlende Variablen nicht auf Null gesetzt werden sollten, aber das Gegenteil ist auch sehr wahrscheinlich. Und Neuzuweisungen helfen offensichtlich nicht bei der Pflege dieses Codes. Wie Sie sehen können, bemerken Sie in einem langen Fußtuch mit denselben Aktionen möglicherweise keinen solchen Fehler, und der Analysator hilft hier weiter.
Fragment 5.5.
Dieses Beispiel ist dem vorherigen sehr ähnlich. Ich werde sofort ein Codefragment und eine Analysatorwarnung dafür zitieren.
V519 Der Variablen 'mfTimePos' werden zweimal nacheinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 49, 53. AnimationState.cpp 53
cAnimationState::cAnimationState(....)
{
....
mfTimePos = 0;
mfWeight = 1;
mfSpeed = 1.0f;
mfBaseSpeed = 1.0f;
mfTimePos = 0;
mfPrevTimePos=0;
....
}Der Variablen mfTimePos wurde zweimal der Wert 0 zugewiesen. Schauen wir uns wie im vorherigen Beispiel die Deklaration dieses Felds an:
class cAnimationState
{
....
private:
....
//Properties of the animation
float mfLength;
float mfWeight;
float mfSpeed;
float mfTimePos;
float mfPrevTimePos;
....
}Sie können sehen, dass dieser Deklarationsblock wie im vorherigen Beispiel auch mit der Zuweisungsreihenfolge im Fehlercode-Snippet übereinstimmt. Hier erhält der Wert in der Zuweisung anstelle der Variablen mfLength mfTimePos . Hier kann der Fehler jedoch nicht durch Kopieren des Blocks und den "Effekt der letzten Zeile" erklärt werden. Es kann sein, dass mfLength kein neuer Wert zugewiesen werden muss, aber dieser Speicherort ist immer noch verdächtig.
Fragment 6.
Der Analysator gab eine ganze Reihe von Warnungen für das nächste Codefragment aus "Amnesia: A Machine For Pigs" aus. Ich werde nur einen Teil des Codes angeben, für den Fehler der gleichen Art ausgegeben wurden:
void cLuxEnemyMover::UpdateMoveAnimation(float afTimeStep)
{
....
if(prevMoveState != mMoveState)
{
....
//Backward
if(mMoveState == eLuxEnemyMoveState_Backward)
{
....
}
....
//Walking
else if(mMoveState == eLuxEnemyMoveState_Walking)
{
bool bSync = prevMoveState == eLuxEnemyMoveState_Running
|| eLuxEnemyMoveState_Jogging
? true : false;
....
}
....
}
}Wo ist der Fehler hier?
Der Analysator gab die folgenden Warnungen aus:
- V768 Die Aufzählungskonstante 'eLuxEnemyMoveState_Jogging' wird als Variable eines Booleschen Typs verwendet. LuxEnemyMover.cpp 672
- V768 Die Aufzählungskonstante 'eLuxEnemyMoveState_Walking' wird als Variable eines Booleschen Typs verwendet. LuxEnemyMover.cpp 680
- V768 Die Aufzählungskonstante 'eLuxEnemyMoveState_Jogging' wird als Variable eines Booleschen Typs verwendet. LuxEnemyMover.cpp 688
Die if-else-if-Kette wird im Originalcode wiederholt, und dann wurden diese Warnungen nur für jeden Körper jedes anderen if ausgegeben .
Betrachten Sie die Zeile, auf die der Parser zeigt:
bool bSync = prevMoveState == eLuxEnemyMoveState_Running
|| eLuxEnemyMoveState_Jogging
? true : false;Es ist nicht überraschend, dass sich ein Fehler in einen solchen Ausdruck eingeschlichen hat, der auch im Original in einer Zeile geschrieben ist. Und Sie haben es wahrscheinlich schon bemerkt. Das Mitglied der Aufzählung eLuxEnemyMoveState_Jogging wird mit nichts verglichen, sein Wert wird überprüft. Höchstwahrscheinlich wurde der Ausdruck 'prevMoveState == eLuxEnemyMoveState_Jogging' impliziert.
Ein solcher Fehler mag völlig harmlos erscheinen. Aber in meinem anderen Artikel über das Überprüfen der Bullet Engine unter den Commits für das Projekt habe ich eine Fehlerbehebung gefundenvon der gleichen Art, was dazu führt, dass Kräfte von der falschen Seite auf Objekte ausgeübt werden. In diesem Fall wurde dieser Fehler mehrmals gemacht. Nun, ich stelle auch fest, dass die ternäre Bedingung völlig bedeutungslos ist, da sie letztendlich für die booleschen Ergebnisse logischer Operatoren erfüllt wird.
Fragment 7.
Und schließlich die letzten Beispiele für Fehler beim Kopieren und Einfügen. Diesmal wieder in einer bedingten Anweisung. Der Analysator gab eine Warnung für den folgenden Code aus:
void iParticleEmitter::SetSubDivUV(const cVector2l &avSubDiv)
{
//Check so that there is any subdivision
// and that no sub divison axis is
//equal or below zero
if( (avSubDiv.x > 1 || avSubDiv.x > 1) && (avSubDiv.x >0 && avSubDiv.y >0))
{
....
}
....
}Ich denke, es ist ziemlich leicht, einen verdächtigen Ort in einem solchen Fragment zu bemerken, der vom gesamten Code getrennt ist. Dieser Fehler konnte sich jedoch vor den Entwicklern dieses Spiels verbergen.
Der Analysator gab die folgende Warnung aus:
V501 Links und rechts vom '||' befinden sich identische Unterausdrücke. Operator: avSubDiv.x> 1 || avSubDiv.x> 1 ParticleEmitter.cpp 199
Die zweite Klammer in der Bedingung zeigt, dass sowohl x- als auch y- Felder aktiviert sind . In der ersten Klammer wurde dieser Moment jedoch aus irgendeinem Grund verpasst und nur das Feld x wird überprüft... Nach dem Validierungskommentar sollten beide Felder validiert worden sein. Hier funktionierte irgendwie nicht der "Effekt der letzten Zeile", sondern der erste, weil sie in der ersten Klammer vergessen hatten, den Aufruf des x- Feldes durch den Aufruf des y- Feldes zu ersetzen .
Daher sind solche Fehler sehr schwierig, da dem Entwickler in diesem Fall nicht einmal geholfen wurde, einen erklärenden Kommentar zu der Bedingung zu schreiben.
In solchen Fällen würde ich empfehlen, es sich zur Gewohnheit zu machen, entsprechende Schecks in tabellarischer Form aufzuzeichnen. Es ist einfacher, auf diese Weise zu bearbeiten, und der Fehler wird viel deutlicher:
if( (avSubDiv.x > 1 || avSubDiv.x > 1)
&& (avSubDiv.x > 0 && avSubDiv.y > 0))Fragment 7.5.
Absolut gleich, tatsächlich wurde der Fehler an einer völlig anderen Stelle gefunden:
static bool EdgeTriEqual(const cTriEdge &edge1, const cTriEdge &edge2)
{
if(edge1.tri1 == edge2.tri1 && edge1.tri2 == edge2.tri2)
return true;
if(edge1.tri1 == edge1.tri1 && edge1.tri2 == edge2.tri1)
return true;
return false;
}Wie haben Sie es geschafft, sofort zu sehen, wo sie sich versteckt hat? Nicht umsonst wurden bereits so viele Beispiele aussortiert :)
Der Analysator gab die folgende Warnung aus:
V501 Links und rechts vom Operator '==' befinden sich identische Unterausdrücke: edge1.tri1 == edge1.tri1 Math.cpp 2914
Lassen Sie uns dies analysieren Fragment in Ordnung. Offensichtlich prüft die erste Prüfung die Gleichheit der Felder edge1.tri1 und edge2.tri2 und gleichzeitig die Gleichheit von edge1.tri2 und edge2.tri2 :
edge1.tri1 -> edge2.tri1
edge1.tri2 -> edge2.tri2Und bei der zweiten Prüfung musste nach dem richtigen Teil der Prüfung 'edge1.tri2 == edge2.tri1' die Gleichheit dieser Felder "quer" überprüft werden:
Anstatt jedoch nach edge1.tri1 == edge2.tri2 zu suchen , wurde eine bedeutungslose Überprüfung durch edge1.tri1 == edge1.tri1 durchgeführt . Aber das ist der ganze Inhalt der Funktion, ich habe nichts daraus gelöscht. Trotzdem ist ein solcher Fehler in den Code eingegangen.
Andere Fehler
Fragment 1.
Ich werde das folgende Codefragment mit Originaleinzügen geben.
void iCharacterBody::CheckMoveCollision(....)
{
....
/////////////////////////////////////
//Forward velocity reflection
//Make sure that new velocity points in the right direction
//and that it is not too large!
if(mfMoveSpeed[eCharDir_Forward] != 0)
{
vForwardVel = ....;
float fForwardSpeed = vForwardVel.Length();
if(mfMoveSpeed[eCharDir_Forward] > 0)
if(mfMoveSpeed[eCharDir_Forward] > fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = fForwardSpeed;
else
if(mfMoveSpeed[eCharDir_Forward] < fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = -fForwardSpeed;
}
....
}PVS-Studio- Warnung : V563 Möglicherweise muss dieser ' else' -Zweig auf die vorherige' if'-Anweisung angewendet werden . CharacterBody.cpp 1591
Dieses Beispiel kann verwirrend sein. Warum wird sonst das gleiche eingerückt wie das äußerste, wenn ? Ist es das andere für den äußeren Zustand? Na , dann müssen Sie die Klammern setzen, sonst sonst bezieht sich auf die unmittelbar vor , wenn .
if(mfMoveSpeed[eCharDir_Forward] > 0)
{
if(mfMoveSpeed[eCharDir_Forward] > fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = fForwardSpeed;
}
else if(mfMoveSpeed[eCharDir_Forward] < fForwardSpeed)
{
mfMoveSpeed[eCharDir_Forward] = -fForwardSpeed;
}Oder nicht? Während ich diesen Artikel schrieb, änderte ich mehrmals meine Meinung darüber, welche Variante der Reihenfolge der konzipierten Aktionen für diesen Code am wahrscheinlichsten ist.
Wenn wir uns etwas eingehender mit diesem Code befassen , stellt sich heraus, dass die Variable fForwardSpeed , mit der der Vergleich im unteren ifs durchgeführt wird , keinen Wert kleiner als Null haben kann, da sie einen Wert von der Length- Methode erhält :
inline T Length() const
{
return sqrt( x * x + y * y + z * z);
}Dann besteht das Wesentliche dieser Überprüfungen höchstwahrscheinlich darin, dass wir zuerst prüfen, ob das Element mfMoveSpeed größer als Null ist, und dann seinen Wert relativ zu fForwardSpeed prüfen . Darüber hinaus entsprechen die letzten beiden ifs in ihrem Wortlaut einander.
In diesem Fall funktioniert der Originalcode wie vorgesehen! Aber es wird definitiv die Person, die kommt, um es zu bearbeiten / umgestalten, zum Rätsel machen.
Ich dachte, ich würde niemals auf Code stoßen, der so aussieht. Aus Interesse habe ich in unserer Datenbank nach Fehlern gesucht , die in Open-Source-Projekten gefunden und in Artikeln beschrieben wurden. Beispiele für diesen Fehler wurden in anderen Projekten gefunden - Sie können sie sich selbst ansehen .
Und bitte schreiben Sie nicht so, auch wenn Sie selbst in diesem Fall klar sind. Oder geschweifte Klammern oder korrekte Einrückungen oder besser beides. Tauchen Sie nicht in das Leiden derer ein, die Ihren Code und sich selbst in der Zukunft verstehen;)
Fragment 2.
Der nächste Fehler verwirrte mich ein wenig und ich versuchte lange Zeit, die Logik hier zu finden. Aber am Ende scheint es mir immer noch, dass dies höchstwahrscheinlich ein Fehler ist und ein ziemlich grober.
Werfen wir einen Blick auf den Code:
bool cBinaryBuffer::DecompressAndAdd(char *apSrcData, size_t alSize)
{
....
///////////////////////////
// Init decompression
int ret = inflateInit(&zipStream);
if (ret != Z_OK) return false;
///////////////////////////
// Decompress, chunk by chunk
do
{
//Set current output chunk
zipStream.avail_out = lMaxChunkSize;
....
//Decompress as much as possible to current chunk
int ret = inflate(&zipStream, Z_NO_FLUSH);
if(ret != Z_OK && ret != Z_STREAM_END)
{
inflateEnd(&zipStream);
return false;
}
....
}
while (zipStream.avail_out == 0 && ret != Z_STREAM_END);
....
return true;
}V711 Es ist gefährlich, eine lokale Variable in einer Schleife mit demselben Namen wie eine Variable zu erstellen, die diese Schleife steuert. BinaryBuffer.cpp 371
Wir haben also eine Variable ret , die den Ausgang aus der do-while-Schleife steuert . In dieser Schleife wird jedoch keine neue Variable mit dem Namen ret deklariert, anstatt dieser externen Variablen einen neuen Wert zuzuweisen . Infolgedessen überschreibt es die externe Variable ret, und die Variable, die in der Schleifenbedingung überprüft wird, ändert sich nie.
In einer unglücklichen Kombination von Umständen könnte ein solcher Zyklus endlos werden. In diesem Fall speichert dieser Code höchstwahrscheinlich eine interne Bedingung, die den Wert der internen Variablen ret überprüft und führt zum Verlassen der Funktion.
Fazit
Sehr oft verwenden Entwickler statische Analysen nicht regelmäßig, sondern mit langen Pausen. Oder sie führen das Projekt sogar einmal durch den Analysator. Infolge dieses Ansatzes erkennt der Analysator häufig nichts Ernstes oder findet etwas wie die Beispiele, die wir betrachten, was die Funktionalität des Spiels möglicherweise nicht besonders beeinflusst hat. Man hat den Eindruck, dass der Analysator nicht besonders nützlich ist. Nun, er hat solche Orte gefunden, funktioniert aber immer noch.
Tatsache ist, dass ähnliche Stellen, an denen der Fehler nicht maskiert wurde, sondern eindeutig zu einem Fehler im Programm führte, bereits durch stundenlanges Debuggen korrigiert wurden, Tests durchlaufen und die Testabteilung. Infolgedessen zeigt der Analysator bei der Überprüfung eines Projekts nur die Probleme an, die sich in keiner Weise manifestiert haben. Manchmal gibt es unter solchen Problemen auch schwerwiegende Momente, die den Betrieb des Programms wirklich beeinflusst haben, aber die Wahrscheinlichkeit, dass das Programm ihrem Pfad folgt, war gering, und daher war dieser Fehler den Entwicklern unbekannt.
Daher ist es äußerst wichtig, die Nützlichkeit der statischen Analyse erst nach ihrer regelmäßigen Verwendung zu bewerten. Wenn ein einmaliger Durchlauf von PVS-Studio solche verdächtigen und ungenauen Stellen im Code dieses Spiels enthüllte, wie viele offensichtliche Fehler dieser Art mussten während des Entwicklungsprozesses lokalisiert und korrigiert werden.
Verwenden Sie regelmäßig einen statischen Analysator!
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Victoria Khanieva. Amnesia: The Dark Descent oder Wie man vergisst, Copy Paste zu reparieren .