В финальной части нашей трилогии, посвящённой Nau Engine, мы уделим внимание ошибкам, возникающим при разработке классов. Приведённые в статье примеры наглядно демонстрируют, как даже небольшие недоработки могут обернуться серьёзными проблемами в работе приложения.

Как уже сказано в начале, это третья статья про проверку Nau Engine. В первой части мы рассмотрели функционал Nau Engine с акцентом на три группы ошибок: проблемы управления памятью, избыточное копирование кода и логические недочёты. Во второй части обсуждались ключевые аспекты оптимизации и повышения производительности.

Теперь предлагаю сфокусироваться на ошибках, обнаруженных статическим анализатором PVS-Studio в процессе разработки классов.

Фрагмент 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))
  {
  }
};

Предупреждение PVS-Studio: V546 Member of a class is initialized with itself: 'value(std::move(value))'. attribute.h 118

В этом коде допущена ошибка, связанная с тем, что в первом конструкторе поле AttributeField::value инициализируется самим собой. В результате переменная используется до своей фактической инициализации, что приводит к неопределённому поведению. Такой код может привести к неожиданным ошибкам, затруднить отладку и оставить объект в некорректном состоянии.

Исправленный вариант инициализации должен использовать переданный аргумент inValue, а не сам член класса:

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

Фрагмент 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);
}

Предупреждение PVS-Studio: V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_animationState. animation_instance.cpp 28

В этом фрагменте рассматривается конструктор класса AnimationInstance, который принимает два параметра: имя объекта и rvalue ссылку на assetRef. Обратите внимание, что в списке инициализации и теле конструктора явно задаются лишь значения для m_name и m_animationAsset, в то время как поле m_animationState остаётся без инициализации. Такая ситуация может привести к неопределённому поведению, поскольку неинициализированные члены класса потенциально содержат мусорные значения из-за default-инициализации. Вероятно, автор кода забыл добавить инициализацию для всех членов класса. Рекомендуется обеспечить явную инициализацию каждого поля: либо через список инициализации конструктора, либо посредством значений по умолчанию при объявлении, чтобы гарантировать корректное состояние объекта при его создании.

Фрагмент N3

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

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

Предупреждение PVS-Studio: 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

В этом примере класс LzmaLoadCB реализует деструктор, в котором сначала вызывается функция close, а затем при выполнении определённого условия происходит вызов функции ceaseReading, объявленной как виртуальная в базовом классе IGenLoad. Вызов виртуальных функций в контексте конструирования или разрушения объекта может быть опасным: на этом моменте у объекта либо ещё не инициализированы производные части (конструирование), либо уже уничтожены его наиболее производные части (разрушение). Это означает, что, если у виртуальной функции ceaseReading есть переопределение в наиболее производном классе, на момент работы конструктора или деструктора всегда будет вызвана виртуальная функция из текущего или базового класса. Это может привести к игнорированию важного функционала, реализованного в наиболее производном классе. Лучше избегать подобных вызовов в конструкторах и деструкторах, чтобы не нарушить корректность работы программы.

И вот ещё случаи:

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

Фрагмент N4

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

Предупреждение PVS-Studio: V794 The assignment operator should be protected from the case of 'this == &other'. fs_path.cpp 36

В представленном фрагменте реализован оператор перемещения присваивания класса FsPath, в котором происходит перенос данных из объекта other в текущий экземпляр. Следует отметить, что отсутствует проверка на самоприсваивание (проверка вида (this == &other)), что может привести к нежелательным последствиям.

Если объект пытаются присвоить самому себе, то операция m_path = std::move(other.m_path); перемещает содержимое other.m_path в m_path, а последующий вызов other.m_path.clear(); очищает данные. В результате m_path оказывается в неожиданном состоянии, и остаётся лишь пожелать удачной отладки :)

Чтобы устранить этот риск, рекомендую добавить в начало оператора следующую проверку:

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

Использование std::addressof вместо оператора & гарантирует корректное сравнение адресов даже при перегрузке оператора & в классе.

Фрагмент 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);
  ....
};

Предупреждение PVS-Studio: 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

В примере определены два класса с именем Node:

cocos2d::Node — оригинальный класс;
nau::ui::Node — класс, который наследуется от cocos2d::Node.

Проблема кроется в том, что функция reorderChild объявлена в обоих классах с первым параметром типа Node. Однако по правилам поиска неквалифицированных имён, при объявлении функции в базовом классе имя Node будет разрешёно как cocos2d::Node, а в производном классе — как nau::ui::Node. Из-за этого различия компилятор не сделает переопределение виртуальной функции из базового класса. В результате механизм виртуальных функций сработает некорректно, и при вызове метода через указатель на базовый класс выполняется версия из базового класса, что может привести к непредвиденному поведению программы.

Чтобы избежать подобных ошибок, рекомендуется при переопределении виртуальных функций явно указать тип первого параметра, а также всегда не забывать о применении спецификатора override:

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

Это обеспечивает полное совпадение сигнатуры метода в производном классе с сигнатурой базового метода, и в случае несовпадения компилятор выдаст ошибку.

И вот ещё случай:

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

