Contents
V704. The expression is always false on newer compilers. Avoid using 'this == 0' comparison.
The analyzer has detected an expression of the 'this == 0' pattern. This expression may work well in some cases but it is extremely dangerous due to certain reasons.
Here is a simple example:
class CWindow {
HWND handle;
public:
HWND GetSafeHandle() const
{
return this == 0 ? 0 : handle;
}
};Calling the CWindow::GetSafeHandle() method for the null pointer 'this' will generally lead to undefined behavior, according to the C++ standard. But since this class' fields are not being accessed while executing the method, it may run well. On the other hand, two negative scenarios are possible when executing this code. First, since the this pointer can never be null, according to the C++ standard, the compiler may optimize the method call by reducing it to the following line:
return handle;Second, suppose we've got the following code fragment:
class CWindow {
.... // CWindow from the previous example
};
class MyWindowAdditions {
unsigned long long x; // 8 bytes
};
class CMyWindow: public MyWindowAdditions, public CWindow {
....
};
....
void foo()
{
CMyWindow * nullWindow = NULL;
nullWindow->GetSafeHandle();
}This code will cause reading from the memory at the address 0x00000008. You can make sure it's true by adding the following line:
std::cout << nullWindow->handle << std::endl;What you will get on the screen is the address 0x00000008, for the source pointer NULL (0x00000000) has been shifted in such a way as to point to the beginning of the CWindow class' subobject. For this purpose, it needs to be shifted by sizeof(MyWindowAdditions) bytes.
What's most interesting, the "this == 0" check turns absolutely meaningless now. The 'this' pointer is always equal to the 0x00000008 value at least.
On the other hand, the error won't reveal itself if you swap the base classes in CMyWindow's declaration:
class CMyWindow: public CWindow, public MyWindowAdditions{
....
};All this may cause very vague errors.
Unfortunately, fixing the code is far from trivial. Theoretically, a correct way out in such cases is to change the class method to static. This will require editing a lot of other places where this method call is used.
class CWindow {
HWND handle;
public:
static HWND GetSafeHandle(CWindow * window)
{
return window == 0 ? 0 : window->handle;
}
};Another way is to use the Null Object pattern which will also require plenty of work.
class CWindow {
HWND handle;
public:
HWND GetSafeHandle() const
{
return handle;
}
};
class CNullWindow : public CWindow {
public:
HWND GetSafeHandle() const
{
return nullptr;
}
};
....
void foo(void)
{
CNullWindow nullWindow;
CWindow * windowPtr = &nullWindow;
// Output: 0
std::cout << windowPtr->GetSafeHandle() << std::endl;
}It should be noted that this defect is extremely dangerous because one is usually too short of time to care about solving it, for it all seems to "work well as it is", while refactoring is too expensive. But code working stably for years may suddenly fail after a slightest change of circumstances: building for a different operating system, changing to a different compiler version (including update), and so on. The following example is quite illustrative: the GCC compiler, starting with version 4.9.0, has learned to throw away the check for null of the pointer dereferenced a bit earlier in the code (see the V595 diagnostic):
int wtf( int* to, int* from, size_t count ) {
memmove( to, from, count );
if( from != 0 ) // <= condition is always true after optimization
return *from;
return 0;
}There are quite a lot of real-life examples of problem code turned broken because of undefined behavior. Here are a few of them to underline the importance of the problem.
Example No. 1. A vulnerability in the Linux core
struct sock *sk = tun->sk; // initialize sk with tun->sk
....
if (!tun) // <= always false
return POLLERR; // if tun is NULL return errorExample No. 2. Incorrect work of srandomdev():
struct timeval tv;
unsigned long junk; // <= not initialized on purpose
gettimeofday(&tv, NULL);
// LLVM: analogue of srandom() of uninitialized variable,
// i.e. tv.tv_sec, tv.tv_usec and getpid() are not taken into account.
srandom((getpid() << 16) ^ tv.tv_sec ^ tv.tv_usec ^ junk);Example No. 3. An artificial example that demonstrates very clearly both compilers' aggressive optimization policy concerning undefined behavior and new ways to "shoot yourself in the foot":
#include <stdio.h>
#include <stdlib.h>
int main() {
int *p = (int*)malloc(sizeof(int));
int *q = (int*)realloc(p, sizeof(int));
*p = 1;
*q = 2;
if (p == q)
printf("%d %d\n", *p, *q); // <= Clang r160635: Output: 1 2
}As far as we know, none of the compilers has ignored the call of the this == 0 check as of the implementation date of this diagnostic, but it's just a matter of time because the C++ standard clearly reads (§9.3.1/1): "If a nonstatic member function of a class X is called for an object that is not of type X, or of a type derived from X, the behavior is undefined.". In other words, the result of calling any nonstatic function for a class with this == 0 is undefined. As I've said, it's just a matter of time for compilers to start substituting false instead of (this == 0) during compilation.
This diagnostic is classified as:
You can look at examples of errors detected by the V704 diagnostic. |
Was this page helpful?
