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

• Введение
• Обзор 10 интересных предупреждений, выданных анализатором
• Заключение

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 будет работать как надо без всяких "грязных трюков" и прочего колдовства :)

Заключение

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

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