Exploring Nau Engine codebase: pt.3

In the final part of our trilogy on Nau Engine, we'll focus on errors that occur when implementing classes. The examples in this article show how even small defects can lead to serious issues in the application operation. As mentioned at the beginning, this is the third article on checking Nau Engine. In the first part, we focused on the engine's functionality, highlighting three key error categories: memory issues, copy-paste code, and logic errors. The second part discussed important aspects of optimization and performance improvement. Now, let's talk about errors in class implementation that the PVS-Studio static analyzer has detected. Fragment N1 template struct AttributeField { using Key = K; using Value = T; T value; AttributeField(T&& inValue) : value(std::move(value)) { } AttributeField(TypeTag, T&& inValue) : value(std::move(inValue)) { } }; The PVS-Studio warning: V546 Member of a class is initialized with itself: 'value(std::move(value))'. attribute.h 118 This code fragment contains an error: the AttributeField::value data member is initialized by itself. As a result, the variable is used before it's actually initialized, which leads to undefined behavior. Such code can cause unexpected errors, make debugging more difficult, and leave the object in an invalid state. To fix the initialization, developers should use the passed inValue argument, not the class member: AttributeField(T&& inValue) : value(std::move(inValue)) { } Fragment N2 class NAU_ANIMATION_EXPORT AnimationInstance : public virtual IRefCounted { .... private: friend class KeyFrameAnimationPlayer; AnimationState m_animationState; nau::Ptr m_animation; AnimationAssetRef m_animationAsset; .... eastl::string m_name; }; AnimationInstance::AnimationInstance(const eastl::string& name, AnimationAssetRef&& assetRef) : m_name(name) { m_animationAsset = std::move(assetRef); } The PVS-Studio warning: V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_animationState. animation_instance.cpp 28 This snippet contains the constructor of the AnimationInstance class, which takes two parameters: an object name and an rvalue reference to assetRef. Notice that only the values for m_name and m_animationAsset are explicitly set in the initialization list and constructor body, while the m_animationState data member remains uninitialized. This can lead to undefined behavior because uninitialized class data members may contain garbage values due to the default-initialization. The code author might have forgotten to initialize all the class data members. To ensure the object is in a valid state upon creation, it's recommended to explicitly initialize every class data member—either in the constructor initialization list or by providing default values at declaration. Fragment N3 class NAU_KERNEL_EXPORT LzmaLoadCB : public IGenLoad { public: .... ~LzmaLoadCB() { close(); } .... }; void LzmaLoadCB::close() { if (isStarted && !isFinished && !inBufLeft && rdBufPos >= rdBufAvail) ceaseReading(); .... } The PVS-Studio warning: V1053 Calling the 'ceaseReading' virtual function indirectly in the destructor may lead to unexpected result at runtime. Check lines: 'dag_lzmaIo.h:27', 'lzmaDecIo.cpp:48', 'dag_genIo.h:249'. dag_lzmaIo.h 27 In this example, the LzmaLoadCB class implements a destructor where the close member function is called first, and then the [ceaseReading](https://github.com/NauEngine/NauEnginePublic/blob/020cbfec7bd4cff7ff4303e0f31d75eebed584be/engine/core/kernel/include/nau/dag_ioSys/dag_genIo.h#L249) member function, declared as virtual in the IGenLoad base class, is called when a certain condition is met. Calling virtual class functions in the context of object construction or destruction can be dangerous: at this point, the object has either not yet initialized its most derived parts (construction) or has already destroyed them (destruction). This means that if the ceaseReading virtual function is overridden in the most derived class, a virtual function from the current or base class will always be called at the moment of construction or destruction. This may lead to ignoring important functionality implemented in the derived class. To preserve correct program behavior, it's better to avoid such calls in constructors or destructors. Another case: V1053 Calling the 'Type' virtual function indirectly in the constructor may lead to unexpected result at runtime. Check lines: 318, 252, 245. d3dx12_state_object.h 318 Fragment N4 FsPath& FsPath::operator=(FsPath&& other) { m_path = std::move(other.m_path); other.m_path.clear(); return *this; } The PVS-Studio warning: V794 The assignment operator should be protected from the case of 'this == &other'. fs_path.cpp 36 The move assignment operator for the FsPath class, which tr

