DeepSpeechプロジェクトのコードの分析、または名前名stdに記述すべきでない理由

DeepSpeechは、Mozillaによって開発されたオープンソースの無料音声認識エンジンです。エンジンはかなり高いパフォーマンスと優れたユーザーレビューを備えているため、プロジェクトコードをチェックするのに興味深いターゲットになります。この記事は、DeepSpeechプロジェクトのC ++コードで見つかったエラーの分析に専念しています。

前書き

私たちは機械学習を使用してプロジェクトのエラーを繰り返し探してきましたが、DeepSpeechも例外ではありませんでした。このプロジェクトは非常に人気があるため、当然のことです。この記事の執筆時点では、GitHubにはすでに15,000を超える星があります。



いつものように、この記事で引用するエラーの検索は、PVS-Studio静的コードアナライザーを使用して実行されました。



その作業のために、DeepSpeechはTensorFlowライブラリを使用します。このライブラリに関する別の記事をすでに書いているので、このライブラリのコードの分析をオフにしました。ただし、使用した残りのライブラリの分析はオフにしませんでした。これの理由は何ですか？プロジェクトに含めるライブラリ内のエラーも、プロジェクト内のエラーになります。したがって、自分だけでなく、使用しているサードパーティのコードも分析すると便利です。これに関する詳細な意見は、最近の記事で読むことができます。



これで簡単な紹介は終わりです。エラーの分析に移りましょう。ちなみに、記事のタイトルで私が尋ねた質問への答えを見つけるためにここに来た場合（なぜnamespace stdで書くべきではないのか）、すぐに記事の終わりを見ることができます。特に興味深い例があなたを待っています！

アナライザーによって発行された10の興味深い警告の概要

警告



1V773「データ」ポインタを解放せずに関数が終了しました。メモリリークが発生する可能性があります。編集-fst.h311

// 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; // <=

  ....
}

このコードスニペットは、メモリリークの古典的な例が含まれています：読み取り関数呼び出し「リターンnullptr式」に割り当てられたメモリの解放せずに「新しいEditFstDataを」。 （delete dataを呼び出さずに）関数をこのように終了すると、ポインター自体のみが削除され、ポインターが指すオブジェクトのデストラクタは呼び出されません。したがって、オブジェクトは引き続きメモリに保存され、削除または使用することはできなくなります。



エラーに加えて、このコードには、あまり良くない別のプラクティスも含まれています。1つの関数のコードは、スマートポインターと通常のポインターの両方を同時に使用します。たとえば、データの場合がスマートポインタでもあった場合、このようなエラーは発生しません。必要に応じて、スコープ外に出ると、スマートポインタは保存されたオブジェクトのデストラクタを自動的に呼び出します。



警告



2V1062「DfsState」クラスはカスタムの「new」演算子を定義します。 'delete'演算子も定義する必要があります。 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」演算子も定義する必要があります。逆も同じように機能します。独自の「削除」を定義する場合は、独自の「新しい」も定義する必要があります。



上記の例では、このルールに違反しています。オブジェクトは、ユーザーが定義した「new」を使用して作成され、標準の「delete」を使用して削除されます。MemoryPoolクラスのAllocate関数が何をするか見てみましょう。私たち自身の「新しい」が呼ぶもの：

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

この関数はアイテムを作成し、リンクされたリストに追加します。そのような割り当てがそれ自身の「新しい」で書かれるべきであったことは論理的です。



しかし、ちょっと待ってください！以下の数行には、次の関数が含まれています。

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

これは、割り当てとリリースの両方の既製の機能がすでにあることを意味します。ほとんどの場合、プログラマーは、Free（）関数を使用してそれを解放し、独自の「削除」演算子を作成する必要がありました。



アナライザーは、少なくとも3つのそのようなエラーを検出しました。

• V1062「VectorState」クラスはカスタムの「new」演算子を定義します。'delete'演算子も定義する必要があります。vector-fst.h 31
• V1062'CacheState 'クラスは、カスタムの' new '演算子を定義します。'delete'演算子も定義する必要があります。cache.h 65

警告



3V703派生クラス「ShortestPathOptions」の「first_path」フィールドが基本クラス「ShortestDistanceOptions」のフィールドを上書きするのは奇妙なことです。チェックライン：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 nullptrに対して検証された後、「aiter」ポインターが安全に使用されませんでした。チェックライン：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になり、の場合、このブランチには入りません。そして、古典的なエラーのすべての基準に従って、nullポインタが逆参照されます。'aiter-> Value（）;'。この逆参照により、未定義の動作が発生します。



警告5



次の例には、一度に2つのエラーが含まれています。

• V595 nullptrに対して検証される前に、「istrm」ポインターが使用されました。チェック行：60、61。mapped-file.cc 60
• V595 nullptrに対して検証される前に、「istrm」ポインターが使用されました。チェック行：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ポインターは最初に逆参照され（2回）、その後でゼロチェックとエラーログが続きます。これは明確に示しています。nullポインタがistrmとしてこの関数に到達すると、ログなしで未定義の動作（または、おそらくプログラムのクラッシュ）が発生します。障害...このようなバグを見逃してはなりません。





警告



