静的コード解析
「Doom 3」「Quake 3: Arena」「Wolfenstein: Enemy Territory」を、静的コード解析ツールである、CppCheckとPVS-Studioに掛けて結果を比較した記事。
Cppcheck and PVS-Studio compared
PVS-Studioの宣伝記事だと思うので、CppCheckとの比較はさておき、検出されたバグの内容がなかなか興味深かったのでまとめる。
sizeofに関連するバグ
現象
- sizeof(型)とするべきところをsizeof(ポインタ)としてしまう
- 定数 とするべきところを sizeof(定数) としてしまう
- sizeof(*ポインタ) とするべきところを sizeof(&ポインタ) としてしまう
上の記事のなかではこれが最も多かった。memsetと一緒に現れることが多いのも特徴。 memset(ptr, 0, sizeof(*ptr)) とすべきところを memset(ptr, 0, sizeof(ptr)) としてしまっても、たいていは0にセットされるバイト数が少ないだけなので気づきにくいバグになる。 memcpyやmemcmpと一緒に現れることも考えられる。
「ポインタに関連するバグ」であると見ることもできる。
対策
- memset、memcpy、memcmpを使わない
sizeofの部分は注意深く書くしかないと思うが、
- 構造体を memset で初期化する
- 構造体を memcpy でコピーする
- 構造体を memcmp で比較する
のがこのバグの原因となっているし、やめるべきだと思う。
とするべきだろう。
単なるメモリブロックとして使っている場合は、std::vector を使うことができる。
メモリ確保と解放の不一致
現象
- malloc して free していない
- new[ ] で確保したのに delete で解放している
他にも
- new して delete していない
- new で確保したのに delete[ ] で解放している
- new で確保したのに free で解放している
- malloc で確保したのに delete で解放している
などのパターンも考えられる。
大きなくくりで言えば、「fopenしてfcloseしていない」も同じパターンだと見ることができる。
対策
たいていは shared_ptr、unique_ptr、vector を適切に使えば解決するだろう。 スマートポインタはデフォルトでdeleteによって解放するので、メモリを確保するのにmallocを使わないようにしたほうがいいと思う。
基本的に、コード中に delete が出てこないようにするのが目標である。
コピペミス
現象
コピペして一部編集するときに直しそこねたことが疑われるパターン
- if (...) {A} else {A}
- if (A) {...} else if (A) {...}
- 無駄な変数への代入
- 変数を間違えて再利用している
- for のループ変数を間違えている
など、一見して不可解で意図のわからない記述の原因となることが多い。 「変数に代入した直後に再度代入」や「then節とelse節が同じ」など、不具合は起こらないが全く意味のない記述となることもある。
また、その他の全てのバグの原因となっている可能性もある。
対策
コピペを減らすためには
- 関数化する
- template を使う
- マクロを使う(どうしてもという場合)
などが考えられる。他にも構造体やクラスを使うことによってコピペを避けることもできるだろう。
コピペが原因のバグを完全になくすことは難しいかもしれない。コピペをした時にそれを自覚して、何か避ける方法がないか考える癖を付ける必要があるだろう。
printf系関数に関連するバグ
現象
- printfでフォーマット文字列と渡す引数の不一致
- sprintfの出力と入力に同じバッファが指定されている
など。未定義動作なので、運が良ければ停止するが、さもなければこっそりメモリを破壊して動き続けたりする。
printf系の関数は
- 仕様が複雑
- コンパイル時のチェックが少ない
- 実装によって挙動が違う
などもあって鬼門である。
対策
printf系の関数の使用をさけるには、std::stream や boost::format を使うことが考えられる。
しかし、std::streamはイマイチ使いづらいし、Boostを全てのプロジェクトで使えるわけではないので悩ましい。
sprintfは std::string と std::to_string の組み合わせで避けられる場合もある。
演算子の優先順位に関連するバグ
現象
- if (!(flag & CONSTANT)) とするべきところを if (!flag & CONSTANT) としている
- (*ptr)++とするべきところを*ptr++としている
など。他にも数えきれないほどのパターンが考えられる。 複雑な条件式に含まれていると見落としがちである。
対策
優先順位をはっきりさせるために、丸括弧を多用する。優先順位が高くてカッコが必要ないときでも、カッコをつけるようにしたほうがいいだろう。
配列の範囲外へのアクセス
現象
- tbl[sizeof(tbl)] = 0
圧倒的に、「配列の最後の要素+1」にアクセスしていることが多い。 環境によっては実行時に検出してくれることもある。
対策
生の配列よりstd::arrayやstd::vectorを使う。 単に全要素にアクセスするときはRange-Based For を使う。
enumに関連するバグ
現象
など。コンパイラで警告を出してくれることが多いと思うが、いったんintにキャストしていたりするとそのチェックもできない。