Apr 9, 2025 - 14:15

In the final part of our trilogy on Nau Engine, we'll focus on errors that occur when implementing classes. The examples in this article show how even small defects can lead to serious issues in the application operation.

As mentioned at the beginning, this is the third article on checking Nau Engine. In the first part, we focused on the engine's functionality, highlighting three key error categories: memory issues, copy-paste code, and logic errors. The second part discussed important aspects of optimization and performance improvement.

Now, let's talk about errors in class implementation that the PVS-Studio static analyzer has detected.

Fragment N1

template <std::derived_from<Attribute> K, typename T>
struct AttributeField
{
  using Key = K;
  using Value = T;

  T value;

  AttributeField(T&& inValue) :
    value(std::move(value))
  {
  }

  AttributeField(TypeTag<Key>, T&& inValue) :
    value(std::move(inValue))
  {
  }
};

The PVS-Studio warning:

V546 Member of a class is initialized with itself: 'value(std::move(value))'. attribute.h 118

This code fragment contains an error: the AttributeField::value data member is initialized by itself. As a result, the variable is used before it's actually initialized, which leads to undefined behavior. Such code can cause unexpected errors, make debugging more difficult, and leave the object in an invalid state.

To fix the initialization, developers should use the passed inValue argument, not the class member:

AttributeField(T&& inValue) :
  value(std::move(inValue))
{
}

Fragment N2

class NAU_ANIMATION_EXPORT AnimationInstance : public virtual IRefCounted
{
  ....
private:
  friend class KeyFrameAnimationPlayer;

  AnimationState m_animationState;

  nau::Ptr<Animation> m_animation;
  AnimationAssetRef m_animationAsset;
  ....
  eastl::string m_name;
};

AnimationInstance::AnimationInstance(const eastl::string& name, 
                                     AnimationAssetRef&& assetRef)
  : m_name(name)
{
  m_animationAsset = std::move(assetRef);
}

The PVS-Studio warning:

V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_animationState. animation_instance.cpp 28

This snippet contains the constructor of the AnimationInstance class, which takes two parameters: an object name and an rvalue reference to assetRef. Notice that only the values for m_name and m_animationAsset are explicitly set in the initialization list and constructor body, while the m_animationState data member remains uninitialized. This can lead to undefined behavior because uninitialized class data members may contain garbage values due to the default-initialization. The code author might have forgotten to initialize all the class data members. To ensure the object is in a valid state upon creation, it's recommended to explicitly initialize every class data member—either in the constructor initialization list or by providing default values at declaration.

Fragment N3

class NAU_KERNEL_EXPORT LzmaLoadCB : public IGenLoad
{
public:
  ....
  ~LzmaLoadCB()
  {
    close();
  }
  ....
};

void LzmaLoadCB::close()
{
  if (isStarted && !isFinished && !inBufLeft && rdBufPos >= rdBufAvail)
    ceaseReading();
  ....
}

The PVS-Studio warning:

V1053 Calling the 'ceaseReading' virtual function indirectly in the destructor may lead to unexpected result at runtime. Check lines: 'dag_lzmaIo.h:27', 'lzmaDecIo.cpp:48', 'dag_genIo.h:249'. dag_lzmaIo.h 27

In this example, the LzmaLoadCB class implements a destructor where the close member function is called first, and then the [ceaseReading](https://github.com/NauEngine/NauEnginePublic/blob/020cbfec7bd4cff7ff4303e0f31d75eebed584be/engine/core/kernel/include/nau/dag_ioSys/dag_genIo.h#L249) member function, declared as virtual in the IGenLoad base class, is called when a certain condition is met. Calling virtual class functions in the context of object construction or destruction can be dangerous: at this point, the object has either not yet initialized its most derived parts (construction) or has already destroyed them (destruction). This means that if the ceaseReading virtual function is overridden in the most derived class, a virtual function from the current or base class will always be called at the moment of construction or destruction. This may lead to ignoring important functionality implemented in the derived class. To preserve correct program behavior, it's better to avoid such calls in constructors or destructors.

Another case:

V1053 Calling the 'Type' virtual function indirectly in the constructor may lead to unexpected result at runtime. Check lines: 318, 252, 245. d3dx12_state_object.h 318

Fragment N4

FsPath& FsPath::operator=(FsPath&& other)
{
    m_path = std::move(other.m_path);
    other.m_path.clear();
    return *this;
}

The PVS-Studio warning:

V794 The assignment operator should be protected from the case of 'this == &other'. fs_path.cpp 36

The move assignment operator for the FsPath class, which transfers data from the other object to the current instance, is implemented in the code fragment. However, there's no check for self-assignment (i.e. (this == &other)), which could lead to unintended consequences.

If an attempt is made to assign the object to itself, the m_path = std::move(other.m_path); operation moves the contents of other.m_path into m_path, and then the subsequent call to other.m_path.clear(); clears the data. As a result, m_path ends up in an unexpected state, and one can only wish developers happy debugging :)

