Jump to content
  • Advertisement
  • 10/14/15 07:12 AM
    Sign in to follow this  

    Problems Found in Appleseed Source Code

    General and Gameplay Programming

    PVS-studio team
    The majority of the projects we report about in these articles contain dozens of PVS-Studio analyzer warnings. Of course we choose just a small portion of data from the analyzer report to be in our articles. There are some projects though, where the quantity of warnings is not that high and the number of some interesting "bloomers" is just not enough for an article. Usually these are small projects, which ceased developing. Today I'm going to tell you about Appleseed project check, the code of which we found quite high-quality, from the point of view of the analyzer.

    Introduction

    Appleseed is a modern, open source, physically-based rendering engine designed to produce photorealistic images, animations and visual effects. It provides individuals and small studios with an efficient, reliable suite of tools built on robust foundations and open technologies. This project contains 700 source code files. Our PVS-Studio analyzer found just several warnings of 1st and 2nd level that could be of interest to us.

    Check Results

    V670 The uninitialized class member m_s0_cache is used to initialize the m_s1_element_swapper member. Remember that members are initialized in the order of their declarations inside a class. animatecamera cache.h 1009 class DualStageCache : public NonCopyable { .... S1ElementSwapper m_s1_element_swapper; //<==Line 679 S1Cache m_s1_cache; S0ElementSwapper m_s0_element_swapper; S0Cache m_s0_cache; //<==Line 683 }; FOUNDATION_DSCACHE_TEMPLATE_DEF(APPLESEED_EMPTY) DualStageCache( KeyHasherType& key_hasher, ElementSwapperType& element_swapper, const KeyType& invalid_key, AllocatorType allocator) : m_s1_element_swapper(m_s0_cache, element_swapper)//warning... // warning: referring to an uninitialized member , m_s1_cache(m_s1_element_swapper, allocator) , m_s0_element_swapper(m_s1_cache) , m_s0_cache(key_hasher, m_s0_element_swapper, invalid_key) { } The analyzer found a possible error in the constructor class initialization. Judging by the comment: "warning: referring to an uninitialized member", which has already been in the code, we see that the developers know that for the m_s1_element_swapper field initialization another uninitialized m_s0_cache field may be used. They are not correcting it though. According to the language standard, the order of initialization of the class members in the constructor goes in their declaration order in the class. V605 Consider verifying the expression: m_variation_aov_index [lessthan] ~0. An unsigned value is compared to the number -1. appleseed adaptivepixelrenderer.cpp 154 size_t m_variation_aov_index; size_t m_samples_aov_index; virtual void on_tile_end( const Frame& frame, Tile& tile, TileStack& aov_tiles) APPLESEED_OVERRIDE { .... if (m_variation_aov_index < ~0) //<== aov_tiles.set_pixel(x, y, m_variation_aov_index, ....); if (m_samples_aov_index != ~0) //<== aov_tiles.set_pixel(x, y, m_samples_aov_index, ....); .... } The inversion result of ~0 is -1, having the int type. Then this number converts into an unsigned size_t type. It's not crucial, but not really graceful. It is recommended to specify a SIZE_MAX constant in such expression right away. At first glance there is no evident error here. But my attention was drawn by the usage of two different conditional operators, though both conditions check the same. The conditions are true if the variables are not equal to the maximum possible size_t type value (SIZE_MAX). These checks are differently written. Such a code looks very suspicious; perhaps there can be some logical error here. V668 There is no sense in testing the 'result' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. appleseed string.cpp 58 char* duplicate_string(const char* s) { assert(s); char* result = new char[strlen(s) + 1]; if (result) strcpy(result, s); return result; } The analyzer detected a situation when the pointer value, returned by the new operator, is compared to null. We should remember, that if the new operator could not allocate the memory, then according to the C++ language standard, an exception std::bad_alloc() would be generated. Thus in the Appleseed project, which is compiled into Visual Studio 2013, the pointer comparison with null will be meaningless. One day such function usage can lead to an unexpected result. It is assumed that the duplicate_string() function will return nullptr if it can't create a string duplicate. It will generate an exception instead, that other parts of the program may be not ready for. V719 The switch statement does not cover all values of the 'InputFormat' enum: InputFormatEntity. appleseed inputarray.cpp 92 enum InputFormat { InputFormatScalar, InputFormatSpectralReflectance, InputFormatSpectralIlluminance, InputFormatSpectralReflectanceWithAlpha, InputFormatSpectralIlluminanceWithAlpha, InputFormatEntity }; size_t add_size(size_t size) const { switch (m_format) { case InputFormatScalar: .... case InputFormatSpectralReflectance: case InputFormatSpectralIlluminance: .... case InputFormatSpectralReflectanceWithAlpha: case InputFormatSpectralIlluminanceWithAlpha: .... } return size; } And where is the case for InputFormatEntity? This switch() block contains neither a default section, nor a variable action with the InputFormatEntity value. Is it a real error or did the author deliberately miss the value? There are two more fragments (cases) like that:
    • V719 The switch statement does not cover all values of the InputFormat enum: InputFormatEntity. appleseed inputarray.cpp 121
    • V719 The switch statement does not cover all values of the InputFormat enum: InputFormatEntity. appleseed inputarray.cpp 182
    If there is no default section and handling of all variable values, you may possibly miss the code addition for a new InputFormat value and not be aware of that for a very long time. V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long int) strvalue appleseed snprintf.cpp 885 #define UINTPTR_T unsigned long int int portable_vsnprintf(char *str, size_t size, const char *format, va_list args) { const char *strvalue; .... fmtint(str, &len, size, (UINTPTR_T)strvalue, 16, width, //<== precision, flags); .... } Finally we found quite a serious error that shows up in a 64-bit version of the program. Appleseed is a cross-platform project that can be compiled on Windows and Linux. To get the project files we use Cmake. In the Windows compilation documentation it is suggested to use "Visual Studio 12 Win64" that's why except the general diagnostics (GA, General Analysis), I've also looked through the diagnostics of 64-bit errors (64, Viva64) of the PVS-Studio analyzer. The full identification code of UINTPTR_T macro looks like this: /* Support for uintptr_t. */ #ifndef UINTPTR_T #if HAVE_UINTPTR_T || defined(uintptr_t) #define UINTPTR_T uintptr_t #else #define UINTPTR_T unsigned long int #endif /* HAVE_UINTPTR_T || defined(uintptr_t) */ #endif /* !defined(UINTPTR_T) */ The uintptr_t is an unsigned, integer memsize-type that can safely hold a pointer no matter what the platform architecture is, although for Windows compilation was defined unsigned long int type. The type size depends on the data model, and unlike Linux OS, the long type is always 32-bits in Windows. That's why the pointer won't fit into this variable type on Win64 platform.

    Conclusion

    All in all the Appleseed project, which is quite a big one, contains only a few analyzer's warnings. That's why it proudly gets a medal "Clear Code" and can no longer be afraid of our unicorn.
    image2.png


      Report Article
    Sign in to follow this  


    User Feedback


    Guys, this is what sending a pull request is for, not for a public airing of open-sourced dirty laundry. If all you want to do is market your code analysis tools, buy a banner.

     

    (I'm calling this comment a positive, constructive one, because it gives specific advice that would improve PVS-Studio's public image and reputation in the eyes of the people likely to use it)

    Share this comment


    Link to comment
    Share on other sites

    Guys, this is what sending a pull request is for, not for a public airing of open-sourced dirty laundry. If all you want to do is market your code analysis tools, buy a banner.
     
    (I'm calling this comment a positive, constructive one, because it gives specific advice that would improve PVS-Studio's public image and reputation in the eyes of the people likely to use it)

    While targeting a smaller opensource project made be rude, I find these articles incredibly helpful. PVS Studio has also targeted major engines like Unreal and idTech4 (Doom3).

    Everybody has errors in their code. I don't get the impression that PVS Studio is fat-shaming/bug-shaming them, but giving real-world examples of common and uncommon mistakes, and showing how their static analyzer catches them.

    In fact, in this article, he's even saying that this opensource project has fewer serious errors than the professional engines.

     

    "Today I'm going to tell you about Appleseed project check, the code of which we found quite high-quality, from the point of view of the analyzer."
    "All in all the Appleseed project, which is quite a big one, contains only a few analyzer's warnings. That's why it proudly gets a medal "Clear Code""

    Sometimes he even gets permission before sharing the project publicly, though I'm not sure if that's always the case.

     

    I do hope they are pull-requested back though.

    (I upvoted because your comment was a good comment, despite me disagreeing)

     

     

     

    There is no sense in testing the 'result' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. appleseed string.cpp 58

     

    This comment is worth suggesting the 'nothrow new operator'.

     

    Many game developers disable exceptions, because of poor support on the previous generations of consoles.

     

    In this situation, the Appleseed developers probably had -fno-exceptions enabled, mistakenly thinking that'd make 'new' return null on failure.

     

    The warning should suggest: "use 'new (nothrow)' if you want 'new' to return null on failure"

    Share this comment


    Link to comment
    Share on other sites

    While targeting a smaller opensource project made be rude, I find these articles incredibly helpful.
    Yeah, that's the problem. I agree on both points.

     

    If PVS-Studio can provide a disclaimer that the owners/maintainers of a project have, by the time the articles were published, either agreed to these public airings or accepted the pull-request-equivalent into their codebase, then I will have no cause for complaint whatsoever. The results of these code-analysis tools is a sort of pair-programming code review, and the most rude place to announce the results of your half of that review? Is in public eye, instead of with your partner.

    Share this comment


    Link to comment
    Share on other sites

    Thanks a lot for your feedback. We are glad to see such vivid discussions of our articles, even though there are not only positive comments. Speaking about pull request: we always let the code writers know about the issues we find. But we also don't want to conceal the fact that we write such articles not only for the sake of bringing some virtue to the mankind. Our additional aim is to demonstate abilities of the PVS-Studio analyzer to the developers who are not using it yet :) Concerning banners and advertisement, we definitely try these channels as well, but the effect is much less than from an article publication. If you have any other questions, feel free to ask.

    Share this comment


    Link to comment
    Share on other sites

    Guys, this is what sending a pull request is for, not for a public airing of open-sourced dirty laundry. If all you want to do is market your code analysis tools, buy a banner.


    The code is open-sourced. Authors expect people being able to see, download, compile and test (in other words, use) the code. I'm sure most of them welcome reviews, too - after all, they make their job easier. So, you say "dirty laundry", I say "understandable human error". Hasn't occured to me to think less of developers for finding a few mistakes in thousands of lines of their code.

    That said, feel free to poke around my code. ;)

    Share this comment


    Link to comment
    Share on other sites

    Thanks a lot for your feedback. We are glad to see such vivid discussions of our articles, even though there are not only positive comments. Speaking about pull request: we always let the code writers know about the issues we find. But we also don't want to conceal the fact that we write such articles not only for the sake of bringing some virtue to the mankind. Our additional aim is to demonstate abilities of the PVS-Studio analyzer to the developers who are not using it yet smile.png Concerning banners and advertisement, we definitely try these channels as well, but the effect is much less than from an article publication. If you have any other questions, feel free to ask.

     

    Give me a break. It's code shaming and I find it to be in poor taste.

    Share this comment


    Link to comment
    Share on other sites

    THIS is code shaming. Starting a thread for the purpose of mocking someone else's public code.

     

    What this article does is examines and discusses someone's public code for education and to promote their tool. It also compliments the quality of the code twice.

     

    Intelligently critiquing something is not automatically "shaming". Taking a piece of work someone made, even if the person is still alive, and discussing the quality of it has always been acceptable in scientific and creative settings.

     

    I guess the difference is, when you do it in a classroom, the painter/author/sculptor/composer/director isn't right in front of you, but when you do it on the internet he might happen across it?

     

    You've never discussed how terrible a movie or game or song is, even mocking it?

    Here, it's intellectually discussing it, in a non-mocking way.

     

    Taking this to the logical conclusion, the gist seems to be, it's okay to mock and shame people if they don't know you're doing it ("because what they don't know can't hurt them!"), but not acceptable to critique their work publicly even in a friendly manner ("because we have to protect their feelings!")? I don't agree with that. There is a kind of consistency in that thought process, but it's one that ultimately leads to duplicity.

     

    I know this isn't what you're explicitly saying, but I'm trying to extrapolate the result to show why I think it doesn't make sense.

     

    Maybe it seems more "real" to us as programmers, because we recognize we make the same coding mistakes, and we also have code publicly shared. So it seems suddenly more likely that our own work might be analyzed. Whereas, not being movie directors, our culture is fine with laughing and insulting movies and the actors and directors who made them, or even mocking guitarists or whomever in small music bands releasing albums we hate.

     

    Personally, I'd rather not mock anyone, publicly or privately, but have the freedom to critique anything publicly or not.

    Share this comment


    Link to comment
    Share on other sites

    Well I still think it's code shaming as it's a one sided unsolicited code review. If they have in fact asked the authors if they could do this then I am very sorry but there is no mention of it in the article.

     

    With that said, what is to say they are errors or issues in the first place? So many time people have come running with 'The Bug' that isn't a bug actually (myself included) because they didn't know the big picture or understand the code.

     

    V670: this error could actually be fine if the constructor just wants a reference or address of the variable.

     

    V668: if the project doesn't use exceptions and/or has a custom allocator then the test makes sense. Still not even an issue really.

     

    V719: is there a compiler known to man that won't report this?

     

    Half if not all of these so called issues could be caught with full warnings on and warnings set to errors, which you should do anyway. These aren't articles by the way. These are full screen ads that use the assets of reputable projects. Ads that they run for free...

    Share this comment


    Link to comment
    Share on other sites
    I'll add my support for articles of this type. I find them very interesting, and if I'd released any code in an open-source manner, I'd consider it quite reasonable for it to be inspected by static analysis tools and the results shared.

    Given the cost of such tools, I'd be very grateful to have it run on my code for nothing.

    Share this comment


    Link to comment
    Share on other sites

     

     

    returned by the new operator, is compared to null. We should remember, that if the new operator could not allocate the memory, then according to the C++ language standard, an exception std::bad_alloc() would be generated.

     

    (Not talking about *this* project..) This is true; but however, this kind of null checks might be necessary if they are using compiler flags that disables exceptions.. (eg. -fno-exceptions). Some game engines may choose to disables RTTI and Exceptions if they implemented their own methods and don't want to waste resources for provided methods.

    Share this comment


    Link to comment
    Share on other sites

    It seems in the next 10 years we wont be able to make bug reports since it might break the poor poor developer's heart to know his pretty beautiful code isn't a perfect and unique snowflake -_-

     

    Or I could read this as an informative article and think no less of the Appleseed developers who pulled off such project. Its that too much to ask for?

    Share this comment


    Link to comment
    Share on other sites

     

    returned by the new operator, is compared to null. We should remember, that if the new operator could not allocate the memory, then according to the C++ language standard, an exception std::bad_alloc() would be generated.

     
    (Not talking about *this* project..) This is true; but however, this kind of null checks might be necessary if they are using compiler flags that disables exceptions.. (eg. -fno-exceptions). Some game engines may choose to disables RTTI and Exceptions if they implemented their own methods and don't want to waste resources for provided methods.

    As far as I can tell, -fno-exceptions doesn't make 'new' return null on failure.

    If your program is compiled with -fno-exceptions, but the standard library isn't, then an exception is still thrown that the program merely doesn't catch. If the standard library is also compiled with -fno-exceptions, then instead of throwing an exception, it calls abort() (still not returning null).

    The error is still important because people (i.e. me) assume that 'new' returns null if exceptions are disabled, but that doesn't appear to be true. This static analyzer's warning message should be more explicit in mentioning this (though I guess that'd make the static analyzer's error message somewhat compiler-specific).

    As far as I know, the only standardized way to make 'new' return null is by doing 'new (std::nothrow)'.

    Other exception-disabling flags for other compilers may actually have new() return null. If so, they are probably violating the standard. =P
    Likewise if the user overrides 'new' with his own version that returns null, that might be undefined behavior, but I'm not sure.

    "Required behavior: Return a non-null pointer to suitably aligned storage (3.7.4), or else throw a bad_alloc exception. This requirement is binding on a replacement version of this function." - § 18.6.1.1.3 (N4296)

    Share this comment


    Link to comment
    Share on other sites

    I liked and like to read such nice write ups about a static code analysis with the knowledge that I do not have to search google 3 hours in order to turn off 3 obscure warnings, which right now have no negative consequences on run time. Instead I am educated and will probably make less errors in the first place. And most of the time I think: good that I code in C#, which seems to be far less prone to a lot of these errors.

    Share this comment


    Link to comment
    Share on other sites

    Hi all,

     

    I'm Franz, the founder and the lead developer of appleseed.

     

    I just found this lively discussion (purely by accident) and I thought I would chime in.

     

    I actually asked the PVS-Studio guys a few months ago if they would consider analyzing our code, because I was curious to see how our codebase would fare. Turned out pretty well I think.

     

    I'm personally a big fan of the blog posts by the PVS-Studio team. In addition to being interesting and informative, I believe they also are extremely helpful to the developers of the projects being analyzed. I'm convinced that ultimately they lead to actual bugs being discovered and fixed and eventually to better open source software for the users.

     

    Selling development tools is hard, so some form of advertisement is necessary. In my opinion, this is the best kind of advertisement possible for this kind of tools and I personnally don't mind it at all.

     

    Regarding the results:

     

    The check for 'new' returning a null pointer was a minor mistake. There was actually a single place where we were doing this. Everywhere else we actually catch std::bad_alloc exceptions (exceptions are enabled in our codebase).

     

    The only actual bug found by PVS-Studio is in a function (portable_snprintf) that we didn't write ourselves (we fixed it).

     

    Everything else is simply potentially risky or ambiguous code, but no other bugs were found. We did rewrite the offending fragments though, if only to lift the ambiguities and make the code more explicit.

     

    (We compile our entire codebase with full warnings and 'warnings as errors' on three different compilers, which might explain why so few issues were found.)

     

    Thanks for the discussion.

    Franz

    Share this comment


    Link to comment
    Share on other sites

    (I mean this only relating to how your post is written, as I think the warning itself is very useful for the programmer to see).

     

    I'm somewhat skeptical of the 64-bit warning.. though I would agree that it is dubious to write it that way without at least a comment or something.. but to say that it would fail on Win64 seems incorrect, as it would only fail without support for uintptr_t. Especially when you write in the same paragraph that the recommended compiler is VS2012 where it would obviously not fail.

     

    The version of that file I found with Google didn't have the same line-numbers as yours though and I didn't try to build it, so apologies if I'm mistaken on this...

     

     

    I also like these posts, I remember reading some other analysis you've posted and they're always informative and helps in becoming aware of common errors.

    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.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!