Jump to content
  • Advertisement
  • 12/29/15 06:14 AM
    Sign in to follow this  

    Not All is Fine in the Morrowind Universe

    General and Gameplay Programming

    PVS-studio team

    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.



      Report Article
    Sign in to follow this  


    User Feedback


    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. 

    Share this comment


    Link to comment
    Share on other sites

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

     

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

    Share this comment


    Link to comment
    Share on other sites

    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. :)

    Share this comment


    Link to comment
    Share on other sites

    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.

    Share this comment


    Link to comment
    Share on other sites

    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.

    Share this comment


    Link to comment
    Share on other sites

     

    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.

    Share this comment


    Link to comment
    Share on other sites

    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:)

    Share this comment


    Link to comment
    Share on other sites

    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.

    Share this comment


    Link to comment
    Share on other sites

    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:

    Share this comment


    Link to comment
    Share on other sites

    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)) {
        ...
    }
    

    Share this comment


    Link to comment
    Share on other sites

     

    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! 

    Share this comment


    Link to comment
    Share on other sites

    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.

    Share this comment


    Link to comment
    Share on other sites


    Create an account or sign in to comment

    You need to be a member in order to leave a comment

    Create an account

    Sign up for a new account in our community. It's easy!

    Register a new account

    Sign in

    Already have an account? Sign in here.

    Sign In Now

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!