Фрагмент N6

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

} // namespace std::chrono

Предупреждение PVS-Studio: V1061 Extending the 'std' namespace may result in undefined behavior. stopwatch.h 26

В пространство имён std::chrono добавили функцию PrintTo. Эта функция – один из способов научить GoogleTest печатать значения пользовательских типов, если для них не перегружен оператор <<. Согласно документации, нужно определить эту функцию в том же пространстве имён, где определён наш пользовательский тип. В данной ситуации, этим типом является специализация шаблона класса std::chrono::duration для работы с миллисекундами.

Расширение стандартного пространства имён std (а также некоторых других пространств, например, posix) — опасная практика, которая может привести к неопределённому поведению. Проблема в том, что стандарт C++ предполагает неизменность своего пространства имён, а реализация библиотек может со временем расширяться, добавляя новые функции и типы. Если в std или его вложенных пространствах объявить пользовательскую функцию или класс, есть вероятность, что в одной из будущих версий стандарта появится аналогичное определение, что приведёт к конфликтам и нестабильной работе программы.

Современный стандарт C++ разрешает добавлять сущности в std из достаточно скромного списка. И, к сожалению, обычные функции в этот список не входят. Поэтому поведение будет не определено при добавлении функции PrintTo для типа std::chrono::milliseconds. Но что же делать, если мы всё же хотим печатать время в миллисекундах в рамках GoogleTest? Есть два способа решения проблемы.

Решение N1. В C++20 всё уже сделали за нас, оператор << уже определён для std::chrono::duration. Осталось лишь дождаться, когда в используемой вами стандартной библиотеке полноценно поддержат проползал P0355 :)

Решение N2. Ну раз в стандартной библиотеке пока нет этого оператора, значит мы определим его сами в глобальном пространстве имён. А дальше же компилятор найдёт нужный нам оператор, ведь так?

Нет, не найдёт.

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

Реализация GoogleTest ограничивает неквалифицированный поиск в обрамляющих пространствах имён (при помощи перегруженного оператора operator<<(LookupBlocker, LookupBlocker). А ADL ищет функции только в тех пространствах имён, где объявлены типы аргументов.

Поэтому до C++20 и реализации проползала P0355 придётся использовать код похитрее. Мы унаследуемся от std::chrono::duration и разместим наследника в пространстве имён nau::test. Там же мы определим перегрузку оператора <<, который будет уметь печатать объекты типа std::chrono::duration в произвольный поток вывода. Далее, когда будет необходимо через GoogleTest напечатать длительность, нужно просто сконвертировать std::chrono::duration в нашего наследника.

Возможная имплементация.

#include <iostream>
#include <chrono>

#define HAS_OVERLOADED_OPERATOR_LTLT_FOR_CHRONO 0
#define HAS_CTAD_FEATURE 0

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

    // 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
}

На посошок

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

Предупреждение PVS-Studio: 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

Анализатор обнаружил, что в комментарии к тесту закралась последовательность байтов E2 80 8B, которая интерпретируется как Zero Width Space (ZWSP). Этот символ представляет собой невидимый пробел, который не отображается на экране, но при этом остаётся частью текста.

Воспользуемся HEX-редактором, что убедиться в этом:

Сразу хочу сказать, что в этом коде никакого Trojan Source не заложено. Однако я хотел бы напомнить читателям, что такая опасность по-прежнему существует. Если опасные невидимые символы случайно окажутся или в имени переменной, или строковом литерале, или даже в комментарии, то поведение вашей программы может отличаться от того, что вы видите в коде. При этом компиляторы прекрасно пропускают такой код без ошибок.

Хочу также отметить, что не всегда включение отображения невидимых символов в вашем редакторе кода полностью исправит положение. Троян может затесаться где-то среди сотен файлов, которые пришли вам в качестве Pull Request. Поэтому, чтобы защититься от подобных проблем, рекомендуется также применять автоматизированные инструменты. Здесь выбор остаётся за вами: это может быть и обычный grep, а может и статический анализатор кода.

Заключение

Приведённые примеры наглядно демонстрируют, как даже незначительные ошибки могут перерасти в серьёзные проблемы на этапе выполнения программы. Независимо от того, связаны ли недочёты с неправильной инициализацией членов, ошибками переопределения виртуальных функций или с расширением стандартных пространств имён, своевременное обнаружение и устранение подобных уязвимостей является залогом создания стабильного и надёжного программного обеспечения.

Надеюсь, что наша трилогия о Nau Engine станет полезным ориентиром для разработчиков, поможет избегать типичных ошибок и повышать качество проектов. Стоит применять полученные знания на практике, постоянно анализировать и совершенствовать свой код, а также делиться опытом с коллегами. Ведь успешная разработка это не только владение техническими навыками, но и способность учиться на ошибках и стремиться к постоянному развитию.

Как говорил Эдсгер Дейкстра: 'Тестирование показывает наличие ошибок, а не их отсутствие'. Поэтому важно не только исправлять баги, но и изначально писать код так, чтобы их было как можно меньше.

Благодарю за внимание!

Третья часть исследования Nau Engine

zproxy.org