Анализ кода проекта 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(¶ms, 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 самостоятельно. Скачать его вы можете по ссылке.