To eliminate the risk, we recommend adding the following check at the beginning of the operator:

if (this == std::addressof(other))
{
    return *this;
}

Using std::addressof instead of the operator & ensures correct address comparison even when the operator & is overloaded in the class.

Fragment N5

// cocos2d::Node
class CC_DLL Node : public Ref
{
  ....  
  virtual void reorderChild(Node* child, int localZOrder);
  ....
};


// nau::ui::Node
class NAU_UI_EXPORT Node : virtual protected cocos2d::Node
{
  ....
  virtual void reorderChild(Node* child, int zOrder);
  ....
};

The PVS-Studio warning:

V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'reorderChild' in derived class 'Node' and base class 'Node'. node.h 145

The example contains two classes named Node:

cocos2d::Node is the original class;
nau::ui::Node is the class derived from cocos2d::Node.

The issue arises because the reorderChild member function is declared in both classes with a parameter of the Node type. However, in the base Node class, this parameter is a pointer to the original cocos2d class, whereas in the derived class, it's the same class. This discrepancy may prevent the compiler from recognizing a member function in the derived class as a valid override of the base class member function. As a result, the virtual function mechanism fails, and when a function is called via a pointer to the base class, a version of the base class is executed, potentially leading to unexpected program behavior.

To avoid such errors, it's recommended to explicitly specify namespace in the function parameter types and always use the override specifier when overriding virtual functions:

// nau::ui::Node
class NAU_UI_EXPORT Node : virtual protected cocos2d::Node
{
  ....
  virtual void reorderChild(cocos2d::Node* child, int zOrder) override;
  ....
};

This ensures that the function signature in the derived class matches the base function signature, and if it doesn't, the compiler will show an error.

Here's another case:

V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'removeChild' in derived class 'Sprite' and base class 'Sprite'. sprite.h 87

Fragment N6

namespace std::chrono
{
    inline void PrintTo(milliseconds t, ostream* os)
    {
        *os << t.count() << "ms";
    }

} // namespace std::chrono

The PVS-Studio warning:

V1061 Extending the 'std' namespace may result in undefined behavior. stopwatch.h 26

A developer has added the PrintTo function to the std::chrono namespace. This function is one of the ways to teach GoogleTest to print values of custom types if the operator<< is not overloaded for them. According to the documentation, we need to define this function in the same namespace where our custom type is defined. In this case, this type is a specialization of the std::chrono::duration class template for working with milliseconds.

Extending the standard std namespace (or other reserved namespaces like posix) is a questionable practice that can lead to undefined behavior. The issue is that the C++ standard expects its namespace to remain unchanged, while library implementations may evolve over time, introducing new functions and types. If a function or class is declared within std or its nested namespaces, there's a chance that a future version of the standard may introduce an identical definition, leading to conflicts and unstable program behavior.

The modern C++ standard allows adding entities to std from a rather small list—and, unfortunately, free functions are not included. Therefore, adding the PrintTo function for the std::chrono::milliseconds type will result in undefined behavior. But what should we do if we still want to print time in milliseconds within GoogleTest? There are two ways to solve the problem.

