Not All is Fine in the Morrowind Universe

Published December 29, 2015 by Andrey Karpov, posted by PVS-studio team
Do you see issues with this article? Let us know.
Advertisement

I have checked the OpenMW project by PVS-Studio and written this tiny article. Very few bugs were found, so OpenMW team can be proud of their code.

OpenMW

OpenMW is an attempt to reconstruct the popular RPG Morrowind, a full-blown implementation of all of the game's specifics with open source code. To run OpenMW, you will need an original Morrowind disk. The source code can be downloaded from https://code.google.com/p/openmw/

Suspicious fragments found

Fragment No. 1 std::string getUtf8(unsigned char c, ToUTF8::Utf8Encoder& encoder, ToUTF8::FromType encoding) { .... conv[0xa2] = 0xf3; conv[0xa3] = 0xbf; conv[0xa4] = 0x0; conv[0xe1] = 0x8c; conv[0xe1] = 0x8c; <<<<==== conv[0xe3] = 0x0; .... } PVS-Studio diagnostic message: V519 The 'conv[0xe1]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 103, 104. openmw fontloader.cpp 104 I guess it is a typo. It is the 0xe2 index that should be probably used in the marked line. Fragment No. 2 enum Flags { .... NoDuration = 0x4, .... } bool CastSpell::cast (const ESM::Ingredient* ingredient) { .... if (!magicEffect->mData.mFlags & ESM::MagicEffect::NoDuration) .... } PVS-Studio diagnostic message: V564 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. openmw spellcasting.cpp 717 Here we deal with a mistake related to operation precedence. At first, the (!magicEffect->mData.mFlags) statement is executed which evaluates either to 0 or 1. Then the statement 0 & 4 or 1 & 4 is executed. But it doesn't make any sense, and the code should most likely look as follows: if ( ! (magicEffect->mData.mFlags & ESM::MagicEffect::NoDuration) ) Fragment No. 3 void Clothing::blank() { mData.mType = 0; mData.mWeight = 0; mData.mValue = 0; mData.mEnchant = 0; mParts.mParts.clear(); mName.clear(); mModel.clear(); mIcon.clear(); mIcon.clear(); mEnchant.clear(); mScript.clear(); } PVS-Studio diagnostic message: V586 The 'clear' function is called twice for deallocation of the same resource. Check lines: 48, 49. components loadclot.cpp 49 The mIcon object is cleared twice. The second clearing is redundant or something else should have been cleared instead. Fragment No. 4 void Storage::loadDataFromStream( ContainerType& container, std::istream& stream) { std::string line; while (!stream.eof()) { std::getline( stream, line ); .... } .... } PVS-Studio diagnostic message: V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. components translation.cpp 45 When working with the std::istream class, calling the eof() function to terminate the loop is not enough. If a failure occurs when reading data, the call of the eof() function will return false all the time. To terminate the loop in this case, you need an additional check of the value returned by fail(). Fragment No. 5 class Factory { .... bool getReadSourceCache() { return mReadSourceCache; } bool getWriteSourceCache() { return mReadSourceCache; } .... bool mReadSourceCache; bool mWriteSourceCache; .... }; PVS-Studio diagnostic message: V524 It is odd that the body of 'getWriteSourceCache' function is fully equivalent to the body of 'getReadSourceCache' function. components factory.hpp 209 I guess the getWriteSourceCache() function should look like this: bool getWriteSourceCache() { return mWriteSourceCache; } Fragments No. 6, 7, 8 std::string rangeTypeLabel(int idx) { const char* rangeTypeLabels [] = { "Self", "Touch", "Target" }; if (idx >= 0 && idx <= 3) return rangeTypeLabels[idx]; else return "Invalid"; } PVS-Studio diagnostic message: V557 Array overrun is possible. The value of 'idx' index could reach 3. esmtool labels.cpp 502 Here we see an incorrect check of an array index. If the idx variable equals 3, an array overrun will occur. The correct code: if (idx >= 0 && idx < 3) A similar defect was found in two other fragments:
  • V557 Array overrun is possible. The value of 'idx' index could reach 143. esmtool labels.cpp 391
  • V557 Array overrun is possible. The value of 'idx' index could reach 27. esmtool labels.cpp 475
Fragment No. 9 enum UpperBodyCharacterState { UpperCharState_Nothing, UpperCharState_EquipingWeap, UpperCharState_UnEquipingWeap, .... }; bool CharacterController::updateWeaponState() { .... if((weaptype != WeapType_None || UpperCharState_UnEquipingWeap) && animPlaying) .... } PVS-Studio diagnostic message: V560 A part of conditional expression is always true: UpperCharState_UnEquipingWeap. openmw character.cpp 949 This condition is very strange. In its current form, it can be reduced to if (animPlaying). Something is obviously wrong with it. Fragments No. 10, 11 void World::clear() { mLocalScripts.clear(); mPlayer->clear(); .... if (mPlayer) .... } PVS-Studio diagnostic message: V595 The 'mPlayer' pointer was utilized before it was verified against nullptr. Check lines: 234, 245. openmw worldimp.cpp 234 Similar defect: V595 The mBody pointer was utilized before it was verified against nullptr. Check lines: 95, 99. openmw physic.cpp 95 Fragment No. 12 void ExprParser::replaceBinaryOperands() { .... if (t1==t2) mOperands.push_back (t1); else if (t1=='f' || t2=='f') mOperands.push_back ('f'); else std::logic_error ("failed to determine result operand type"); } PVS-Studio diagnostic message: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw logic_error(FOO); components exprparser.cpp 101 The keyword throw is missing. The fixed code should look like this: throw std::logic_error ("failed to determine result operand type");

Conclusion

