V704. The expression is always false on newer compilers. Avoid using 'this == 0' comparison.
The analyzer has detected an expression of the this == 0
form. 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;
}
};
According to the C++ standard, calling the CWindow::GetSafeHandle()
method for the this
null pointer usually leads to undefined behavior. However, since no data members of this class are accessed while the method is being executed, it may still work. On the other hand, there are two potentially problematic execution scenarios for 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, let's say we have 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 causes reading from the memory at the 0x00000008
address. You can make sure it's true by adding the following line:
std::cout << nullWindow->handle << std::endl;
The 0x00000008
address will be displayed, as the original NULL
pointer (0x00000000
) has been moved to point to the beginning of a subobject within the CWindow
class. To achieve this, it needs to be shifted by sizeof(MyWindowAdditions)
bytes.
Then, the this == 0
check turns absolutely meaningless. The this
pointer is always 0x00000008
at least.
On the other hand, the error will not emerge if you swap the base classes in the CMyWindow
declaration:
class CMyWindow: public CWindow, public MyWindowAdditions{
....
};
All this can lead to extremely non-obvious errors.
Fixing the code is far from trivial. A correct way in such cases is to change the class method to static. This requires editing a lot of other code fragments 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 also requires 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;
}
Note. This defect is particularly dangerous because there is almost never time to address it—after all, "everything works as is," and the cost of refactoring is high. However, code that has worked for years may suddenly break at the slightest change in conditions: building for a different OS, changing compiler versions (including its updates), and so on.
For example, starting with the 4.9.0 version, the GCC compiler has learned to eliminate null checks for pointers that were dereferenced earlier in the code (see the V595 diagnostic rule):
int wtf( int* to, int* from, size_t count ) {
memmove( to, from, count );
if( from != 0 ) // <= the condition is always true after optimization
return *from;
return 0;
}
There are many real-life examples of code turned broken because of undefined behavior. Here are a few of them to highlight the issue:
Example 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 error
Example 2. Incorrect work of srandomdev():
struct timeval tv;
unsigned long junk; // <= uninitialized on purpose
gettimeofday(&tv, NULL);
// LLVM: the srandom() analogue of the uninitialized variable,
// i.e. tv.tv_sec, tv.tv_usec and getpid() are not considered.
srandom((getpid() << 16) ^ tv.tv_sec ^ tv.tv_usec ^ junk);
Example 3. A synthetic example that demonstrates both how compilers can aggressively optimize due to undefined behavior and the new "shoot yourself in the foot" possibilities that come with it:
#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
}
At the time of writing this diagnostic rule, none of the compilers ignore the call of the this == 0
check. However, the C++ standard clearly states (§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 non-static function for a class with this == 0
is undefined. So, it's just a matter of time for compilers to start replacing (this == 0)
with false
at compile time.
This diagnostic is classified as:
You can look at examples of errors detected by the V704 diagnostic. |