Анализ кода проекта DeepSpeech или почему не стоит писать в namespace std

DeepSpeech – это открытый и свободно распространяемый движок распознавания речи, разрабатываемый компанией Mozilla. Движок имеет довольно высокую производительность и хорошие отзывы пользователей, и это делает код проекта интересной мишенью для проверки. Данная статья посвящена разбору ошибок, найденных в C++-коде проекта DeepSpeech.

Введение

Мы уже неоднократно искали ошибки в проектах, использующих машинное обучение, и DeepSpeech не стал для нас исключением. Не удивительно, ведь этот проект является достаточно популярным: на момент написания статьи он имеет уже более 15k звёзд на GitHub.

Как обычно, поиск ошибок, которые я приведу в этой статье, проводился с помощью статического анализатора кода PVS-Studio.

Для своей работы DeepSpeech использует библиотеку TensorFlow. Я отключил анализ кода этой библиотеки, потому что про неё мы уже писали отдельную статью, однако я не стал отключать анализ остальных используемых библиотек. С чем это связано? Ошибки внутри любой библиотеки, которую вы включаете в ваш проект, также становятся и ошибками внутри вашего проекта. Поэтому полезно проводить анализ не только вашего, но и любого стороннего кода, который вы используете. Подробное мнение об этом вы можете прочитать в нашей недавней статье.

На этом краткое вступление заканчивается – пора переходить к разбору ошибок. Кстати, если вы зашли сюда, чтобы узнать ответ на вопрос, заданный мной в заголовке статьи (почему не стоит писать в namespace std) – можете сразу заглянуть в конец статьи. Там вас ждёт по-особенному интересный пример!

Обзор 10 интересных предупреждений, выданных анализатором

Предупреждение 1

V773 The function was exited without releasing the 'data' pointer. A memory leak is possible. edit-fst.h 311

// EditFstData method implementations: just the Read method. template <typename A, typename WrappedFstT, typename MutableFstT> EditFstData<A, WrappedFstT, MutableFstT> * EditFstData<A, WrappedFstT, MutableFstT>::Read(std::istream &strm,                                                const FstReadOptions &opts) {   auto *data = new EditFstData<A, WrappedFstT, MutableFstT>();   // next read in MutabelFstT machine that stores edits   FstReadOptions edits_opts(opts);    ....      std::unique_ptr<MutableFstT> edits(MutableFstT::Read(strm, edits_opts));   if (!edits) return nullptr; // <=    .... }

Данный фрагмент кода содержит классический пример утечки памяти: функция Read вызывает 'return nullptr', не освободив при этом память, выделенную с помощью выражения 'new EditFstData'. При подобном выходе из функции (без вызова delete data) будет удалён только сам указатель, а деструктор объекта, на который он указывает, вызван не будет. Таким образом, объект продолжит храниться в памяти, а удалить или использовать его уже будет нельзя.

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

Предупреждение 2

V1062 The 'DfsState' class defines a custom 'new' operator. The 'delete' operator must also be defined. dfs-visit.h 62

// An FST state's DFS stack state. template <class FST> struct DfsState { public:   ....   void *operator new(size_t size,                       MemoryPool<DfsState<FST>> *pool) {     return pool->Allocate();   }   .... }

PVS-Studio не стоит на месте и продолжает пополняться новыми диагностиками. Данный фрагмент кода – отличный пример для того, чтобы показать на нём работу свежей диагностики под номером V1062.

Правило, за выполнением которого следит эта диагностика, просто: если вы определяете собственный оператор 'new', то вы также должны определить и собственный оператор 'delete'. Аналогично работает и наоборот: если вы определяете собственный 'delete', то и собственный 'new' тоже надо определить.

В примере выше данное правило нарушено: создаваться объект будет с помощью определенного нами 'new', а удаляться – стандартным 'delete'. Давайте посмотрим, что же делает функция Allocate класса MemoryPool, которую вызывает наш собственный 'new':

void *Allocate() {   if (free_list_ == nullptr) {     auto *link = static_cast<Link *>(mem_arena_.Allocate(1));     link->next = nullptr;     return link;   } else {     auto *link = free_list_;     free_list_ = link->next;     return link;   } }

Эта функция создаёт элемент и добавляет его в связный список. Логично, что такую аллокацию следовало прописать в собственном 'new'.