Solution N1. C++20 has already done everything for us, the [operator<<](https://en.cppreference.com/w/cpp/chrono/duration/operator_ltlt) is defined for std::chrono::duration. All that's left is to wait for the standard library you are using to fully support the P0355 proposal :)

Solution N2. Well, since the standard library doesn't have this operator yet, we'll define it ourselves in the global namespace. And then, the compiler will find the operator we need, right?

No, it won't.

namespace internal_stream_operator_without_lexical_name_lookup {

// The presence of an operator<< here will terminate lexical scope lookup
// straight away (even though it cannot be a match because of its argument
// types). Thus, the two operator<< calls in StreamPrinter will find only ADL
// candidates.
struct LookupBlocker {};
void operator<<(LookupBlocker, LookupBlocker);

struct StreamPrinter {
  template <typename T,
            // Don't accept member pointers here. We'd print them via implicit
            // conversion to bool, which isn't useful.
            typename = typename std::enable_if<
                !std::is_member_pointer<T>::value>::type>
  // Only accept types for which we can find a streaming operator via
  // ADL (possibly involving implicit conversions).
  // (Use SFINAE via return type, because it seems GCC < 12 doesn't handle name
  // lookup properly when we do it in the template parameter list.)
  static auto PrintValue(const T& value, ::std::ostream* os)
      -> decltype((void)(*os << value)) {
    // Call streaming operator found by ADL, possibly with implicit conversions
    // of the arguments.
    *os << value;
  }
};

}  // namespace internal_stream_operator_without_lexical_name_lookup

The GoogleTest implementation restricts unqualified name lookup in enclosing scopes (this is done by using the overloaded operator<<(LookupBlocker, LookupBlocker)). As a result, ADL will only find functions in namespaces where argument types are declared.

That's why we have to use trickier code before C++20 and the implementation of P0355. We will define a new class derived from std::chrono::duration in the nau::test namespace. In that same namespace, we will also define an overload of the operator<< to print objects of type std::chrono::duration to an arbitrary output stream. Then, when it's necessary to print a duration via GoogleTest, we just need to convert std::chrono::duration into our derived class.

Possible implementation.

#include 
#include 

#define HAS_OVERLOADED_OPERATOR_LTLT_FOR_CHRONO 0
#define HAS_CTAD_FEATURE 0

#ifdef __has_include
  #if __has_include()
    // use feature test macro
    #include 

    // check for the 'operator<<' in the standard library
    #if defined(__cpp_lib_chrono) && __cpp_lib_chrono >= 201907L
      #undef HAS_OVERLOADED_OPERATOR_LTLT_FOR_CHRONO
      #define HAS_OVERLOADED_OPERATOR_LTLT_FOR_CHRONO 1
    #endif

    // check for class template argument deduction feature
    #if defined(__cpp_deduction_guides) && __cpp_deduction_guides >= 201703L
      #undef HAS_CTAD_FEATURE
      #define HAS_CTAD_FEATURE 1
    #endif
  #endif
#endif

namespace nau::test
{
#if !HAS_OVERLOADED_OPERATOR_LTLT_FOR_CHRONO
  template <typename Rep, typename Period>
  struct duration : public std::chrono::duration<Rep, Period>
  {
    using base_class = std::chrono::duration<Rep, Period>;
    using base_class::base_class;

    duration(const base_class &base) : base_class { base } {}
  };

  #if HAS_CTAD_FEATURE
  template <typename Rep, typename Period>
  duration(const std::chrono::duration<Rep, Period> &) -> duration<Rep, Period>;
  #endif

  template <typename Rep, typename Period>
  using duration_wrapper = duration<Rep, Period>;

  using milliseconds_wrapper = duration<std::chrono::milliseconds::rep,
                                        std::chrono::milliseconds::period>;

  using nanoseconds_wrapper = duration<std::chrono::nanoseconds::rep,
                                       std::chrono::nanoseconds::period>;

  template <typename Rep, typename Period>
  std::ostream& operator<<(std::ostream& out,
                           const std::chrono::duration<Rep, Period>& obj)
  {
    using namespace std::literals;

    std::string_view postfix = "s"sv;
    if constexpr (Period::type::num == 1)
    {
      // attoseconds, as
           if constexpr (Period::type::den == 1000000000000000000) 
             postfix = "as"sv;
      // femtoseconds, fs
      else if constexpr (Period::type::den == 1000000000000000) 
             postfix = "fs"sv;
      // picoseconds, ps
      else if constexpr (Period::type::den == 1000000000000) postfix = "fs"sv;
      // nanoseconds, ns
      else if constexpr (Period::type::den == 1000000000) postfix = "ns"sv;
      // microseconds, us
      else if constexpr (Period::type::den == 1000000) postfix = "us"sv;
      // milliseconds, ms
      else if constexpr (Period::type::den == 1000) postfix = "ms"sv;
      // centiseconds, cs
      else if constexpr (Period::type::den == 100) postfix = "cs"sv;
      // deciseconds, ds
      else if constexpr (Period::type::den == 10) postfix = "ds"sv;
    }
    else if constexpr (Period::type::den == 1)
    {
      // minutes, min
           if constexpr (Period::type::num == 60) postfix = "min"sv;
      // hours, h
      else if constexpr (Period::type::num == 3600) postfix = "h"sv;
      // days, d
      else if constexpr (Period::type::num == 86400) postfix = "d"sv;
      // decaseconds, das
      else if constexpr (Period::type::num == 10) postfix = "das"sv;
      // hectoseconds, hs
      else if constexpr (Period::type::num == 100) postfix = "hs"sv;
      // kiloseconds, ks
      else if constexpr (Period::type::num == 1000) postfix = "ks"sv;
      // megaseconds, Ms
      else if constexpr (Period::type::num == 1000000) postfix = "Ms"sv;
      // gigaseconds, ns
      else if constexpr (Period::type::num == 1000000000) postfix = "Gs"sv;
      // teraseconds, ps
      else if constexpr (Period::type::num == 1000000000000) postfix = "Ts"sv;
      // petaseconds, fs
      else if constexpr (Period::type::num == 1000000000000000) 
             postfix = "Ps"sv;
      // exaseconds, Es
      else if constexpr (Period::type::num == 1000000000000000000) 
             postfix = "Es"sv;
    }

    out << obj.count() << postfix;
    return out;
  }

#else
  template <typename Rep, typename Period>
  using duration_wrapper = std::chrono::duration<Rep, Period>;

  using milliseconds_wrapper = duration<std::chrono::milliseconds::rep,
                                        std::chrono::milliseconds::period>;

  using nanoseconds_wrapper = duration<std::chrono::nanoseconds::rep,
                                       std::chrono::nanoseconds::period>;
#endif
}

One last thing

/**
    Test:
        Attributes with non-serializable values and empty string keys
        should not be accessible through the attribute container.
 */

The PVS-Studio warning:

V1076 Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor. test_runtime_attributes.cpp 91

In the test comment, the analyzer detected a byte sequence E2 80 8B, which is interpreted as Zero Width Space (ZWSP). This character is an invisible space that is not displayed on the screen but remains part of the text.

Let's use a HEX editor to see for ourselves:

We would like to say right away that this code does not contain any Trojan Source. But it's worth reminding everyone that this danger still exists. If some dangerous invisible characters accidentally appear in a variable name, or a string literal, or even in a comment, your program may behave differently than expected—without showing any errors during compilation.

However, enabling the display of invisible characters in your code editor will not always fix the situation. A trojan may be somewhere among the hundreds of files that have been sent to you as a pull request. Therefore, to protect yourself from such problems, it's recommended to use automated tools. Here the choice is yours: it can be an ordinary grep or a static code analyzer.

Conclusion

The examples above clearly show how even small errors can escalate into serious issues at runtime. Flaws like incorrect member initialization, errors in overriding virtual functions, or extending standard namespaces can lead to vulnerabilities. Detecting and addressing such vulnerabilities in time is key to developing stable and reliable software.

We hope our trilogy on Nau Engine will provide developers with a useful reference, helping them avoid common mistakes and enhance project quality. Applying this knowledge in real-world situations, continuously analyzing and refining code, and sharing insights with colleagues are all essential steps toward professional growth. After all, successful development isn't just about technical skills—it's about learning from mistakes and striving for continuous improvement.

Edsger W. Dijkstra once said, "Program testing can be used to show the presence of bugs, but never to show their absence." That's why it's important not just to fix bugs but to write code in a way that minimizes them from the start.

Thank you for reading!