Search code examples
c++qtpvs-studio

False positive V595 The '_parent' pointer was utilized before it was verified against nullptr

I use PVS-Studio for my project Torrent File Editor. There is one false positive. Here no real problem but I get such error:

torrent-file-editor/abstracttreenode.h:138: error: V595 The '_parent' pointer was utilized before it was verified against nullptr. Check lines: 138, 139.

Code snippet:

inline T *sibling(int row) const
{
    Q_ASSERT(_parent);
    Q_ASSERT(row < _parent->childCount()); // -V595 PVS-Studio
    return _parent ? _parent->child(row) : nullptr;
}

Here Q_ASSERT is only Debug version checking. Such checking is not executed in Release version. For Release I use _parent ? ... : ... to prevent possible crash. So it's fully OK that check duplicated in Debug version.

I supress this false positive with special comment. So this is not a problem but think that PVS-Studio should handle this case.

Solution

  • V595 diagnostic logic is simple. A warning is issued in case if in the beginning a pointer is dereferenced, then is verified for equality to nullptr.

    Of course there is a number of situations when the analyzer will be quiet, having met such a pattern. Including a situation when a pointer is not equal to nullptr, so the analyzer will keep quiet.

    However, the Q_ASSERT(_parent) does not guarantee that the pointer _parent is nonzero. If _parent is zero, the Q_ASSERT statement will output the following message using the qFatal function. If you are using the default message handler this function will abort to create a core dump.

    You can install your own handler, which will continue to run the program. So theoretically the analyzer is right. Potential dereference of a null pointer may occur.

    We are not theorists but practics and we realize that this code should be regarded as correct. The analyzer is not familiar with such code view, where the macro Q_ASSERT is used yet. We will modify the analyzer, so that it begins to perceive such patterns of code as correct. I.e. in the future analyzer will assume that here:

    Q_ASSERT(_parent);
Q_ASSERT(row < _parent->childCount());

    _parent->childCount() function call is never executed, if the pointer _parent is equal to nullptr. If the pointer is null, then the program will stop working earlier because of calling qFatal().

    Of course, as I have already said above, you can change the handler's behavior and it will not cause abort program. However, in practice, no one will change the handler and write such code that we are considering here.

    This could be the finish point of the answer. So, we’ll improve the analyzer and that’s all. However, it is impossible to foresee all possible options. How to suppress a warning, if it is our own macro?

    Let’s suppose that this self-made errors logging system and the analyzer knows nothing about custom function Foo().

    void Foo(bool expr);
#define Q_ASSERT(expr) Foo(expr);

inline T *sibling(int row) const
{
  Q_ASSERT(_parent);
  Q_ASSERT(row < _parent->childCount())
  return _parent ? _parent->child(row) : nullptr;
}

    The easiest but not the best way is to explicitly mark warning as false is using comments:

    Q_ASSERT(row < _parent->childCount())   //-V595

    Another option is to change the style of writing code and write as follows:

    inline T *sibling(int row) const
{
  if (_parent == nullptr)
  {
    Q_ASSERT(false);
    return nullptr;
  }
  Q_ASSERT(row < _parent->childCount());
  return _parent->child(row);
}

    For such code the analyzer will not issue warning V595, as there is no reason for this. The code became a bit longer, but in my opinion it is now more logically correct and secure. I recommend this way of dealing with this type of warnings.

    And the last is to use warnings suppression mechanism in macros. To do this in the header file where the macro is defined, you should write a comment:

    //-V:Q_ASSERT:595

    After this warnings will disappear. Of course, it is not always possible to change the file where the macro is declared. Then, you can use one of the global files. In Visual C++ projects, a good candidate is the stdafx.h. Another option is to use the diagnostic configuration files (pvsconfig). All these methods are described in detail in documentation in section "Suppression of false alarms". The markup base also exists.