Но постойте-ка! Прямо несколькими строками ниже содержится вот такая функция:

void Free(void *ptr) {   if (ptr) {     auto *link = static_cast<Link *>(ptr);     link->next = free_list_;     free_list_ = link;   } }

Значит, у нас уже есть готовые функции как для аллокации, так и для освобождения. Скорее всего программист должен был написать собственный оператор 'delete', использующий для освобождения именно функцию Free().

Анализатор обнаружил еще как минимум три таких ошибки:

• V1062 The 'VectorState' class defines a custom 'new' operator. The 'delete' operator must also be defined. vector-fst.h 31
• V1062 The 'CacheState' class defines a custom 'new' operator. The 'delete' operator must also be defined. cache.h 65

Предупреждение 3

V703 It is odd that the 'first_path' field in derived class 'ShortestPathOptions' overwrites field in base class 'ShortestDistanceOptions'. Check lines: shortest-path.h:35, shortest-distance.h:34. shortest-path.h 35

// Base class template <class Arc, class Queue, class ArcFilter> struct ShortestDistanceOptions {   Queue *state_queue;    // Queue discipline used; owned by caller.   ArcFilter arc_filter;  // Arc filter (e.g., limit to only epsilon graph).   StateId source;        // If kNoStateId, use the FST's initial state.   float delta;           // Determines the degree of convergence required   bool first_path;       // For a semiring with the path property (o.w.                          // undefined), compute the shortest-distances along                          // along the first path to a final state found                          // by the algorithm. That path is the shortest-path                          // only if the FST has a unique final state (or all                          // the final states have the same final weight), the                          // queue discipline is shortest-first and all the                          // weights in the FST are between One() and Zero()                          // according to NaturalLess.    ShortestDistanceOptions(Queue *state_queue, ArcFilter arc_filter,                           StateId source = kNoStateId,                           float delta = kShortestDelta)       : state_queue(state_queue),         arc_filter(arc_filter),         source(source),         delta(delta),         first_path(false) {} }; // Derived class template <class Arc, class Queue, class ArcFilter> struct ShortestPathOptions     : public ShortestDistanceOptions<Arc, Queue, ArcFilter> {   using StateId = typename Arc::StateId;   using Weight = typename Arc::Weight;    int32 nshortest;    // Returns n-shortest paths.   bool unique;        // Only returns paths with distinct input strings.   bool has_distance;  // Distance vector already contains the                       // shortest distance from the initial state.   bool first_path;    // Single shortest path stops after finding the first                       // path to a final state; that path is the shortest path                       // only when:                       // (1) using the ShortestFirstQueue with all the weights                       // in the FST being between One() and Zero() according to                       // NaturalLess or when                       // (2) using the NaturalAStarQueue with an admissible                       // and consistent estimate.   Weight weight_threshold;  // Pruning weight threshold.   StateId state_threshold;  // Pruning state threshold.    ShortestPathOptions(Queue *queue, ArcFilter filter, int32 nshortest = 1,                       bool unique = false, bool has_distance = false,                       float delta = kShortestDelta, bool first_path = false,                       Weight weight_threshold = Weight::Zero(),                       StateId state_threshold = kNoStateId)       : ShortestDistanceOptions<Arc, Queue, ArcFilter>(queue, filter,                                                        kNoStateId, delta),         nshortest(nshortest),         unique(unique),         has_distance(has_distance),         first_path(first_path),         weight_threshold(std::move(weight_threshold)),         state_threshold(state_threshold) {} };

Согласитесь, не так-то просто найти потенциальную ошибку, верно?

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

Чтобы лучше понять, что я имею ввиду, предлагаю рассмотреть краткий синтетический пример из нашей документации. Допустим, мы имеем следующий код:

