On my previous post i’ve talked about what tools do we use at Mozilla for static analysis, now i want to take a closer look on some specific checkers triggered by Coverity and why i do consider them very useful.

1. Side effect in assertion
   This is a very common mistake that is pretty hard to discover when reviewing, this is triggered when inside an assert an assignment expression is present instead of a logical comparison.  There are two key problems that can occur in this context:
   • As most of the asserts are intended only for debug builds, on release the assignment will not take place resulting in a different behaviour, that can lead to a bug.
   • The result of the assignment expression is the assignee itself thus the following assert will never be false:

     ASSERT(a = 3);

   This is an actual issue from Coverity that signals the problem and the eventual side effects that there could be.
   

2. Side effect in assertion
   Often happens that a pointer is null checked on a brach, and o separate code path this check is no longer performed despite that the pointer is dereferenced. In case the pointer was null the resulting of the dereference is undefined.From this example is obvious that pointer aAcc is null checked on line 649 and later on line 651 it is dereferenced without null checking it.  But what causes the null pointer dereference and how Coverity figured out that this is an actual issue? Looking further in the code we find:
   There are a couple of exceptions when dereferencing a pointer that’s null will not have an impact and is perfectly correct to do this:

   class Test {
   public:
     void print(std::string str) {
       std::cout<<str<<std::endl; } }; int main(int argc, const char **argv) { Test *myCls = nullptr; myCls->print("Just A Test");
     return 0;
   }
   

   The pointer is not needed to call the function, the type of the pointer is known so the code for the function is known and inside print we don’t dereference pointer this. In case the method would have been virtual the call would have failed since this would have been dereferenced.

3. Buffer not null terminated
   This type of issue can happen in many situations but most likely when a copying with the length of the same size as the target size. This is very useful if the buffer is treated as a null terminated string in later operations, a buffer overflow or over-read may occur.
   In this example the correct approach would be to have the value of the 3rd parameter from strncpy be IFNAMSIZ – 1.
4. Result is not floating-point
   The result of a division is truncated to an integer witch translates to a loss of precision in calculus. When dividing two values of integer types, integer division is used, which ignores any remainder. When such a result is used in a context expecting a floating-point number, it is likely that floating-point division was intended.
   In this context type of nsrecord is integer, and in order to fix this one of the variables from the division expression should be first casted to float.
5. Resource leak
   The system resources will not be freed and reused thus reducing the future availability of resources.  It’s a very common issue, that in most situations tends to appear when naked pointers are used.
   
   On line 87 an allocation is demanded if it fails the function will return on line 90. The problem appears in switch statement on the default branch where it just returns without freeing the allocated resource.
6. Nesting level does not match indentation
   Code that should be used conditionally is actually executed outside of the conditional branch. This is based on the indentation of code, like below:
7. Uninitialised class members
   This consists of two sub-checkers, one for pointers and the other for scalars. If we are talking about scalars the uninitialised one will contain an arbitrary value. For an uninitialised pointer field it will point to a random memory location. These issues are very common and can be easily avoided by initialising  all of the members from that class in the constructors.
    

Coverity is a great tool, with a low rate of false positive, it really achieves at being one of the best, but despite this i was very sad that it doesn’t have a very good support for thread analysis and nor it has support for any thread sanitisation checkers.

 

https://blog.influx.ro/2016/07/07/a-more-detailed-look-on-some-of-coveritys-checkers-for-static-analysis/