6V730クラスのすべてのメンバーがコンストラクター内で初期化されるわけではありません。検査を検討してください：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診断は、これらのユースケースを事前にデバッグするのに役立ちます。結局のところ、自然な疑問が生じます。プログラマーがクラスのすべてのフィールドを具体的に初期化することにした場合、なぜ1つのフィールドを値なしのままにする理由があるのでしょうか。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)
{
  ....
}

ここで、クラスのすべてのフィールドが初期化されます。これにより、プログラマーは実際にすべてのフィールドを初期化することを計画しましたが、誤って1つのことを忘れてしまいました。



警告



7V780非パッシブ（非PDS）タイプのオブジェクト '＆params'は、memset関数を使用して初期化できません。 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 DataStructureの略です。 「PDS」の代わりに「POD」-「プレーンオールドデータ」と言うこともあります。簡単に言うと（ロシアのウィキペディアから引用）、PDSタイプは、メモリ内のフィールドの場所が厳密に定義されているデータタイプであり、アクセス制限や自動制御は必要ありません。簡単に言えば、組み込み型のみで構成されるデータ型です。



PODタイプの特徴は、これらのタイプの変数を、プリミティブメモリ管理関数（memset、memcpyなど）を使用して変更および処理できることです。ただし、これは「非PDS」タイプについては言えません。これらの値のこのような低レベルの処理は、重大なエラーにつながる可能性があります。たとえば、メモリリーク、同じリソースのダブルフラッシュ、または未定義の動作。



PVS-Studioは、上記のコードに対して警告を発行します。この方法でParametersタイプの構造を処理することはできません。この構造の定義を見ると、2番目のメンバーのタイプがstd :: vectorであることがわかります。..。このタイプは、自動メモリ管理を積極的に使用し、コンテンツデータに加えて、追加のサービス変数を格納します。memsetを使用してこのようなフィールドをゼロにすると、クラスのロジックが破損する可能性があり、重大な間違いです。



警告



8V575潜在的なnullポインタが「memcpy」関数に渡されます。最初の引数を調べます。チェックライン：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'middle_begin_ +（counts.size（）-2）'式の 'middle_begin_'ポインターはnullptrである可能性があります。このような場合、結果の値は無意味になるため、使用しないでください。チェックライン：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



そして最後に、私の意見で最も興味深い例は、DeepSpeechに含まれているkenlmライブラリにありました。V1061 「std」名前



名を拡張すると、未定義の動作が発生する可能性があります。 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最終作業ドラフトからの翻訳された引用です。（項目17.6.4.2.1を参照）：「特に指定がない限り、C ++プログラムがstd名前空間またはstdネストされた名前空間に宣言または定義を追加する場合、C ++プログラムの動作は未定義です。」この見積もりは、後続のすべての標準（C ++ 11、C ++ 14、C ++ 17、およびC ++ 20）に適用されます。



この例から問題のあるコードを修正する方法を検討することを提案します。最初の論理的な質問：これらの「反対が示されているケース」とは何ですか？std展開が未定義の動作を引き起こさない状況がいくつかあります。これらすべての状況の詳細については、V1061診断のドキュメントページを参照してください。ただし、これらのケースの1つが、関数テンプレートの特殊化の追加であることが重要です。



なぜなら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最終作業ドラフトのセクション16.5.4.2.1を参照）。また、このコードはライブラリに属しているため、遅かれ早かれ-std = C ++ 20フラグを使用してビルドされる可能性があります。ちなみに、PVS-Studioは、コードで使用されている標準のバージョンを区別し、これに応じて、警告を発行するかどうかを決定します。自分自身を参照してください：C ++ 17たとえば、C ++ 20のための一例。



実際、はるかに簡単に行うことができます。エラーを修正するには、iter_swapの独自の定義を転送する必要があります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;
}

これで、コンパイラは、引数検索（ADL）に基づいて、iter_swap関数の必要なオーバーロードを個別に選択します。SizedIteratorクラスの場合、namespace utilのバージョンが呼び出され、他のタイプの場合、namespacestdのバージョンが呼び出されます。証拠はリンクにあります。さらに、ライブラリ関数内に「using」を追加する必要はありません。コードはすでにstd内にあるため、コンパイラは正しいオーバーロードを選択します。



そして-出来上がり-カスタムiter_swap関数は、「汚いトリック」やその他の魔術がなくても正常に機能します:)

結論

これで私の記事は終わりです。私が見つけたエラーがあなたにとって興味深いものであり、あなたがあなた自身にとって新しくて役に立つ何かを学んだことを願っています。ここまで読んだことがあるなら、エラーのないクリーンで整頓されたコードを心から願っています。バグがプロジェクトをバイパスするようにしましょう！



PS名前名stdに独自のコードを書くのは悪い習慣だと思います。どう思いますか？コメントでのご回答をお待ちしております。



C、C ++、C＃、またはJavaで開発していて、私のように静的分析のトピックに興味がある場合は、PVS-Studioを自分で試すことをお勧めします。リンクからダウンロードできます。

この記事を英語を話す聴衆と共有したい場合は、翻訳リンクを使用してください：GeorgeGribkov。DeepSpeechのコードを確認する、または名前名stdに書き込むべきではない理由。