class U { public:   int x; };  class V : public U { public:   int x;  // <= V703 here   int z; };

Здесь имя x перекрыто внутри производного класса. Теперь вопрос: какое значение выведет следующий код?

int main() {   V vClass;   vClass.x = 1;   U *uClassPtr = &vClass;   std::cout << uClassPtr->x << std::endl;   .... }

Если вы считаете, что будет выведено неопределённое значение, то вы правы. В данном примере запись единицы будет произведена в поле производного класса, а вот чтение будет производиться из поля базового класса, которое на момент вывода всё еще осталось неопределённым.

Перекрытие имен в иерархии классов – это потенциальная ошибка, которую лучше не допускать 🙂

Предупреждение 4

V1004 The 'aiter' pointer was used unsafely after it was verified against nullptr. Check lines: 107, 119. visit.h 119

template <....> void Visit(....) {   ....   // Deletes arc iterator if done.   auto *aiter = arc_iterator[state];   if ((aiter && aiter->Done()) || !visit) {     Destroy(aiter, &aiter_pool);     arc_iterator[state] = nullptr;     state_status[state] |= kArcIterDone;   }   // Dequeues state and marks black if done.   if (state_status[state] & kArcIterDone) {     queue->Dequeue();     visitor->FinishState(state);     state_status[state] = kBlackState;     continue;   }   const auto &arc = aiter->Value();       // <=   .... }

Указатель aiter использован после того, как он был проверен на nullptr. Анализатор делает предположение: если указатель проверяется на nullptr, значит во время проверки он может иметь такое значение.

В таком случае давайте проследим, что случится с aiter, если он действительно будет равен нулю. Сначала этот указатель будет проверен в выражении 'if ((aiter && aiter->Done()) || !visit)'. Это условие будет равняться false, и в then-ветвь этого if мы не попадём. А дальше, по всем канонам классических ошибок, произойдет разыменование нулевого указателя: 'aiter->Value();'. Результатом такого разыменования является неопределённое поведение.

Предупреждение 5

Следующий пример содержит сразу две ошибки:

• V595 The 'istrm' pointer was utilized before it was verified against nullptr. Check lines: 60, 61. mapped-file.cc 60
• V595 The 'istrm' pointer was utilized before it was verified against nullptr. Check lines: 39, 61. mapped-file.cc 39

MappedFile *MappedFile::Map(std::istream *istrm, bool memorymap,                             const string &source, size_t size) {   const auto spos = istrm->tellg();        // <=   ....   istrm->seekg(pos + size, std::ios::beg); // <=   if (istrm) {                             // <=     VLOG(1) << "mmap'ed region of " << size             << " at offset " << pos             << " from " << source             << " to addr " << map;   return mmf.release();   }   .... }

Ошибка, найденная здесь, более понятна, чем ошибка из предыдущего примера. Указатель istrm сначала разыменовывается (два раза), а только после этого следует проверка на ноль и логирование ошибки. Это явно указывает: если в эту функцию в качестве istrm придет нулевой указатель, то неопределённое поведение (или, что более вероятно, падение программы) произойдёт без всякого логирования. Непорядок… такие баги не стоит пропускать.

Предупреждение 6

V730 Not all members of a class are initialized inside the constructor. Consider inspecting: stones_written_. ersatz_progress.cc 14

ErsatzProgress::ErsatzProgress()   : current_(0)   , next_(std::numeric_limits<uint64_t>::max())   , complete_(next_)   , out_(NULL) {}

Анализатор предупреждает нас, что конструктор инициализирует не все поля структуры ErzatzProgress. Давайте сравним этот конструктор со списком полей этой структуры:

class ErsatzProgress {   .... private:     void Milestone();      uint64_t current_, next_, complete_;     unsigned char stones_written_;     std::ostream *out_; };

Действительно, можно заметить, что конструктор инициализирует все поля, кроме stones_written_.

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

Тем не менее, диагностика V730 помогает заблаговременно отлаживать случаи такого использования. Ведь возникает закономерный вопрос: если программист решил специально проинициализировать все поля класса, то почему у него должна быть причина оставить одно поле без значения?

Мои догадки насчет того, что поле stones_written_ не было инициализировано по ошибке, подтвердились, когда несколькими строками ниже я увидел еще один конструктор:

ErsatzProgress::ErsatzProgress(uint64_t complete,                                std::ostream *to,                                const std::string &message)   : current_(0)   , next_(complete / kWidth)   , complete_(complete)   , stones_written_(0)   , out_(to) {   .... }

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

Предупреждение 7

V780 The object '& params' of a non-passive (non-PDS) type cannot be initialized using the memset function. binary_format.cc 261

/* Not the best numbering system,    but it grew this way for historical reasons  * and I want to preserve existing binary files. */ typedef enum {   PROBING=0,   REST_PROBING=1,   TRIE=2,   QUANT_TRIE=3,   ARRAY_TRIE=4,   QUANT_ARRAY_TRIE=5 } ModelType;  ....  struct FixedWidthParameters {   unsigned char order;   float probing_multiplier;   // What type of model is this?   ModelType model_type;   // Does the end of the file    // have the actual strings in the vocabulary?   bool has_vocabulary;   unsigned int search_version; };  ....  // Parameters stored in the header of a binary file. struct Parameters {   FixedWidthParameters fixed;   std::vector<uint64_t> counts; };  ....  void BinaryFormat::FinishFile(....) {   ....   // header and vocab share the same mmap.   Parameters params = Parameters();   memset(&params, 0, sizeof(Parameters)); // <=   .... }

Чтобы разобраться в этом предупреждении, предлагаю сначала разобраться, что такое PDS-тип. Аббревиатура "PDS" означает "Passive Data Structure" – простая структура данных. Иногда вместо "PDS" говорят "POD" – "Plain Old Data". Говоря простым языком (цитирую с русской Википедии), PDS-тип – это тип данных, имеющий жёстко определённое расположение полей в памяти, не требующий ограничения доступа и автоматического управления. Говоря еще проще, это тип данных, состоящий только из встроенных типов.

Отличительной особенностью POD-типов является то, что переменные этих типов можно изменять и обрабатывать с помощью примитивных функций управления памятью (memset, memcpy и так далее). Однако про "не-PDS" типы такое сказать нельзя: столь низкоуровневое обращение с их значениями может приводить к серьезным ошибкам. Например, к утечкам памяти, двойному очищению одного и того же ресурса или к неопределённому поведению.

На код, приведенный выше, PVS-Studio выдаёт предупреждение: так обращаться со структурой типа Parameters нельзя. Если заглянуть в определение этой структуры, можно увидеть, что второй её член имеет тип std::vector. Этот тип активно использует автоматическое управление памятью и помимо содержимых данных хранит в себе дополнительные, служебные переменные. Зануление такого поля с помощью memset может нарушить логику работы класса и является серьезной ошибкой.

Предупреждение 8

V575 The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 73, 68. modelstate.cc 73

Metadata* ModelState::decode_metadata(const DecoderState& state,                              size_t num_results) {   ....   Metadata* ret = (Metadata*)malloc(sizeof(Metadata));   ....   memcpy(ret, &metadata, sizeof(Metadata));   return ret; }

Следующее предупреждение говорит нам о том, что в функцию memcpy передаётся нулевой указатель. Да, действительно, если функция malloc не сможет произвести аллокацию памяти, то она вернёт NULL. В этом случае этот указатель будет передан в функцию memset, где произойдёт его разыменование – и, соответственно, фееричное падение программы.

Однако, некоторые наши читатели могут возмутиться: если память переполнилась/фрагментировалась настолько, что malloc не смог выделить память, то не всё ли равно, что произойдет дальше? Программа так и так упадёт, ведь из-за отсутствия памяти она не сможет функционировать нормально.

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

Предупреждение 9

Следующее предупреждение вызвано теми же причинами, что и предыдущее. Правда, указывает оно немного на другую ошибку.

V769 The 'middle_begin_' pointer in the 'middle_begin_ + (counts.size() — 2)' expression could be nullptr. In such case, resulting value will be senseless and it should not be used. Check lines: 553, 552. search_trie.cc 553

template <class Quant, class Bhiksha> class TrieSearch { .... private:   ....   Middle *middle_begin_, *middle_end_;   .... };  template <class Quant, class Bhiksha> uint8_t *TrieSearch<Quant, Bhiksha>::SetupMemory(....) {   ....   middle_begin_     = static_cast<Middle*>(malloc(sizeof(Middle) * (counts.size() - 2)));   middle_end_ = middle_begin_ + (counts.size() - 2);   .... }

Так же, как и в предыдущем примере, здесь происходит аллокация памяти с помощью функции malloc. Возвращенный при этом указатель без всякой проверки на nullptr используется в арифметическом выражении. Увы, результат такого выражения не будет иметь никакого смысла, и в поле middle_end_ будет храниться совершенно бесполезное значение.

Предупреждение 10

Ну и, наконец, самый интересный на мой взгляд пример был найден в библиотеке kenlm, включенной в DeepSpeech:

V1061 Extending the 'std' namespace may result in undefined behavior. sized_iterator.hh 210

// Dirty hack because g++ 4.6 at least wants // to do a bunch of copy operations. namespace std { inline void iter_swap(util::SizedIterator first,                       util::SizedIterator second) {   util::swap(*first, *second); } } // namespace std

Приём, названный в комментарии "грязный трюк" действительно является грязным. Дело в том, что подобное расширение пространства имен std может привести к неопределённому поведению.

Почему? Потому что содержимое пространства имён std определяется исключительно комитетом стандартизации. Именно поэтому международный стандарт языка C++ явно запрещает расширять std подобным образом.

Последним стандартом, поддерживаемым в g++ 4.6, является C++03. Приведу переведённую цитату из C++03 final working draft (см. пункт 17.6.4.2.1): "Поведение C++-программы не определено, если она добавляет объявления или определения в пространство имён std или пространство имён, вложенное в std, за исключением случаев, для которых указано обратное". Эта цитата актуальна для всех последующих стандартов (C++11, C++14, C++17, и C++20).

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

Т.к. std уже имеет функцию под названием iter_swap (отмечу: функцию-шаблон), логично предположить, что программист желал расширить её возможности так, чтобы она смогла работать с типом util::SizedIterator. Только вот незадача: вместо того, чтобы добавить специализацию шаблона функции, программист просто написал обыкновенную перегрузку. Следовало написать так:

namespace std { template <> inline void iter_swap(util::SizedIterator first,                       util::SizedIterator second) {   util::swap(*first, *second); } } // namespace std

Однако, с этим кодом тоже всё не так просто. Дело в том, что данный код будет корректен только до стандарта C++20. Да-да, специализации шаблонов функций в нём тоже отметили как приводящие к неопределённому поведению (см. C++20 final working draft, п. 16.5.4.2.1). А поскольку данный код принадлежит библиотеке, вполне вероятно, что рано или поздно её соберут с флагом -std=C++20. Кстати, PVS-Studio различает, какая версия стандарта используется в коде, и в зависимости от этого выдает или не выдаёт предупреждение. Убедитесь сами: пример для C++17, пример для C++20.

На самом деле можно поступить гораздо проще. Для того, чтобы исправить ошибку, достаточно лишь перенести собственное определение iter_swap в тот же namespace, в котором определён класс SizedIterator. При этом в местах, где вызывается iter_swap, нужно дописать "using std::iter_swap;". Получится так (определение класса SizedIterator и функции util::swap() изменены для простоты):

namespace util {   class SizedIterator   {   public:     SizedIterator(int i) : m_data(i) {}      int& operator*()     {       return m_data;     }   private:     int m_data;   };    ....    inline void iter_swap(SizedIterator first,                         SizedIterator second)   {     std::cout << "we are inside util::iter_swap" << std::endl;     swap(*first, *second);   } }   int main() {   double d1 = 1.1, d2 = 2.2;   double *pd1 = &d1, *pd2 = &d2;   util::SizedIterator si1(42), si2(43);    using std::iter_swap;    iter_swap(pd1, pd2);   iter_swap(si1, si2); // "we are inside util::iter_swap"    return 0; }

Теперь компилятор самостоятельно выберет нужную перегрузку функции iter_swap на основе поиска с учётом аргументов (ADL). Для класса SizedIterator будет вызываться именно версия из namespace util, а для остальных типов будет вызываться версия из namespace std. Доказательства по ссылке. Более того, внутри библиотечных функций не нужно дописывать никаких "using": поскольку их код и так находится внутри std, компилятор всё равно будет выбирать правильную перегрузку.

И тогда – вуаля – пользовательская функция iter_swap будет работать как надо без всяких "грязных трюков" и прочего колдовства 🙂

Заключение

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

P.S. Мы считаем, что писать собственный код в namespace std – это плохая практика. А как думаете вы? Жду ваших ответов в комментариях.

Если вы разрабатываете на C, C++, C# или Java и вам, как и мне, интересна тема статического анализа, то предлагаю попробовать PVS-Studio самостоятельно. Скачать его вы можете по ссылке.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: George Gribkov. Checking the Code of DeepSpeech, or Why You Shouldn’t Write in namespace std.