Purchase a PVS-Studio for in your team, and you will save huge amount of time usually spent on eliminating typos and diverse bugs.

Cancel Save
0 Likes 16 Comments

Comments

TheChubu

You might have cloned the wrong repository there, that Google Code repository is from almost two years ago. OpenMW moved to GitHub, this is the repo they link in their site:

https://github.com/OpenMW/openmw

December 24, 2015 06:16 AM
PVS-studio team

You might have cloned the wrong repository there, that Google Code repository is from almost two years ago. OpenMW moved to GitHub, this is the repo they link in their site:

https://github.com/OpenMW/openmw

Thank you for pointing to a new repository. We will check the new code very soon.

December 24, 2015 12:06 PM
Alpha_ProgDes

I'm assuming you've shared this information with the OpenMW team?

In any case, I can definitely see the benefit of your software.

December 24, 2015 04:13 PM
Eck
Eck

I think you should talk more about the purpose of this article. As it stands, it looks like you're just pointing out problems in other people's code and I don't really see the point of that in a game development article.

Talk a little bit more about the benefits of using code analysis tools.

The types of problems your software find seems very cool though. :)

December 24, 2015 05:29 PM
Servant of the Lord

We had an extended discussion about that earlier.

December 24, 2015 05:40 PM
Bacterius

Sorry, but the last line is just blatant advertising: "Purchase a PVS-Studio for in your team, and you will save huge amount of time usually spent on eliminating typos and diverse bugs.". Uh-uh. I don't mind such articles but that sentence just does not belong here.

Show the value of your static analysis software by writing informative articles and letting developers evaluate the result for themselves, that's great and I support it. But this is supposed to be a technical article, not your website's landing page.

December 24, 2015 10:05 PM
JeffRS

Yes this is obviously promotional material thinly disguised as a tech article. There are multiple articles on here posted by the same person to promote this software. It either needs to be made clear that it is an advertisement or at the very least present the content in a way that has some value other than showing the functionality of this commercial software and then telling people to purchase it.

Perhaps present some interesting statistics at least. What are the most common types of problems found? What are the average amount of errors found per 100/1000/10,000 lines of code. What are some of the real world results gained from fixing all the detected problems... speed increase, size decrease, stability?

For what it's worth the software seems excellent and very useful. I just feel like this is an abuse of the spirit of this article system.

December 25, 2015 02:40 AM
PVS-studio team

Perhaps present some interesting statistics at least. What are the most common types of problems found? What are the average amount of errors found per 100/1000/10,000 lines of code. What are some of the real world results gained from fixing all the detected problems... speed increase, size decrease, stability?

Thank you for your constructive feedback. In the nearest future, we are planning to recheck the source code of OpenMV and compare the dynamics of the first bug report and the second bug report. Right now our tool does't provide any interesting statistics reposrts, but it is a good idea.
Speaking about interesting bugs statistics in open-source projects, I advise you to review this article http://www.viva64.com/en/b/0300/ It should be interesting.
December 25, 2015 11:40 AM
PVS-studio team

Sorry, but the last line is just blatant advertising:

Thank you for the feedback. We will be more accurate next time and ask the editor to check our article once more. Also, I will tell you a small story, that I hope you will find funny. The word "blatant" if you transliterate it in Russian sounds like the word from the jail world. This word means a person/group that is conntected with criminality and has some respect. I am not a linguist but it looks luke this word has interesting etiminology:)

December 25, 2015 11:53 AM
Althar

Am I the only one wanting to see an article on "PVS-Studio" checked by itself?

December 26, 2015 11:14 AM
swiftcoder

Let's not rehash the code-shaming/this-is-just-advertising discussion on every one of these posts, please?

The prevailing opinion over the last few discussions has been that these articles are instructive as to interesting bugs and language idiosyncrasies, and that PVS-studio makes a good-faith effort to bring benefit to the projects being analysed.

December 27, 2015 01:57 PM
PVS-studio team

Am I the only one wanting to see an article on "PVS-Studio" checked by itself?

Thank you for the feedback! We reqularly check PVS-Studio with the help of PVS-Studio. So, you can be sure, it has no errors:) Also, we has an article that desribes how we test our tool http://www.viva64.com/en/a/0047/#ID0EMQAE

December 28, 2015 06:50 AM
Servant of the Lord

We reqularly check PVS-Studio with the help of PVS-Studio. So, you can be sure, it has no errors :)

We can only be sure PVS-Studios has no errors that PVS-Studio can detect. :lol:

December 28, 2015 09:11 PM
Zao
Zao

Constructs like the one in fragment #4 is often erroneous even if you check both flags.

eof() will not be true until a read attempt is made across the end of the file.

Unless there is additional checking below the std::getline call, it will run one final failing time after all the data is consumed and the code below may expect the string to contain valid data. Instead the code ought to inspect the state of the stream after a read operation. Typically you would design a "read all the lines" loop like:


while (std::getline(is, str)) {
    ...
}
January 04, 2016 03:16 AM
PVS-studio team

You might have cloned the wrong repository there, that Google Code repository is from almost two years ago. OpenMW moved to GitHub, this is the repo they link in their site:

https://github.com/OpenMW/openmw

Thank you for pointing to a new repository. We will check the new code very soon.

I have some news regarding the new source code. We scanned it and found very minor errors. So, congratulations to OpenMW!

January 15, 2016 07:55 AM
PVS-studio team

Guys, if you have suggestions about the projects that can be checked by our analyzer, please write to us! We are ready to fight:). We have started supporting C# from the 6.00 version, so C# open-source projects are also welcome.

January 15, 2016 08:01 AM
You must log in to join the conversation.
Don't have a GameDev.net account? Sign up!
Advertisement